pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/29845 )
Change subject: Avoid loosing DL tbf from old_ms when rx premier UL data ......................................................................
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
-
pespin