Change in osmo-pcu[master]: Fix Dl EGPRS data blocks being generated occasionally on GPRS TBFs

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pespin gerrit-no-reply at lists.osmocom.org
Mon Jan 25 17:14:34 UTC 2021


pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/22435 )


Change subject: Fix Dl EGPRS data blocks being generated occasionally on GPRS TBFs
......................................................................

Fix Dl EGPRS data blocks being generated occasionally on GPRS TBFs

Under some circumstances, it could happen that a DL TBF is created as a
GPRS TBF due to not yet having enough information of the MS, and only
after the TBF is created the PCU gains that information and upgrades the
MS mode to "EGPRS". Hence, there's the possibility to run into a
situation where a GPRS TBF is attached to a EGPRS MS.

It may also happen sometimes that despite the TBF and the MS be EGPRS,
there's need to further limit the DL MCS to use, eg. MCS1-4 (GMSK).

As a result, when asking for the current DL (M)CS to use, we must tell
the MS which kind of limitations we want to apply. The later reasoning
was already implemented when GPRS+EGPRS multiplexing was added, but the
former was not being checked. Hence, by further spreading through the
call stack the "req_kind_mode" we match both cases.

Related: OS#4973
Change-Id: Ic0276ce045660713129f0c72f1158a3321c5977f
---
M src/gprs_ms.c
M src/gprs_ms.h
M src/pcu_vty_functions.cpp
M src/tbf.cpp
M src/tbf_dl.cpp
M tests/ms/MsTest.cpp
6 files changed, 62 insertions(+), 36 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/35/22435/1

diff --git a/src/gprs_ms.c b/src/gprs_ms.c
index 0c4db89..3948abf 100644
--- a/src/gprs_ms.c
+++ b/src/gprs_ms.c
@@ -778,13 +778,46 @@
 	}
 }
 
-enum CodingScheme ms_current_cs_dl(const struct GprsMs *ms)
+/* req_mcs_kind acts as a set filter, where EGPRS means any and GPRS is the most restrictive */
+enum CodingScheme ms_current_cs_dl(const struct GprsMs *ms, enum mcs_kind req_mcs_kind)
 {
-	enum CodingScheme cs = ms->current_cs_dl;
+	enum CodingScheme orig_cs = ms->current_cs_dl;
+	struct gprs_rlcmac_bts *bts = ms->bts;
 	size_t unencoded_octets;
+	enum CodingScheme cs;
 
-	if (!ms->bts)
-		return cs;
+	/* It could be that a TBF requests a GPRS CS despite the MS currently
+	   being upgraded to EGPRS (hence reporting MCS). That could happen
+	   because the TBF was created early in the process where we didn't have
+	   yet enough information about the MS, and only AFTER it was created we
+	   upgraded the MS to be EGPRS capable.
+	   As a result, when  the MS is queried for the target CS here, we could be
+	   returning an MCS despite the current TBF being established as GPRS,
+	   but we rather stick to the TBF type we assigned to the MS rather than
+	   magically sending EGPRS data blocks to a GPRS TBF.
+	   It could also be that the caller requests specific MCS kind
+	   explicitly too due to scheduling restrictions (GPRS+EGPRS multiplexing). */
+	if (req_mcs_kind == EGPRS_GMSK && mcs_is_edge(orig_cs) && orig_cs > MCS4) {
+		cs = bts_cs_dl_is_supported(bts, MCS4) ? MCS4 :
+		     bts_cs_dl_is_supported(bts, MCS3) ? MCS3 :
+		     bts_cs_dl_is_supported(bts, MCS2) ? MCS2 :
+		     MCS1;
+	} else if (req_mcs_kind == GPRS && mcs_is_edge(orig_cs)) { /* GPRS */
+			int i;
+			cs = orig_cs > MCS4 ? MCS4 : orig_cs;
+			cs -= (MCS1 - CS1); /* MCSx -> CSx */
+			/* Find suitable CS starting from equivalent MCS which is supported by BTS: */
+			for (i = mcs_chan_code(cs); !bts_cs_dl_is_supported(bts, CS1 + i); i--);
+			OSMO_ASSERT(i >= 0 && i <= 3); /* CS1 is always supported */
+			cs = CS1 + i;
+	} else {
+		cs = orig_cs;
+	}
+
+	if (orig_cs != cs)
+		LOGPMS(ms, DRLCMACDL, LOGL_INFO, "MS (mode=%s) suggests transmitting "
+			"DL %s, downgrade to %s in order to match TBF & scheduler requirements\n",
+			mode_name(ms_mode(ms)), mcs_name(orig_cs), mcs_name(cs));
 
 	unencoded_octets = llc_queue_octets(&ms->llc_queue);
 
diff --git a/src/gprs_ms.h b/src/gprs_ms.h
index 12809f1..39d65de 100644
--- a/src/gprs_ms.h
+++ b/src/gprs_ms.h
@@ -114,7 +114,7 @@
 void ms_set_egprs_ms_class(struct GprsMs *ms, uint8_t ms_class_);
 void ms_set_ta(struct GprsMs *ms, uint8_t ta_);
 
-enum CodingScheme ms_current_cs_dl(const struct GprsMs *ms);
+enum CodingScheme ms_current_cs_dl(const struct GprsMs *ms, enum mcs_kind req_mcs_kind);
 enum CodingScheme ms_max_cs_ul(const struct GprsMs *ms);
 enum CodingScheme ms_max_cs_dl(const struct GprsMs *ms);
 void ms_set_current_cs_dl(struct GprsMs *ms, enum CodingScheme scheme);
diff --git a/src/pcu_vty_functions.cpp b/src/pcu_vty_functions.cpp
index 48780c0..c2250d9 100644
--- a/src/pcu_vty_functions.cpp
+++ b/src/pcu_vty_functions.cpp
@@ -133,7 +133,7 @@
 	vty_out(vty, "  Timing advance (TA):    %d%s", ms_ta(ms), VTY_NEWLINE);
 	vty_out(vty, "  Coding scheme uplink:   %s%s", mcs_name(ms_current_cs_ul(ms)),
 		VTY_NEWLINE);
-	vty_out(vty, "  Coding scheme downlink: %s%s", mcs_name(ms_current_cs_dl(ms)),
+	vty_out(vty, "  Coding scheme downlink: %s%s", mcs_name(ms_current_cs_dl(ms, ms_mode(ms))),
 		VTY_NEWLINE);
 	vty_out(vty, "  Mode:                   %s%s", mode_name(ms_mode(ms)), VTY_NEWLINE);
 	vty_out(vty, "  MS class:               %d%s", ms_ms_class(ms), VTY_NEWLINE);
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 248e8ef..d2d55f2 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -194,11 +194,12 @@
 enum CodingScheme gprs_rlcmac_tbf::current_cs() const
 {
 	enum CodingScheme cs;
+	enum mcs_kind req_mcs_kind = is_egprs_enabled() ? EGPRS : GPRS;
 
 	if (direction == GPRS_RLCMAC_UL_TBF)
 		cs = ms_current_cs_ul(m_ms);
 	else
-		cs = ms_current_cs_dl(m_ms);
+		cs = ms_current_cs_dl(m_ms, req_mcs_kind);
 
 	return cs;
 }
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index de43349..9562d86 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -448,29 +448,24 @@
 {
 	int bsn;
 	int data_len2, force_data_len = -1;
-	enum CodingScheme force_cs = UNKNOWN;
+	enum CodingScheme tx_cs;
+
+	/* Scheduler may be fine with sending any kind of data, but if
+	   the selected TBF is GPRS-only, then let's filter out EGPRS
+	   here */
+	if (!is_egprs_enabled())
+		req_mcs_kind = GPRS;
 
 	/* search for a nacked or resend marked bsn */
 	bsn = m_window.resend_needed();
 
 	if (previous_bsn >= 0) {
-		force_cs = m_rlc.block(previous_bsn)->cs_current_trans;
-		if (!mcs_is_edge(force_cs))
+		tx_cs = m_rlc.block(previous_bsn)->cs_current_trans;
+		if (!mcs_is_edge(tx_cs))
 			return -1;
 		force_data_len = m_rlc.block(previous_bsn)->len;
-	} else if (bsn < 0 && is_egprs_enabled() && req_mcs_kind == EGPRS_GMSK) {
-		/* New data to be sent for EGPRS TBF but we are required to downgrade to
-		 * MCS1-4, because USF for GPRS-only MS will be sent */
-		force_cs = ms_current_cs_dl(m_ms);
-		if (force_cs > MCS4) {
-			force_cs = bts_cs_dl_is_supported(bts, MCS4) ? MCS4 :
-				   bts_cs_dl_is_supported(bts, MCS3) ? MCS3 :
-				   bts_cs_dl_is_supported(bts, MCS2) ? MCS2 :
-				   MCS1;
-			LOGPTBFDL(this, LOGL_DEBUG,
-				  "Force downgrading DL %s -> %s due to USF for GPRS-only MS\n",
-				  mcs_name(ms_current_cs_dl(m_ms)), mcs_name(force_cs));
-		}
+	} else {
+		tx_cs = ms_current_cs_dl(ms(), req_mcs_kind);
 	}
 
 	if (bsn >= 0) {
@@ -484,15 +479,14 @@
 
 		if (is_egprs_enabled()) {
 			/* Table 8.1.1.2 and Table 8.1.1.1 of 44.060 */
-			m_rlc.block(bsn)->cs_current_trans = get_retx_mcs(m_rlc.block(bsn)->cs_init,
-									  ms_current_cs_dl(ms()),
+			m_rlc.block(bsn)->cs_current_trans = get_retx_mcs(m_rlc.block(bsn)->cs_init, tx_cs,
 									  bts->pcu->vty.dl_arq_type == EGPRS_ARQ1);
 
 			LOGPTBFDL(this, LOGL_DEBUG,
 				  "initial_cs_dl(%s) last_mcs(%s) demanded_mcs(%s) cs_trans(%s) arq_type(%d) bsn(%d)\n",
 				  mcs_name(m_rlc.block(bsn)->cs_init),
 				  mcs_name(m_rlc.block(bsn)->cs_last),
-				  mcs_name(ms_current_cs_dl(ms())),
+				  mcs_name(tx_cs),
 				  mcs_name(m_rlc.block(bsn)->cs_current_trans),
 				  the_pcu->vty.dl_arq_type, bsn);
 
@@ -537,14 +531,12 @@
 			return take_next_bsn(fn, previous_bsn, req_mcs_kind, may_combine);
 	} else if (have_data()) {
 		/* The window has space left, generate new bsn */
-		enum CodingScheme new_cs;
-		new_cs = force_cs ? force_cs : current_cs();
 		LOGPTBFDL(this, LOGL_DEBUG,
 			  "Sending new block at BSN %d, CS=%s%s\n",
-			  m_window.v_s(), mcs_name(new_cs),
-			  force_cs ? " (forced)" : "");
+			  m_window.v_s(), mcs_name(tx_cs),
+			  force_data_len != -1 ? " (forced)" : "");
 
-		bsn = create_new_bsn(fn, new_cs);
+		bsn = create_new_bsn(fn, tx_cs);
 	} else if (bts->pcu->vty.dl_tbf_preemptive_retransmission && !m_window.window_empty()) {
 		/* The window contains unacked packages, but not acked.
 		 * Mark unacked bsns as RESEND */
@@ -558,8 +550,8 @@
 		/* Nothing left to send, create dummy LLC commands */
 		LOGPTBFDL(this, LOGL_DEBUG,
 			  "Sending new dummy block at BSN %d, CS=%s\n",
-			  m_window.v_s(), mcs_name(current_cs()));
-		bsn = create_new_bsn(fn, current_cs());
+			  m_window.v_s(), mcs_name(tx_cs));
+		bsn = create_new_bsn(fn, tx_cs);
 		/* Don't send a second block, so don't set cs_current_trans */
 	}
 
diff --git a/tests/ms/MsTest.cpp b/tests/ms/MsTest.cpp
index f459e99..677c695 100644
--- a/tests/ms/MsTest.cpp
+++ b/tests/ms/MsTest.cpp
@@ -522,11 +522,11 @@
 
 	OSMO_ASSERT(!ms_is_idle(ms));
 
-	OSMO_ASSERT(mcs_chan_code(ms_current_cs_dl(ms)) == 3);
+	OSMO_ASSERT(mcs_chan_code(ms_current_cs_dl(ms, ms_mode(ms))) == 3);
 
 	the_pcu->vty.cs_downgrade_threshold = 200;
 
-	OSMO_ASSERT(mcs_chan_code(ms_current_cs_dl(ms)) == 2);
+	OSMO_ASSERT(mcs_chan_code(ms_current_cs_dl(ms, ms_mode(ms))) == 2);
 
 	talloc_free(dl_tbf);
 	talloc_free(bts);
@@ -536,7 +536,7 @@
 static void dump_ms(const GprsMs *ms, const char *pref)
 {
 	printf("%s MS DL %s/%s, UL %s/%s, mode %s, <%s>\n", pref,
-	       mcs_name(ms_current_cs_dl(ms)), mcs_name(ms_max_cs_dl(ms)),
+	       mcs_name(ms_current_cs_dl(ms, ms_mode(ms))), mcs_name(ms_max_cs_dl(ms)),
 	       mcs_name(ms_current_cs_ul(ms)), mcs_name(ms_max_cs_ul(ms)),
 	       mode_name(ms_mode(ms)),
 	       ms_is_idle(ms) ? "IDLE" : "ACTIVE");

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/22435
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Ic0276ce045660713129f0c72f1158a3321c5977f
Gerrit-Change-Number: 22435
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210125/8bd3a985/attachment.htm>


More information about the gerrit-log mailing list