pespin has uploaded this change for review.
Avoid loosing DL tbf from old_ms when rx premier UL data
Scenario:
MS has UL TBF to attach to the network in FINISHED state, PCU received DL data
to continue with registration and creates a DL TBF in ASSIGN state (through PCH).
At that point, the MS creates a new UL TBF through RACH request and PCU creates
it with a new MS object (because TLLI is not known at that time).
When first UL data is received, due to contention resolution the PCU earns the
TLLI and finds out it already has another older MS object with that TLLI, hence
a duplicate and a merge must be done.
During that merge of the 2 MS objects, the DL TBF in ASSIGN state was
being freed. Let's actually not free it since we'll need to tell that DL
TBF in this scenario that it should instead re-start assignment through
PACCH.
Otherwise the MS keeps trying to register but the PCU is never able to
establish a DL TBF because it keeps assigning it through PCH instead of
PACCH.
Related: OS#5700
Change-Id: I9a989af89f9469d7894b9e70a2ea01989b9dbb75
---
M src/gprs_ms.c
M src/tbf.cpp
M src/tbf_dl.cpp
M tests/tbf/TbfTest.cpp
M tests/tbf/TbfTest.err
5 files changed, 44 insertions(+), 46 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/45/29845/1
diff --git a/src/gprs_ms.c b/src/gprs_ms.c
index 239824f..be5b867 100644
--- a/src/gprs_ms.c
+++ b/src/gprs_ms.c
@@ -383,9 +383,17 @@
ms->imsi[0] = '\0';
}
-static void ms_merge_old_ms(struct GprsMs *ms, struct GprsMs *old_ms)
+/* Merge 'old_ms' object into 'ms' object.
+ * 'old_ms' may be freed during the call to this function, don't use the pointer to it afterwards */
+void ms_merge_and_clear_ms(struct GprsMs *ms, struct GprsMs *old_ms)
{
+ char old_ms_name[128];
OSMO_ASSERT(old_ms != ms);
+ ms_ref(old_ms);
+
+ ms_name_buf(old_ms, old_ms_name, sizeof(old_ms_name));
+
+ LOGPMS(ms, DRLCMAC, LOGL_INFO, "Merge MS: %s\n", old_ms_name);
if (strlen(ms_imsi(ms)) == 0 && strlen(ms_imsi(old_ms)) != 0)
osmo_strlcpy(ms->imsi, ms_imsi(old_ms), sizeof(ms->imsi));
@@ -398,14 +406,14 @@
llc_queue_move_and_merge(&ms->llc_queue, &old_ms->llc_queue);
- ms_reset(old_ms);
-}
-
-void ms_merge_and_clear_ms(struct GprsMs *ms, struct GprsMs *old_ms)
-{
- OSMO_ASSERT(old_ms != ms);
-
- ms_ref(old_ms);
+ if (!ms_dl_tbf(ms) && ms_dl_tbf(old_ms)) {
+ struct gprs_rlcmac_tbf *dl_tbf = (struct gprs_rlcmac_tbf *)ms_dl_tbf(old_ms);
+ LOGPTBFDL(dl_tbf, LOGL_NOTICE,
+ "Merge MS: Move DL TBF: %s => %s\n",
+ old_ms_name, ms_name(ms));
+ /* Move the DL TBF to the new MS */
+ tbf_set_ms(dl_tbf, ms);
+ }
/* Clean up the old MS object */
/* TODO: Use timer? */
@@ -414,7 +422,7 @@
if (ms_dl_tbf(old_ms) && !tbf_timers_pending((struct gprs_rlcmac_tbf *)ms_dl_tbf(old_ms), T_MAX))
tbf_free((struct gprs_rlcmac_tbf *)ms_dl_tbf(old_ms));
- ms_merge_old_ms(ms, old_ms);
+ ms_reset(old_ms);
ms_unref(old_ms);
}
@@ -508,6 +516,7 @@
imsi, ms_tlli(old_ms));
ms_merge_and_clear_ms(ms, old_ms);
+ /* old_ms may no longer be available here */
}
diff --git a/src/tbf.cpp b/src/tbf.cpp
index cb5d9f3..cd273af 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -215,25 +215,25 @@
void gprs_rlcmac_tbf::update_ms(uint32_t tlli, enum gprs_rlcmac_tbf_direction dir)
{
+ GprsMs *old_ms = NULL;
+
if (tlli == GSM_RESERVED_TMSI)
return;
- /* TODO: When the TLLI does not match the ms, check if there is another
+ /* When the TLLI does not match the ms, check if there is another
* MS object that belongs to that TLLI and if yes make sure one of them
- * gets deleted. This is the same problem that can arise with
- * IMSI in dl_tbf_handle() so there should be a unified solution */
- if (!ms_check_tlli(ms(), tlli)) {
- GprsMs *old_ms;
-
+ * gets deleted. */
+ if (!ms_check_tlli(ms(), tlli))
old_ms = bts_ms_store(bts)->get_ms(tlli, GSM_RESERVED_TMSI, NULL);
- if (old_ms)
- ms_merge_and_clear_ms(ms(), old_ms);
- }
if (dir == GPRS_RLCMAC_UL_TBF)
ms_set_tlli(ms(), tlli);
else
ms_confirm_tlli(ms(), tlli);
+
+ if (old_ms)
+ ms_merge_and_clear_ms(ms(), old_ms);
+ /* old_ms may no longer be available here */
}
static void tbf_unlink_pdch(struct gprs_rlcmac_tbf *tbf)
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index 67ac573..e5fdf3e 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -302,20 +302,8 @@
LOGP(DTBF, LOGL_NOTICE,
"There is a new MS object for the same MS: (0x%08x, '%s') -> (0x%08x, '%s')\n",
ms_tlli(ms_old), ms_imsi(ms_old), ms_tlli(ms), ms_imsi(ms));
-
- ms_ref(ms_old);
-
- if (!ms_dl_tbf(ms) && ms_dl_tbf(ms_old)) {
- LOGP(DTBF, LOGL_NOTICE,
- "IMSI %s, old TBF %s: moving DL TBF to new MS object\n",
- imsi ? : "unknown", ms_dl_tbf(ms_old)->name());
- dl_tbf = ms_dl_tbf(ms_old);
- /* Move the DL TBF to the new MS */
- dl_tbf->set_ms(ms);
- }
ms_merge_and_clear_ms(ms, ms_old);
-
- ms_unref(ms_old);
+ /* old_ms may no longer be available here */
}
}
diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index c18fa77..ff56e6f 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -2124,8 +2124,9 @@
OSMO_ASSERT(ms2 == ms);
OSMO_ASSERT(ms1 != ms);
- /* DL TBF should be removed */
- OSMO_ASSERT(!ms_dl_tbf(ms));
+ /* DL TBF should still be alive in assign state */
+ dl_tbf = ms_dl_tbf(ms);
+ OSMO_ASSERT(dl_tbf && tbf_state(dl_tbf) == TBF_ST_ASSIGN);
/* No queued packets should be lost */
OSMO_ASSERT(llc_queue_size(ms_llc_queue(ms)) == 2);
diff --git a/tests/tbf/TbfTest.err b/tests/tbf/TbfTest.err
index 0cb024b..ba57cc0 100644
--- a/tests/tbf/TbfTest.err
+++ b/tests/tbf/TbfTest.err
@@ -617,6 +617,7 @@
Modifying MS object, TLLI = 0xf1000001, IMSI '001001000000001' -> '001001000000002'
Modifying MS object, TLLI = 0xf1000002, IMSI '' -> '001001000000002'
MS(TLLI=0xf1000002, IMSI=, TA=0, 45/0, DL) IMSI '001001000000002' was already assigned to another MS object: TLLI = 0xf1000001, that IMSI will be removed
+MS(TLLI=0xf1000002, IMSI=, TA=0, 45/0, DL) Merge MS: MS(TLLI=0xf1000001, IMSI=001001000000002, TA=0, 45/0, DL)
TBF(TFI=0 TLLI=0xf1000001 DIR=DL STATE=FLOW) free
PDCH(bts=0,trx=0,ts=4) Detaching TBF(TFI=0 TLLI=0xf1000001 DIR=DL STATE=FLOW), 2 TBFs, USFs = 00, TFIs = 00000003.
MS(TLLI=0xf1000001, IMSI=001001000000002, TA=0, 45/0,) Detaching TBF: TBF(TFI=0 TLLI=0xf1000001 DIR=DL STATE=FLOW)
@@ -2228,9 +2229,10 @@
TBF(TFI=1 TLLI=0xf5667788 DIR=UL STATE=FLOW) No gaps in received block, last block: BSN=0 CV=15
Old MS: TLLI = 0xf1223344, TA = 7, IMSI = 0011223344, LLC = 0
There is a new MS object for the same MS: (0xf1223344, '0011223344') -> (0xf5667788, '')
-IMSI 0011223344, old TBF TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=FINISHED): moving DL TBF to new MS object
+MS(TLLI=0xf5667788, IMSI=, TA=7, 1/0, UL) Merge MS: MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, UL DL)
+TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=FINISHED) Merge MS: Move DL TBF: MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, UL DL) => MS(TLLI=0xf5667788, IMSI=0011223344, TA=7, 1/0, UL)
MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, UL) Detaching TBF: TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=FINISHED)
-MS(TLLI=0xf5667788, IMSI=, TA=7, 1/0, UL) Attaching DL TBF: TBF(TFI=0 TLLI=0xf5667788 DIR=DL STATE=FINISHED)
+MS(TLLI=0xf5667788, IMSI=0011223344, TA=7, 1/0, UL) Attaching DL TBF: TBF(TFI=0 TLLI=0xf5667788 DIR=DL STATE=FINISHED)
TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW) free
PDCH(bts=0,trx=0,ts=7) Detaching TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW), 2 TBFs, USFs = 03, TFIs = 00000003.
MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0,) Detaching TBF: TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW)
@@ -2572,17 +2574,14 @@
TBF(TFI=0 TLLI=0xffffffff DIR=UL STATE=FLOW) BSN 0 storing in window (0..63)
TBF(TFI=0 TLLI=0xffffffff DIR=UL STATE=FLOW) data_length=20, data=f1 22 33 44 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
TBF(TFI=0 TLLI=0xffffffff DIR=UL STATE=FLOW) Decoded premier TLLI=0xf1223344 of UL DATA TFI=0.
-TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=ASSIGN) free
-PDCH(bts=0,trx=0,ts=7) Detaching TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=ASSIGN), 1 TBFs, USFs = 01, TFIs = 00000001.
+Modifying MS object, UL TLLI: 0xffffffff -> 0xf1223344, not yet confirmed
+MS(TLLI=0xf1223344, IMSI=, TA=7, 0/0, UL) Merge MS: MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, DL)
+Modifying MS object, TLLI = 0xf1223344, MS class 0 -> 1
+TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=ASSIGN) Merge MS: Move DL TBF: MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, DL) => MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, UL)
MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0,) Detaching TBF: TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=ASSIGN)
-********** DL-TBF ends here **********
-TBF(DL-TFI_0){ASSIGN}: Deallocated
-UL_ASS_TBF(DL-TFI_0){NONE}: Deallocated
-DL_ASS_TBF(DL-TFI_0){NONE}: Deallocated
-Modifying MS object, TLLI = 0xffffffff, MS class 0 -> 1
+MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, UL) Attaching DL TBF: TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=ASSIGN)
MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0,) Clearing MS object
MS(TLLI=0xffffffff, IMSI=, TA=7, 1/0,) Destroying MS object
-Modifying MS object, UL TLLI: 0xffffffff -> 0xf1223344, not yet confirmed
TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW) Assembling frames: (len=20)
TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW) Frame 1 starts at offset 4, length=16, is_complete=1
TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW) complete UL frame len=16
@@ -2597,8 +2596,9 @@
UL_ACK_TBF(UL-TFI_0){NONE}: Received Event SCHED_ACK
UL_ACK_TBF(UL-TFI_0){NONE}: state_chg to SCHED_UL_ACK
New MS: TLLI = 0xf1223344, TA = 7, IMSI = 0011223344, LLC = 2
-MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, UL) Destroying MS object
-MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0,) Detaching TBF: TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FINISHED)
+MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, UL DL) Destroying MS object
+MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0, DL) Detaching TBF: TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FINISHED)
+MS(TLLI=0xf1223344, IMSI=0011223344, TA=7, 1/0,) Detaching TBF: TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=ASSIGN)
=== end test_tbf_dl_flow_and_rach_single_phase ===
=== start test_tbf_dl_reuse ===
PDCH(bts=0,trx=0,ts=7) PDCH state: disabled => enabled
To view, visit change 29845. To unsubscribe, or for help writing mail filters, visit settings.