Bugzilla – Bug 2731
LTE RLC AM entity paused due to fail to retransmit missing packets
Last modified: 2017-07-14 04:04:12 UTC
We found a bug in the LteRlcAm class: When an outage triggered the ExpirePollRetransmitTimer method, the RLC AM entity does not retransmit the missing packets. As a results, the RLC AM entity is paused and the throughput goes to zero. We went through the LteRlcAm class, and find out the problem was from the state variables m_vtA and m_vtS, representing last Acked sequence and last sent sequence, respectively. According to 3GPP, since all the state variables are cycling from 0 to 1023, when comparing state variables, they should subtract the m_vtA/m_vtR and modulo 1024, as mentioned in [1]. This procedure is correctly performed when receiving Ack/Nack messages, but not in the ExpirePollRetransmitTimer method. Consequently, if the m_vtS is "overflowed", m_vtS become less than m_vtA and no packets are retransmitted. These ignored packets will cause the RLC receiver stop forwarding packets to PDCP. For example, if the current m_vtA = 1000, and m_vtS = 1100%1024=86, the following for loop would not retransmit the missing packets, in this case packets within [m_vtA,1023] and [0,m_vtS]. for ( sn = m_vtA.GetValue(); sn < m_vtS.GetValue (); sn++ ) We have been able to fix this issue with the following change, before the for loop, m_vtS is compared with m_vtA, if m_vtS is smaller than m_vtA, the transmitter should aware m_vtS is "overflowed", and retransmit the packets within [m_vtA,1023] and [0,m_vtS]. [1].AMD PDUs and UMD PDUs are numbered integer sequence numbers (SN) cycling through the field: 0 to 1023 for 10 bit SN and 0 to 65535 for 16 bit SN for AMD PDU and 0 to [2^[sn-FieldLength] – 1] for UMD PDU. When performing arithmetic comparisons of state variables or SN values, a modulus base shall be used. VT(A) and VR(R) shall be assumed as the modulus base at the transmitting side and receiving side of an AM RLC entity, respectively. This modulus base is subtracted from all the values involved, and then an absolute comparison is performed (e.g. VR(R) <= SN < VR(MR) is evaluated as [VR(R) – VR(R)] modulo 1024 <= [SN – VR(R)] modulo 1024 < [VR(MR) – VR(R)] modulo 1024).
Hi Menglei, thanks you very much for reporting the issue and explaining it in detail. It would be very useful if you could upload the patch file containing the suggested change. Thanks, Biljana
Created attachment 2842 [details] a patch file for the bug fix
I reviewed the code provided by Menglei Zhang and it seems fine, all tests pass. I have formatted it to be according to ns-3 coding style. It is included in ns-3-dev as changeset: 12905 39839e91252e. Menglei thank you very much for reporting the issue and for the patch.
I noticed the patch only once it was pushed to ns-3-dev, but I don't like how it was fixed. Isn't replacing uint16_t type of sn with SequenceNumber10 enough? SequenceNumber10 has this comparison logic built-in, numbers are already compared relative to m_vtA, because base of comparison is set to m_vtA for m_vtS: m_vtS.SetModulusBase (m_vtA); The fix is correct, but copying almost the same code 3 times and using 1024 instead of 2 * m_windowSize does not look good. Wouldn't just replacting the loop with for (SequenceNumber10 sn = m_vtA; sn < m_vtS; sn++) and sn with sn.GetValue () work?
Hi Alexander, thank you for reviewing this. Can you please attach the patch that you suggest, just to have a look, then if all ok, I would revert the changeset, and push the modified patch.
Created attachment 2864 [details] Patch to simplify fix Here is a patch that reverts current patch and applies simpler patch at the same time. I replaced the loop, added snValue variable and replaced "sn" with "snValue" inside the loop.
Hi Alexander, thank you very much for your effort in revising this patch. I am not expert on RLC, but I think that what was original idea from Menglei was to cover the case when (acked > sent). Now, I tried now to analyse your patch, and I do not see that this case is covered. Because, the loop will only happen when m_Vta < m_Vts: "for (SequenceNumber10 sn = m_vtA; sn < m_vtS; sn++)". Please explain me if I am wrong. Thanks, Biljana
Hi Menglei, I analysed now the patch and I have one question. Why in the first loop is until 1024? Should this be some configurable parameter of RLC? "for ( sn = m_vtA.GetValue(); sn < 1024; sn++ )" Thanks, Biljana
Hi Mengelei, I understood now why is 1024. I did not know how SequenceNumber10 is implemented. Thanks, Biljana
(In reply to Biljana Bojović from comment #7) > Hi Alexander, > > thank you very much for your effort in revising this patch. I am not expert > on RLC, but I think that what was original idea from Menglei was to cover > the case when (acked > sent). > > Now, I tried now to analyse your patch, and I do not see that this case is > covered. Because, the loop will only happen when m_Vta < m_Vts: > > "for (SequenceNumber10 sn = m_vtA; sn < m_vtS; sn++)". > > Please explain me if I am wrong. > > Thanks, > Biljana SequenceNumber10 takes care of wrapped arithmetic in operator> (which is used by operator<) and operator++. VT_A is always < than VT_S in terms of SequenceNumber10 < implementation, because VT_A is the modulus base for VT_S calculations according to this line: m_vtS.SetModulusBase (m_vtA);
Ok, I understand now, thank you for explanation. Please fill free to commit your patch whenever is convenient for you.
Hi Alexander, sorry, if you didn't start to commit, please hold on, I need to check one more thing. Thanks, Biljana
Alexander, I performed all necessary checks. It works perfectly. Please feel free to push your patch. And thanks again. Biljana
Pushed as changeset 50da2e62e513