pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33525 ) Change subject: WIP: Re-store last LLC frames never completely acked when freeing DL TBF ...................................................................... WIP: Re-store last LLC frames never completely acked when freeing DL TBF <pespin> Hmm I need to improve the osmo-pcu case where we assign a DL TBF over PCH and we start transmitting DL data blocks blindly after X2002, but at the same time the MS starts packet-access-procedure to request an UL TBF. Right now the osmo-pcu correctly detects the MS is avaiable in PDCH and re-assigns a DL TBF using PACCH, but the LLC frames it transmitted in the old PCH-assigned DL TBF are lost :/ <pespin> so I need somehow to cache those LLC blocks transmitted but not ACKed, and prepend them to the LLC queue of the MS if the DL TBF is freed <pespin> this issue is now more usual since I added X2002 timer to delay starting requesting USF for a UL TBF, hence the contention resolution in general takes more and hence the PACCH assignmend of the DL TBF too, so more DL data blocks are transmitted to the DL TBF assigned over PC 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, 139 insertions(+), 21 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/25/33525/1 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..4ecb335 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 + * trasnmission, 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..f75b73d 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 lossing 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: 1 Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: newchange