Bug #5508
closedLLC does not prioritize based on SAPI
100%
Description
Right now, the PCU treats all LLC frames as equal. It doesn't parse them so it is oblivious to the SAPI or any other LLC or higher layer aspects.
This can lead to situations where GMM/SM signalling gets delayed a lot due to user plane traffic queues growing long.
I couldn't find a clear spec reference for this, but it makes a lot of sense to prioritize. SAPI=1 is GMM/SM, and it should always have higher priority that anything else. As the number of GMM/SM frames is low, this should be without any risks.
Updated by laforge about 2 years ago
Looking at the osmo-pcu code, this likely means that the llc.cpp gprs_llc_queue class needs to be modified, specifically in the gprs_llc_queue::enqueue() method, where the LLC msgb is currently not interpreted.
The lower 4 bits of the first octet are the SAPI.
It may also make sense to treat TOM2 (2) and LLLSMS (7) at higher priority.
So at the minimum, SAPI=1 should precede anything else (simply enqueue at the head instead of tail?).
A more sophisticated implementation would probably have three queues:- SAPI=1 at highest priority
- SAPI=2/7/8 as medium priority
- any other SAPI as lowest priority.
Updated by lynxis about 2 years ago
So at the minimum, SAPI=1 should precede anything else (simply enqueue at the head instead of tail?).
We should use separate queues. In case of an SGSN restart: the SGSN would send a XID RESET followed by a Detach Request. The order must be preserved.
Updated by laforge about 2 years ago
lynxis wrote in #note-2:
We should use separate queues. In case of an SGSN restart: the SGSN would send a XID RESET followed by a Detach Request. The order must be preserved.
Thanks, very valid point (re-ordering). So indeed it would have to be separate queues, and if we do two queues, we can just as well do 3 at that point.
Updated by pespin about 2 years ago
- Assignee set to pespin
- Spec Reference set to 3GPP TS 44.064 (LLC)
Updated by pespin about 2 years ago
- Status changed from New to In Progress
- % Done changed from 0 to 50
I already have a TTCN3 reproducing the LLC SAPI prio issue with current osmo-pcu master:
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/27632 RLCMAC_EncDec: Fix bug decoding LI0=0 [NEW]
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/27633 pcu: Introduce test TC_dl_llc_sapi_priority [NEW]
I have a WIP patch to implement the LLC SAPI prio which is altready making the test pass:
https://gerrit.osmocom.org/c/osmo-pcu/+/27627/1
I reviewed the code to see if the statistics had to be updated when moving to multiple queue, but they don't seem to be affected (leak rate, etc. can be kept calculated globally since they are sent globally anyway).
Regarding codel, I think it makes sense to have a codel state per prio queue. Otherwise the sojourn time state is not properly reset when a high prio packet is dequeued.
If we have a global codel state shared for all prio queues of an MS, then basically high prio (GMM) packets will "never" be dropped because they are handled/dequeued way quicker, so it's sojourn time will be below the threshold most probably, stopping the "dropping" state for the rest of lower prio packets.
Hence, we probably need to move the current ms->codel_state to be per prio_queue, that is, move codel_state inside struct gprs_llc_queue:
struct gprs_llc_queue {
...
struct llist_head queue[_LLC_QUEUE_PRIO_SIZE];
struct gprs_codel codel_state[_LLC_QUEUE_PRIO_SIZE];
}
Then, in gprs_rlcmac_dl_tbf::llc_dequeue, update llc_queue_dequeue() API to handle codel internally and report back the amount of frames+packets dropped (or do the bssgp_tx_llc_discarded internally, that's also fine).
Updated by pespin about 2 years ago
- Status changed from In Progress to Feedback
- % Done changed from 50 to 90
Commit changing to per prio-queue CoDel state has been submitted here:
https://gerrit.osmocom.org/c/osmo-pcu/+/27641 llc_queue: Refactor to handle codel_state per prio queue internally
Updated by pespin about 2 years ago
- Status changed from Feedback to Resolved
- % Done changed from 90 to 100
Merged a long time ago, closing.
