2 Jul
2023
2 Jul
'23
4:16 p.m.
pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33525 )
Change subject: Reestore last LLC frames never completely acked when freeing DL TBF
......................................................................
Reestore last LLC frames never completely acked when freeing DL TBF
Scenario: A DL TBF is assigned over PCH (CCCH) and we start transmitting
DL data blocks blindly after X2002, but at the same time the MS start
packet-access-procedure to request an UL TBF.
Right now osmo-pcu correctly detects the MS is available in PDCH and
re-assigns a DL TBF using PACCH, but the LLC frames it transmitted in
the old PCH-assigned DL TBF get lost when that older TBF is freed
(because the DL blocks were removed from the GprsMs llc_queue).
This issue is now more frequent since X2002 timer was added recently to
delay starting requesting USF for a UL TBF, hence the contention
resolution in general takes more time and hence the PACCH assignment of
the DL TBF takes more time too, so more DL data blocks are transmitted
to the DL TBF assigned over PCH during that time.
This patch improves the situation to at least recover the DL blocks
transmitted if the DL TBF is freed (due to MS merge trigger by scenario
mentioned above), where no DL ACK/NACK was ever received by the MS.
Ideal solution would be to have complete tracking of which LLC PDUs from
the llc_queues were completely ACKed at RLC/MAC level, but that really
requires a lot of work and major refactoring, which are left as a future
improvement.
Change-Id: I9be4035fb2cf2b3ee56e91dcc12cc8c24028b4aa
---
M src/gprs_ms.c
M src/llc.c
M src/llc.h
M src/tbf_dl.cpp
M src/tbf_dl.h
M tests/llc/LlcTest.cpp
M tests/tbf/TbfTest.err
7 files changed, 158 insertions(+), 21 deletions(-)
Approvals:
osmith: Looks good to me, but someone else must approve
Jenkins Builder: Verified
laforge: Looks good to me, but someone else must approve
fixeria: Looks good to me, approved
diff --git a/src/gprs_ms.c b/src/gprs_ms.c
index 06d012c..6e6390b 100644
--- a/src/gprs_ms.c
+++ b/src/gprs_ms.c
@@ -456,6 +456,8 @@
void ms_merge_and_clear_ms(struct GprsMs *ms, struct GprsMs *old_ms)
{
char old_ms_name[128];
+ struct gprs_rlcmac_dl_tbf *dl_tbf;
+
OSMO_ASSERT(old_ms != ms);
ms_ref(old_ms, __func__);
@@ -472,6 +474,12 @@
if (!ms_egprs_ms_class(ms) && ms_egprs_ms_class(old_ms))
ms_set_egprs_ms_class(ms, ms_egprs_ms_class(old_ms));
+
+ if ((dl_tbf = ms_dl_tbf(old_ms))) {
+ /* Move the last partially/totally unacked LLC PDU back to the LLC queue: */
+ dl_tbf_copy_unacked_pdus_to_llc_queue(dl_tbf);
+ }
+ /* Now merge the old_ms queue into the new one: */
llc_queue_move_and_merge(&ms->llc_queue, &old_ms->llc_queue);
/* Clean up the old MS object */
diff --git a/src/llc.c b/src/llc.c
index 7745bd0..45b8ac4 100644
--- a/src/llc.c
+++ b/src/llc.c
@@ -35,6 +35,8 @@
{
llc->index = 0;
llc->length = 0;
+ llc->prio = 0;
+ llc->meta_info = (struct MetaInfo){0};
memset(llc->frame, 0x42, sizeof(llc->frame));
}
@@ -231,6 +233,26 @@
q->queue_octets = queue_octets;
}
+/* Prepend / Put back a previously dequeued LLC frame (llc_queue_dequeue()) */
+void llc_queue_merge_prepend(struct gprs_llc_queue *q, struct gprs_llc *llc)
+{
+ struct MetaInfo *meta_storage;
+ unsigned int len = llc_frame_length(llc);
+ struct msgb *llc_msg = msgb_alloc(len, "llc_pdu_queue");
+
+ OSMO_ASSERT(llc_msg);
+ memcpy(msgb_put(llc_msg, len), llc->frame, len);
+
+ q->queue_size += 1;
+ q->queue_octets += msgb_length(llc_msg);
+
+ meta_storage = (struct MetaInfo *)&llc_msg->cb[0];
+ memcpy(meta_storage, &llc->meta_info, sizeof(struct MetaInfo));
+
+ /* Prepend: */
+ llist_add(&llc_msg->list, &q->pq[llc->prio].queue);
+}
+
#define ALPHA 0.5f
static struct msgb *llc_queue_pick_msg(struct gprs_llc_queue *q, enum gprs_llc_queue_prio
*prio)
@@ -265,7 +287,7 @@
return msg;
}
-struct msgb *llc_queue_dequeue(struct gprs_llc_queue *q)
+struct msgb *llc_queue_dequeue(struct gprs_llc_queue *q, enum gprs_llc_queue_prio
*out_prio, struct MetaInfo *out_info)
{
struct msgb *msg;
struct timespec tv_now, tv_now2;
@@ -274,6 +296,7 @@
struct gprs_pcu *pcu = bts->pcu;
struct timespec hyst_delta = {0, 0};
enum gprs_llc_queue_prio prio;
+ const struct MetaInfo *info = NULL;
if (pcu->vty.llc_discard_csec)
csecs_to_timespec(pcu->vty.llc_discard_csec, &hyst_delta);
@@ -282,7 +305,7 @@
timespecadd(&tv_now, &hyst_delta, &tv_now2);
while ((msg = llc_queue_pick_msg(q, &prio))) {
- const struct MetaInfo *info = (const struct MetaInfo *)&msg->cb[0];
+ info = (const struct MetaInfo *)&msg->cb[0];
const struct timespec *tv_disc = &info->expire_time;
const struct timespec *tv_recv = &info->recv_time;
@@ -335,6 +358,17 @@
bssgp_tx_llc_discarded(pcu->bssgp.bctx, ms_tlli(q->ms), frames, octets);
}
+ if (!msg)
+ return NULL;
+
+ if (out_prio)
+ *out_prio = prio;
+
+ if (out_info) {
+ OSMO_ASSERT(info);
+ *out_info = *info;
+ }
+
return msg;
}
diff --git a/src/llc.h b/src/llc.h
index d12daac..594f77f 100644
--- a/src/llc.h
+++ b/src/llc.h
@@ -50,6 +50,17 @@
uint8_t control[0];
} __attribute__ ((packed));
+struct MetaInfo {
+ struct timespec recv_time;
+ struct timespec expire_time;
+};
+enum gprs_llc_queue_prio { /* lowest value has highest prio */
+ LLC_QUEUE_PRIO_GMM = 0, /* SAPI 1 */
+ LLC_QUEUE_PRIO_TOM_SMS, /* SAPI 2,7,8 */
+ LLC_QUEUE_PRIO_OTHER, /* Other SAPIs */
+ _LLC_QUEUE_PRIO_SIZE /* used to calculate size of enum */
+};
+
/**
* I represent the LLC data to a MS
*/
@@ -57,6 +68,10 @@
uint8_t frame[LLC_MAX_LEN]; /* current DL or UL frame */
uint16_t index; /* current write/read position of frame */
uint16_t length; /* len of current DL LLC_frame, 0 == no frame */
+
+ /* Saved when dequeue from llc_queue; allows re-enqueing in the queue if Tx fails */
+ enum gprs_llc_queue_prio prio;
+ struct MetaInfo meta_info;
};
void llc_init(struct gprs_llc *llc);
@@ -99,19 +114,9 @@
return llc->length + chunk_size <= LLC_MAX_LEN;
}
-struct MetaInfo {
- struct timespec recv_time;
- struct timespec expire_time;
-};
/**
* I store the LLC frames that come from the SGSN.
*/
-enum gprs_llc_queue_prio { /* lowest value has highest prio */
- LLC_QUEUE_PRIO_GMM = 0, /* SAPI 1 */
- LLC_QUEUE_PRIO_TOM_SMS, /* SAPI 2,7,8 */
- LLC_QUEUE_PRIO_OTHER, /* Other SAPIs */
- _LLC_QUEUE_PRIO_SIZE /* used to calculate size of enum */
-};
struct gprs_llc_prio_queue {
struct gprs_codel codel_state;
struct llist_head queue; /* queued LLC DL data. See enum gprs_llc_queue_prio. */
@@ -133,8 +138,9 @@
void llc_queue_clear(struct gprs_llc_queue *q, struct gprs_rlcmac_bts *bts);
void llc_queue_set_codel_interval(struct gprs_llc_queue *q, unsigned int interval);
void llc_queue_move_and_merge(struct gprs_llc_queue *q, struct gprs_llc_queue *o);
+void llc_queue_merge_prepend(struct gprs_llc_queue *q, struct gprs_llc *llc);
void llc_queue_enqueue(struct gprs_llc_queue *q, struct msgb *llc_msg, const struct
timespec *expire_time);
-struct msgb *llc_queue_dequeue(struct gprs_llc_queue *q);
+struct msgb *llc_queue_dequeue(struct gprs_llc_queue *q, enum gprs_llc_queue_prio
*out_prio, struct MetaInfo *out_info);
static inline size_t llc_queue_size(const struct gprs_llc_queue *q)
{
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index f4439d3..dc35b84 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -173,6 +173,8 @@
state_fi = osmo_fsm_inst_alloc(&tbf_dl_fsm, this, &state_fsm, LOGL_INFO, NULL);
OSMO_ASSERT(state_fi);
+ INIT_LLIST_HEAD(&this->tx_llc_until_first_dl_ack_rcvd);
+
/* This has to be called in child constructor because enable_egprs()
* uses the window() virtual function which is dependent on subclass. */
if (ms_mode(m_ms) != GPRS)
@@ -542,7 +544,7 @@
return;
/* dequeue next LLC frame, if any */
- msg = llc_queue_dequeue(llc_queue());
+ msg = llc_queue_dequeue(llc_queue(), &m_llc.prio, &m_llc.meta_info);
if (!msg)
return;
@@ -661,6 +663,15 @@
LOGPTBFDL(this, LOGL_DEBUG, "Complete DL frame, len=%d\n",
llc_frame_length(&m_llc));
gprs_rlcmac_dl_bw(this, llc_frame_length(&m_llc));
bts_do_rate_ctr_add(bts, CTR_LLC_DL_BYTES, llc_frame_length(&m_llc));
+
+ /* Keep transmitted LLC PDUs until first ACK to avoid lossing them if MS is not there.
*/
+ if (!this->m_first_dl_ack_rcvd) {
+ struct gprs_dl_llc_llist_item *llc_it = talloc(this, struct gprs_dl_llc_llist_item);
+ memcpy(&llc_it->llc, &m_llc, sizeof(llc_it->llc));
+ /* Prepend to list to store them in inverse order of transmission, see
+ * dl_tbf_copy_unacked_pdus_to_llc_queue() for the complete picture. */
+ llist_add(&llc_it->list, &this->tx_llc_until_first_dl_ack_rcvd);
+ }
llc_reset(&m_llc);
if (is_final) {
@@ -1073,7 +1084,15 @@
int rc;
LOGPTBFDL(this, LOGL_DEBUG, "downlink acknowledge\n");
- m_first_dl_ack_rcvd = true;
+ if (m_first_dl_ack_rcvd == false) {
+ /* MS is there, free temporarily stored transmitted LLC PDUs */
+ struct gprs_dl_llc_llist_item *llc_it;
+ while ((llc_it =
llist_first_entry_or_null(&this->tx_llc_until_first_dl_ack_rcvd, struct
gprs_dl_llc_llist_item, list))) {
+ llist_del(&llc_it->list);
+ talloc_free(llc_it);
+ }
+ m_first_dl_ack_rcvd = true;
+ }
m_last_dl_poll_ack_lost = false;
/* reset N3105 */
@@ -1112,6 +1131,33 @@
return tbf->m_first_dl_ack_rcvd;
}
+/* Copy back to GprsMs' llc_queue the LLC PDUs previously dequeued and never
+ * fully ACKED at the MS side.
+ * FIXME: For now, only blocks transmitted and without first ever DL ACK are
+ * copied, because we have no way yet to track LLC PDUs once they are converted
+ * to RLC blocks. This is however enough to cover the case where a DL TBF is
+ * assigned over PCH and the MS never answers.
+ */
+void dl_tbf_copy_unacked_pdus_to_llc_queue(struct gprs_rlcmac_dl_tbf *tbf)
+{
+ struct GprsMs *ms = tbf_ms(dl_tbf_as_tbf(tbf));
+ struct gprs_dl_llc_llist_item *llc_it;
+
+ /* If we have LLC PDU still being transmitted, prepend it first to the queue: */
+ if (llc_frame_length(&tbf->m_llc) > 0)
+ llc_queue_merge_prepend(&ms->llc_queue, &tbf->m_llc);
+
+ /* Iterate over the list of totally transmitted LLC PDUs and merge them
+ * into the queue. The items in the list are in inverse order of
+ * transmission, hence when popping from here and enqueueing (prepending)
+ * back to the llc_queue it ends up in the exact same initial order. */
+ while ((llc_it = llist_first_entry_or_null(&tbf->tx_llc_until_first_dl_ack_rcvd,
struct gprs_dl_llc_llist_item, list))) {
+ llist_del(&llc_it->list);
+ llc_queue_merge_prepend(&ms->llc_queue, &llc_it->llc);
+ talloc_free(llc_it);
+ }
+}
+
/* Does this DL TBF require to poll the MS for DL ACK/NACK? */
bool gprs_rlcmac_dl_tbf::need_poll_for_dl_ack_nack() const
{
diff --git a/src/tbf_dl.h b/src/tbf_dl.h
index dabe9a9..7223fa0 100644
--- a/src/tbf_dl.h
+++ b/src/tbf_dl.h
@@ -42,6 +42,11 @@
DL_PRIO_CONTROL, /* a control block needs to be sent */
};
+struct gprs_dl_llc_llist_item {
+ struct llist_head list; /* this item added in dl_tbf->tx_llc_until_first_dl_ack_rcvd
*/
+ struct gprs_llc llc;
+};
+
struct gprs_rlcmac_dl_tbf : public gprs_rlcmac_tbf {
gprs_rlcmac_dl_tbf(struct gprs_rlcmac_bts *bts, GprsMs *ms);
~gprs_rlcmac_dl_tbf();
@@ -78,6 +83,11 @@
/* Whether we receive at least one PKT DL ACK/NACK from MS since this DL TBF was
assigned: */
bool m_first_dl_ack_rcvd;
+ /* Keep transmitted LLC PDUs until first ACK to avoid losing them if MS is not there.
+ * list of gprs_dl_llc_llist_item, stored in inverse order of transmission (last
transmitted
+ * is first in the list ) */
+ struct llist_head tx_llc_until_first_dl_ack_rcvd;
+
struct BandWidth {
struct timespec dl_bw_tv; /* timestamp for dl bw calculation */
uint32_t dl_bw_octets; /* number of octets since bw_tv */
@@ -161,6 +171,8 @@
bool dl_tbf_first_dl_ack_rcvd(const struct gprs_rlcmac_dl_tbf *tbf);
int dl_tbf_upgrade_to_multislot(struct gprs_rlcmac_dl_tbf *tbf);
+void dl_tbf_copy_unacked_pdus_to_llc_queue(struct gprs_rlcmac_dl_tbf *tbf);
+
static inline struct gprs_rlcmac_tbf *dl_tbf_as_tbf(struct gprs_rlcmac_dl_tbf *dl_tbf)
{
return (struct gprs_rlcmac_tbf *)dl_tbf;
diff --git a/tests/llc/LlcTest.cpp b/tests/llc/LlcTest.cpp
index 554c749..f5bc54a 100644
--- a/tests/llc/LlcTest.cpp
+++ b/tests/llc/LlcTest.cpp
@@ -75,11 +75,10 @@
size_t len, const MetaInfo *exp_info)
{
struct msgb *llc_msg;
- const MetaInfo *info_res;
+ MetaInfo info_res;
- llc_msg = llc_queue_dequeue(queue);
+ llc_msg = llc_queue_dequeue(queue, NULL, &info_res);
OSMO_ASSERT(llc_msg != NULL);
- info_res = (struct MetaInfo *)&llc_msg->cb[0];
fprintf(stderr, "dequeued msg, length %u (expected %zu), data %s\n",
msgb_length(llc_msg), len, msgb_hexdump(llc_msg));
@@ -88,8 +87,8 @@
fprintf(stderr, "check failed!\n");
if (exp_info) {
- OSMO_ASSERT(memcmp(&exp_info->recv_time, &info_res->recv_time,
sizeof(info_res->recv_time)) == 0);
- OSMO_ASSERT(memcmp(&exp_info->expire_time, &info_res->expire_time,
sizeof(info_res->expire_time)) == 0);
+ OSMO_ASSERT(memcmp(&exp_info->recv_time, &info_res.recv_time,
sizeof(info_res.recv_time)) == 0);
+ OSMO_ASSERT(memcmp(&exp_info->expire_time, &info_res.expire_time,
sizeof(info_res.expire_time)) == 0);
}
msgb_free(llc_msg);
}
diff --git a/tests/tbf/TbfTest.err b/tests/tbf/TbfTest.err
index e582807..02cefe2 100644
--- a/tests/tbf/TbfTest.err
+++ b/tests/tbf/TbfTest.err
@@ -2852,7 +2852,7 @@
TBF(DL:TFI-0-0-1:G:IMSI-0011223344:TLLI-0xf5667788){NEW} set ass. type PACCH [prev
CCCH:0, PACCH:0]
DL_TBF(DL:TFI-0-0-1:G:IMSI-0011223344:TLLI-0xf5667788){NEW}: state_chg to ASSIGN
TBF(DL:TFI-0-0-1:G:IMSI-0011223344:TLLI-0xf5667788){ASSIGN} Starting timer X2001
[assignment (PACCH)] with 2 sec. 0 microsec
-New MS: TLLI = 0xf5667788, TA = 7, IMSI = 0011223344, LLC = 1
+New MS: TLLI = 0xf5667788, TA = 7, IMSI = 0011223344, LLC = 2
MS(IMSI-0011223344:TLLI-0xf5667788:TA-7:MSCLS-1-0:UL:DL) Destroying MS object
MS(IMSI-0011223344:TLLI-0xf5667788:TA-7:MSCLS-1-0:UL:DL) Detaching TBF:
TBF(UL:TFI-0-0-1:G:IMSI-0011223344:TLLI-0xf5667788){FLOW}
MS(IMSI-0011223344:TLLI-0xf5667788:TA-7:MSCLS-1-0:DL): - tbf: now used by 1 (tbf)
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33525
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I9be4035fb2cf2b3ee56e91dcc12cc8c24028b4aa
Gerrit-Change-Number: 33525
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged