<p>pespin <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22435">View Change</a></p><div style="white-space:pre-wrap">Approvals:
laforge: Looks good to me, but someone else must approve
osmith: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Fix Dl EGPRS data blocks being generated occasionally on GPRS TBFs<br><br>Under some circumstances, it could happen that a DL TBF is created as a<br>GPRS TBF due to not yet having enough information of the MS, and only<br>after the TBF is created the PCU gains that information and upgrades the<br>MS mode to "EGPRS". Hence, there's the possibility to run into a<br>situation where a GPRS TBF is attached to a EGPRS MS.<br><br>It may also happen sometimes that despite the TBF and the MS be EGPRS,<br>there's need to further limit the DL MCS to use, eg. MCS1-4 (GMSK).<br><br>As a result, when asking for the current DL (M)CS to use, we must tell<br>the MS which kind of limitations we want to apply. The later reasoning<br>was already implemented when GPRS+EGPRS multiplexing was added, but the<br>former was not being checked. Hence, by further spreading through the<br>call stack the "req_kind_mode" we match both cases.<br><br>Related: OS#4973<br>Change-Id: Ic0276ce045660713129f0c72f1158a3321c5977f<br>---<br>M src/gprs_ms.c<br>M src/gprs_ms.h<br>M src/pcu_vty_functions.cpp<br>M src/tbf.cpp<br>M src/tbf_dl.cpp<br>M tests/ms/MsTest.cpp<br>6 files changed, 62 insertions(+), 36 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/gprs_ms.c b/src/gprs_ms.c</span><br><span>index 0c4db89..3948abf 100644</span><br><span>--- a/src/gprs_ms.c</span><br><span>+++ b/src/gprs_ms.c</span><br><span>@@ -778,13 +778,46 @@</span><br><span> }</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-enum CodingScheme ms_current_cs_dl(const struct GprsMs *ms)</span><br><span style="color: hsl(120, 100%, 40%);">+/* req_mcs_kind acts as a set filter, where EGPRS means any and GPRS is the most restrictive */</span><br><span style="color: hsl(120, 100%, 40%);">+enum CodingScheme ms_current_cs_dl(const struct GprsMs *ms, enum mcs_kind req_mcs_kind)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- enum CodingScheme cs = ms->current_cs_dl;</span><br><span style="color: hsl(120, 100%, 40%);">+ enum CodingScheme orig_cs = ms->current_cs_dl;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_rlcmac_bts *bts = ms->bts;</span><br><span> size_t unencoded_octets;</span><br><span style="color: hsl(120, 100%, 40%);">+ enum CodingScheme cs;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!ms->bts)</span><br><span style="color: hsl(0, 100%, 40%);">- return cs;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* It could be that a TBF requests a GPRS CS despite the MS currently</span><br><span style="color: hsl(120, 100%, 40%);">+ being upgraded to EGPRS (hence reporting MCS). That could happen</span><br><span style="color: hsl(120, 100%, 40%);">+ because the TBF was created early in the process where we didn't have</span><br><span style="color: hsl(120, 100%, 40%);">+ yet enough information about the MS, and only AFTER it was created we</span><br><span style="color: hsl(120, 100%, 40%);">+ upgraded the MS to be EGPRS capable.</span><br><span style="color: hsl(120, 100%, 40%);">+ As a result, when the MS is queried for the target CS here, we could be</span><br><span style="color: hsl(120, 100%, 40%);">+ returning an MCS despite the current TBF being established as GPRS,</span><br><span style="color: hsl(120, 100%, 40%);">+ but we rather stick to the TBF type we assigned to the MS rather than</span><br><span style="color: hsl(120, 100%, 40%);">+ magically sending EGPRS data blocks to a GPRS TBF.</span><br><span style="color: hsl(120, 100%, 40%);">+ It could also be that the caller requests specific MCS kind</span><br><span style="color: hsl(120, 100%, 40%);">+ explicitly too due to scheduling restrictions (GPRS+EGPRS multiplexing). */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (req_mcs_kind == EGPRS_GMSK && mcs_is_edge(orig_cs) && orig_cs > MCS4) {</span><br><span style="color: hsl(120, 100%, 40%);">+ cs = bts_cs_dl_is_supported(bts, MCS4) ? MCS4 :</span><br><span style="color: hsl(120, 100%, 40%);">+ bts_cs_dl_is_supported(bts, MCS3) ? MCS3 :</span><br><span style="color: hsl(120, 100%, 40%);">+ bts_cs_dl_is_supported(bts, MCS2) ? MCS2 :</span><br><span style="color: hsl(120, 100%, 40%);">+ MCS1;</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (req_mcs_kind == GPRS && mcs_is_edge(orig_cs)) { /* GPRS */</span><br><span style="color: hsl(120, 100%, 40%);">+ int i;</span><br><span style="color: hsl(120, 100%, 40%);">+ cs = orig_cs > MCS4 ? MCS4 : orig_cs;</span><br><span style="color: hsl(120, 100%, 40%);">+ cs -= (MCS1 - CS1); /* MCSx -> CSx */</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Find suitable CS starting from equivalent MCS which is supported by BTS: */</span><br><span style="color: hsl(120, 100%, 40%);">+ for (i = mcs_chan_code(cs); !bts_cs_dl_is_supported(bts, CS1 + i); i--);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(i >= 0 && i <= 3); /* CS1 is always supported */</span><br><span style="color: hsl(120, 100%, 40%);">+ cs = CS1 + i;</span><br><span style="color: hsl(120, 100%, 40%);">+ } else {</span><br><span style="color: hsl(120, 100%, 40%);">+ cs = orig_cs;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (orig_cs != cs)</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPMS(ms, DRLCMACDL, LOGL_INFO, "MS (mode=%s) suggests transmitting "</span><br><span style="color: hsl(120, 100%, 40%);">+ "DL %s, downgrade to %s in order to match TBF & scheduler requirements\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ mode_name(ms_mode(ms)), mcs_name(orig_cs), mcs_name(cs));</span><br><span> </span><br><span> unencoded_octets = llc_queue_octets(&ms->llc_queue);</span><br><span> </span><br><span>diff --git a/src/gprs_ms.h b/src/gprs_ms.h</span><br><span>index 12809f1..39d65de 100644</span><br><span>--- a/src/gprs_ms.h</span><br><span>+++ b/src/gprs_ms.h</span><br><span>@@ -114,7 +114,7 @@</span><br><span> void ms_set_egprs_ms_class(struct GprsMs *ms, uint8_t ms_class_);</span><br><span> void ms_set_ta(struct GprsMs *ms, uint8_t ta_);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-enum CodingScheme ms_current_cs_dl(const struct GprsMs *ms);</span><br><span style="color: hsl(120, 100%, 40%);">+enum CodingScheme ms_current_cs_dl(const struct GprsMs *ms, enum mcs_kind req_mcs_kind);</span><br><span> enum CodingScheme ms_max_cs_ul(const struct GprsMs *ms);</span><br><span> enum CodingScheme ms_max_cs_dl(const struct GprsMs *ms);</span><br><span> void ms_set_current_cs_dl(struct GprsMs *ms, enum CodingScheme scheme);</span><br><span>diff --git a/src/pcu_vty_functions.cpp b/src/pcu_vty_functions.cpp</span><br><span>index 48780c0..c2250d9 100644</span><br><span>--- a/src/pcu_vty_functions.cpp</span><br><span>+++ b/src/pcu_vty_functions.cpp</span><br><span>@@ -133,7 +133,7 @@</span><br><span> vty_out(vty, " Timing advance (TA): %d%s", ms_ta(ms), VTY_NEWLINE);</span><br><span> vty_out(vty, " Coding scheme uplink: %s%s", mcs_name(ms_current_cs_ul(ms)),</span><br><span> VTY_NEWLINE);</span><br><span style="color: hsl(0, 100%, 40%);">- vty_out(vty, " Coding scheme downlink: %s%s", mcs_name(ms_current_cs_dl(ms)),</span><br><span style="color: hsl(120, 100%, 40%);">+ vty_out(vty, " Coding scheme downlink: %s%s", mcs_name(ms_current_cs_dl(ms, ms_mode(ms))),</span><br><span> VTY_NEWLINE);</span><br><span> vty_out(vty, " Mode: %s%s", mode_name(ms_mode(ms)), VTY_NEWLINE);</span><br><span> vty_out(vty, " MS class: %d%s", ms_ms_class(ms), VTY_NEWLINE);</span><br><span>diff --git a/src/tbf.cpp b/src/tbf.cpp</span><br><span>index 248e8ef..d2d55f2 100644</span><br><span>--- a/src/tbf.cpp</span><br><span>+++ b/src/tbf.cpp</span><br><span>@@ -194,11 +194,12 @@</span><br><span> enum CodingScheme gprs_rlcmac_tbf::current_cs() const</span><br><span> {</span><br><span> enum CodingScheme cs;</span><br><span style="color: hsl(120, 100%, 40%);">+ enum mcs_kind req_mcs_kind = is_egprs_enabled() ? EGPRS : GPRS;</span><br><span> </span><br><span> if (direction == GPRS_RLCMAC_UL_TBF)</span><br><span> cs = ms_current_cs_ul(m_ms);</span><br><span> else</span><br><span style="color: hsl(0, 100%, 40%);">- cs = ms_current_cs_dl(m_ms);</span><br><span style="color: hsl(120, 100%, 40%);">+ cs = ms_current_cs_dl(m_ms, req_mcs_kind);</span><br><span> </span><br><span> return cs;</span><br><span> }</span><br><span>diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp</span><br><span>index de43349..9562d86 100644</span><br><span>--- a/src/tbf_dl.cpp</span><br><span>+++ b/src/tbf_dl.cpp</span><br><span>@@ -448,29 +448,24 @@</span><br><span> {</span><br><span> int bsn;</span><br><span> int data_len2, force_data_len = -1;</span><br><span style="color: hsl(0, 100%, 40%);">- enum CodingScheme force_cs = UNKNOWN;</span><br><span style="color: hsl(120, 100%, 40%);">+ enum CodingScheme tx_cs;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Scheduler may be fine with sending any kind of data, but if</span><br><span style="color: hsl(120, 100%, 40%);">+ the selected TBF is GPRS-only, then let's filter out EGPRS</span><br><span style="color: hsl(120, 100%, 40%);">+ here */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!is_egprs_enabled())</span><br><span style="color: hsl(120, 100%, 40%);">+ req_mcs_kind = GPRS;</span><br><span> </span><br><span> /* search for a nacked or resend marked bsn */</span><br><span> bsn = m_window.resend_needed();</span><br><span> </span><br><span> if (previous_bsn >= 0) {</span><br><span style="color: hsl(0, 100%, 40%);">- force_cs = m_rlc.block(previous_bsn)->cs_current_trans;</span><br><span style="color: hsl(0, 100%, 40%);">- if (!mcs_is_edge(force_cs))</span><br><span style="color: hsl(120, 100%, 40%);">+ tx_cs = m_rlc.block(previous_bsn)->cs_current_trans;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!mcs_is_edge(tx_cs))</span><br><span> return -1;</span><br><span> force_data_len = m_rlc.block(previous_bsn)->len;</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (bsn < 0 && is_egprs_enabled() && req_mcs_kind == EGPRS_GMSK) {</span><br><span style="color: hsl(0, 100%, 40%);">- /* New data to be sent for EGPRS TBF but we are required to downgrade to</span><br><span style="color: hsl(0, 100%, 40%);">- * MCS1-4, because USF for GPRS-only MS will be sent */</span><br><span style="color: hsl(0, 100%, 40%);">- force_cs = ms_current_cs_dl(m_ms);</span><br><span style="color: hsl(0, 100%, 40%);">- if (force_cs > MCS4) {</span><br><span style="color: hsl(0, 100%, 40%);">- force_cs = bts_cs_dl_is_supported(bts, MCS4) ? MCS4 :</span><br><span style="color: hsl(0, 100%, 40%);">- bts_cs_dl_is_supported(bts, MCS3) ? MCS3 :</span><br><span style="color: hsl(0, 100%, 40%);">- bts_cs_dl_is_supported(bts, MCS2) ? MCS2 :</span><br><span style="color: hsl(0, 100%, 40%);">- MCS1;</span><br><span style="color: hsl(0, 100%, 40%);">- LOGPTBFDL(this, LOGL_DEBUG,</span><br><span style="color: hsl(0, 100%, 40%);">- "Force downgrading DL %s -> %s due to USF for GPRS-only MS\n",</span><br><span style="color: hsl(0, 100%, 40%);">- mcs_name(ms_current_cs_dl(m_ms)), mcs_name(force_cs));</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(120, 100%, 40%);">+ } else {</span><br><span style="color: hsl(120, 100%, 40%);">+ tx_cs = ms_current_cs_dl(ms(), req_mcs_kind);</span><br><span> }</span><br><span> </span><br><span> if (bsn >= 0) {</span><br><span>@@ -484,15 +479,14 @@</span><br><span> </span><br><span> if (is_egprs_enabled()) {</span><br><span> /* Table 8.1.1.2 and Table 8.1.1.1 of 44.060 */</span><br><span style="color: hsl(0, 100%, 40%);">- m_rlc.block(bsn)->cs_current_trans = get_retx_mcs(m_rlc.block(bsn)->cs_init,</span><br><span style="color: hsl(0, 100%, 40%);">- ms_current_cs_dl(ms()),</span><br><span style="color: hsl(120, 100%, 40%);">+ m_rlc.block(bsn)->cs_current_trans = get_retx_mcs(m_rlc.block(bsn)->cs_init, tx_cs,</span><br><span> bts->pcu->vty.dl_arq_type == EGPRS_ARQ1);</span><br><span> </span><br><span> LOGPTBFDL(this, LOGL_DEBUG,</span><br><span> "initial_cs_dl(%s) last_mcs(%s) demanded_mcs(%s) cs_trans(%s) arq_type(%d) bsn(%d)\n",</span><br><span> mcs_name(m_rlc.block(bsn)->cs_init),</span><br><span> mcs_name(m_rlc.block(bsn)->cs_last),</span><br><span style="color: hsl(0, 100%, 40%);">- mcs_name(ms_current_cs_dl(ms())),</span><br><span style="color: hsl(120, 100%, 40%);">+ mcs_name(tx_cs),</span><br><span> mcs_name(m_rlc.block(bsn)->cs_current_trans),</span><br><span> the_pcu->vty.dl_arq_type, bsn);</span><br><span> </span><br><span>@@ -537,14 +531,12 @@</span><br><span> return take_next_bsn(fn, previous_bsn, req_mcs_kind, may_combine);</span><br><span> } else if (have_data()) {</span><br><span> /* The window has space left, generate new bsn */</span><br><span style="color: hsl(0, 100%, 40%);">- enum CodingScheme new_cs;</span><br><span style="color: hsl(0, 100%, 40%);">- new_cs = force_cs ? force_cs : current_cs();</span><br><span> LOGPTBFDL(this, LOGL_DEBUG,</span><br><span> "Sending new block at BSN %d, CS=%s%s\n",</span><br><span style="color: hsl(0, 100%, 40%);">- m_window.v_s(), mcs_name(new_cs),</span><br><span style="color: hsl(0, 100%, 40%);">- force_cs ? " (forced)" : "");</span><br><span style="color: hsl(120, 100%, 40%);">+ m_window.v_s(), mcs_name(tx_cs),</span><br><span style="color: hsl(120, 100%, 40%);">+ force_data_len != -1 ? " (forced)" : "");</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- bsn = create_new_bsn(fn, new_cs);</span><br><span style="color: hsl(120, 100%, 40%);">+ bsn = create_new_bsn(fn, tx_cs);</span><br><span> } else if (bts->pcu->vty.dl_tbf_preemptive_retransmission && !m_window.window_empty()) {</span><br><span> /* The window contains unacked packages, but not acked.</span><br><span> * Mark unacked bsns as RESEND */</span><br><span>@@ -558,8 +550,8 @@</span><br><span> /* Nothing left to send, create dummy LLC commands */</span><br><span> LOGPTBFDL(this, LOGL_DEBUG,</span><br><span> "Sending new dummy block at BSN %d, CS=%s\n",</span><br><span style="color: hsl(0, 100%, 40%);">- m_window.v_s(), mcs_name(current_cs()));</span><br><span style="color: hsl(0, 100%, 40%);">- bsn = create_new_bsn(fn, current_cs());</span><br><span style="color: hsl(120, 100%, 40%);">+ m_window.v_s(), mcs_name(tx_cs));</span><br><span style="color: hsl(120, 100%, 40%);">+ bsn = create_new_bsn(fn, tx_cs);</span><br><span> /* Don't send a second block, so don't set cs_current_trans */</span><br><span> }</span><br><span> </span><br><span>diff --git a/tests/ms/MsTest.cpp b/tests/ms/MsTest.cpp</span><br><span>index f459e99..677c695 100644</span><br><span>--- a/tests/ms/MsTest.cpp</span><br><span>+++ b/tests/ms/MsTest.cpp</span><br><span>@@ -522,11 +522,11 @@</span><br><span> </span><br><span> OSMO_ASSERT(!ms_is_idle(ms));</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- OSMO_ASSERT(mcs_chan_code(ms_current_cs_dl(ms)) == 3);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(mcs_chan_code(ms_current_cs_dl(ms, ms_mode(ms))) == 3);</span><br><span> </span><br><span> the_pcu->vty.cs_downgrade_threshold = 200;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- OSMO_ASSERT(mcs_chan_code(ms_current_cs_dl(ms)) == 2);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(mcs_chan_code(ms_current_cs_dl(ms, ms_mode(ms))) == 2);</span><br><span> </span><br><span> talloc_free(dl_tbf);</span><br><span> talloc_free(bts);</span><br><span>@@ -536,7 +536,7 @@</span><br><span> static void dump_ms(const GprsMs *ms, const char *pref)</span><br><span> {</span><br><span> printf("%s MS DL %s/%s, UL %s/%s, mode %s, <%s>\n", pref,</span><br><span style="color: hsl(0, 100%, 40%);">- mcs_name(ms_current_cs_dl(ms)), mcs_name(ms_max_cs_dl(ms)),</span><br><span style="color: hsl(120, 100%, 40%);">+ mcs_name(ms_current_cs_dl(ms, ms_mode(ms))), mcs_name(ms_max_cs_dl(ms)),</span><br><span> mcs_name(ms_current_cs_ul(ms)), mcs_name(ms_max_cs_ul(ms)),</span><br><span> mode_name(ms_mode(ms)),</span><br><span> ms_is_idle(ms) ? "IDLE" : "ACTIVE");</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22435">change 22435</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-pcu/+/22435"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ic0276ce045660713129f0c72f1158a3321c5977f </div>
<div style="display:none"> Gerrit-Change-Number: 22435 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>