Bugzilla – Bug 2421
Negative RemainingAmpduDuration for the last MPDU in A-MPDU tag
Last modified: 2017-11-27 16:35:02 UTC
Hi all, I noticed that it may happen the remainingAmpduDuration for the last A-Mpdu fragment (MacLow::ForwardDown()) to be negative. It's probably due to the rounding procedures (CalculateTxDuration()). It can be reproduced by running 802.11n-mimo.cc.
Created attachment 2457 [details] Tx Calculation for the last MPDU fragment
After some digging I found out why the RemainingAmpduDuration in most of the cases is not equal to zero for the last MPDU packet. In particular, inside the for loop (MacLow::ForwardDown()) the tx Time for each packed is calculated according to the CalculateTxDuration() where incFlag = 0. Even though, the tx time for the last mpdu pkt is being calculated based on the remainingSymbols of the total AMPDU due to the conversions from double to int etc, the time returned doesn't correspond to the txTime left. As a result, the remainingAmpduDuration for the last MPDU pkt is an integer, but zero. A solution is to calculate the tx Time for the last MPDU based on the time left. There is one more thing to be considered. The first MPDU pkt is sent when delay=0, triggering the SendPacket(), which will trigger the CalculateTxDuration() with incFlag = 1. As a consequence, the m_totalAmpduNumSymbols in WifiPhy is not zero any more, affecting the tx time for the last MPDU (MacLow::ForwardDown()). By moving out of the for loop the SendPacket() for the first MPDU, the above issue is solved. The above patch solves these issues, however if that is the case, some changes in the MinstrelHt might required (?).
I could not reproduce the issue by running 80211n-mimo example. Did you try with ns-3-dev? It is running fine at my side with ns-3-dev + patches for bugs 2376 and 2379. Could you retry with the same baseline?
Yes it was with ns-3-dev. It's not related with bug 2376, as this one is for missing an A-MPDU frame at the PHY receiver. However, I'm not sure if bug 2379 has something to do with that. In MacLow::ForwardDown (), before entering the for loop and dequeue the MPDU packets, the transmission time for the whole A-MPDU is being calculated ( Time mpduDuration = m_phy->CalculateTxDuration (newPacket->GetSize (), txVector, preamble, m_phy->GetFrequency (), mpdutype, 0)). Now, when delay==0 (1st packet) the m_phy->SendPacket (newPacket, txVector, preamble, mpdutype) will trigger the SendPacket where Time txDuration = CalculateTxDuration (packet->GetSize (), txVector, preamble, GetFrequency (), mpdutype, 1). As this is the first packet and the flag is 1, in wifiPhy the totalAmpduNumSymbols will not be zero but m_totalAmpduNumSymbols += numSymbols. Now back to the MacLow::ForwardDown(), the txTime for the last frame is computed based on numSymbols -= m_totalAmpduNumSymbols, even if the flag is 0 (Note that m_totalAmpduNumSymbols is not zero any more but equal to the size of the first fragment). I know that the title might be misleading, because the remainingAmpduDuration sometimes is negative and some is not, but it should be zero after the last fragment in MacLow:;ForwardDown() instead. If you get a zero remainingAmpduDuration for the last fragment, then I'm missing something.
Can you point me how I can reproduce the issue you suspect? You need to know that the split in MPDUs is not so strict in practice, since it is encapsulated in DMT symbols, which is actually computed in WifiPhy. We had a similar bug that was fixed in the past, but we maybe still have some side effects now he have MIMO included.
*OFDM symbols
You can run 802.11n-mimo with the latest ns-3-dev (no need to run it for the whole duration or all rates, just until the transmission of the first aggregated frame) and print out the remainingAmpduDuration before entering the loop and after the remainingAmpduDuration -= mpduDuration; inside the loop in MacLow::ForwardDown(). You should get something like the output below: HtMcs0 Distance = 0m: Total Duration: 9540000 ns Fragm 1 RemainingNbOfMpdus: 4 mpduDuration: 1939692 RemainingDuration: 7600308 ns Fragm 2 RemainingNbOfMpdus: 3 mpduDuration: 1900307 RemainingDuration: 5700001 ns Fragm 3 RemainingNbOfMpdus: 2 mpduDuration: 1900307 RemainingDuration: 3799694 ns Fragm 4 RemainingNbOfMpdus: 1 mpduDuration: 1900307 RemainingDuration: 1899387 ns Fragm 5 RemainingNbOfMpdus: 0 mpduDuration: 1900307 RemainingDuration: -920 ns assert failed. cond="remainingAmpduDuration==0"
OK, now I understand what you mean. The MPDU duration is not used for the last MPDU, because there is no need to compute a delay for a next MPDU: if (queueSize > 1) { delay = delay + mpduDuration; } For the last MPDU, queueSize is 0. But I agree this is not correctly set in the tag: the remaining duration for the last MPDU should be 0. I will provide a fix.
I changed the title a bit to be in line with the bug.
Created attachment 2462 [details] patch to fix
Even thoughSebastien's patch correctly sets the remaining duration for the last MPDU in the A-MPDU tag, there are some other issues too. The tx Duration for the last MPDU frame calculated in MacLow::ForwardDown() do not match with the tx Duration in YansWifiPhy::SendPacket(). Furthermore, the total tx Duration in ForwardDown and in SendPacket do not match as well. Besides that, the issue with the WifiPhy and the last fragment remains (the one I mentioned in a previous post, numSymbols -= m_totalAmpduNumSymbols, even if the flag is 0)
(In reply to Ioannis from comment #11) > Even thoughSebastien's patch correctly sets the remaining duration for the > last MPDU in the A-MPDU tag, there are some other issues too. The tx > Duration for the last MPDU frame calculated in MacLow::ForwardDown() do not > match with the tx Duration in YansWifiPhy::SendPacket(). As I said, it is not used there, and we know the value of the last MPDU in that call is not ok, but will be never used so it is acceptable. Furthermore, the > total tx Duration in ForwardDown and in SendPacket do not match as well. > Besides that, the issue with the WifiPhy and the last fragment remains (the > one I mentioned in a previous post, numSymbols -= m_totalAmpduNumSymbols, > even if the flag is 0) I do not get your comment here. The total duration here is computed for the complete frame, and will not enter in A-MPDU specific cases in WifiPhy::CalculatePayloadDuration.
(In reply to sebastien.deronne from comment #12) > I do not get your comment here. > The total duration here is computed for the complete frame, and will not > enter in A-MPDU specific cases in WifiPhy::CalculatePayloadDuration. I agree that in MacLow::ForwardDown() the tx duration is calculated for the whole packet and should be the equal to the sum of the tx duration of the MPDU frames computed in YansWifiPhy::SendPacket(), which is not. Of course, their difference is only a few NanoSeconds (rounding numbers, this is the reason that I calculated the tx of the last frame based on the time left and not on the symbols) which I guess is acceptable and won't cause any issues with the schedule of the SendBlockAckAfterAmpdu (which by the way, it seems to me that it can be called even if the first A-MPDU frame is not destined to that node leading to assert failed. cond="i != m_bAckCaches.end ()").
(In reply to Ioannis from comment #13) > (In reply to sebastien.deronne from comment #12) > > I do not get your comment here. > > The total duration here is computed for the complete frame, and will not > > enter in A-MPDU specific cases in WifiPhy::CalculatePayloadDuration. > > I agree that in MacLow::ForwardDown() the tx duration is calculated for the > whole packet and should be the equal to the sum of the tx duration of the > MPDU frames computed in YansWifiPhy::SendPacket(), which is not. Of course, > their difference is only a few NanoSeconds (rounding numbers, this is the > reason that I calculated the tx of the last frame based on the time left and > not on the symbols) which I guess is acceptable and won't cause any issues > with the schedule of the SendBlockAckAfterAmpdu Can you show how to observe a variation in the total duration? Few nanoseconds seems impossible, since it needs to be a given number of symbols. (which by the way, it seems > to me that it can be called even if the first A-MPDU frame is not destined > to that node leading to assert failed. cond="i != m_bAckCaches.end ()"). I will check (in another thread if it pops up to be a bug), I indeed have doubt now about this, thanks.
Ioannis, any updates? Otherwise I suggest to deliver the proposed fix and close this issue. For the other bug you mentioned, I opened another critical thread and I expect to deliver the patch this week still.
(In reply to sebastien.deronne from comment #15) > Ioannis, any updates? > Otherwise I suggest to deliver the proposed fix and close this issue. I printed out the total Tx duration in the ForwardDown() and I did sum up the tx duration of each individual frame (belonging on the same A-MPDU) in SendPacket(). I ran the 802.11n-mimo (i.e. you can try it for HtMcs0 and check the time for the first aggregated frame). I'll try to upload the trace file during the day. > For the other bug you mentioned, I opened another critical thread and I > expect to deliver the patch this week still. I did the same modification and myself.
Created attachment 2468 [details] Last_MPDU_Tx_Duration_log The first part is the log information based on the patch_to_fix file, while the second part based on my implementation. The negative remaining in the first part is because the first MPDU frame triggers the SendPacket before finishing the last frame in ForwardDown(). As a result, the tx duration (in ForwardDown) for the last frame is affected by the numSymbols -= m_totalAmpduNumSymbols; (WifiPhy) where m_totalAmpduNumSymbols = symbolsOfFirstFrame.
I do not agree, the problem here is that you do the sum of several durations, that are rounded to the nanoseconds (default time resolution). Because of the rounding process, you miss here 3 nanoseconds in total. You need to call once Time::SetResolution(Time::FS) in 80211n-mimo to have a resolution up to the femtosecond. In our wifiphy code, we could also increase the time accuracy (I will attach a patch for that).
Created attachment 2469 [details] patch to increase time accuracy in wifiphy
(In reply to sebastien.deronne from comment #19) > Created attachment 2469 [details] > patch to increase time accuracy in wifiphy I agree with the patch to increase time accuracy in wifiphy, but this time we'll get a 3 fs difference instead. Of course, 3 fs is significant small number, but I'm not sure if it increases with the number of A-MPDU frames or the data rate. Out of curiosity, mobility affects the reception time (StartReceivePreambleAndHeader), introducing time gaps between the consecutive frames or even worse resulting in dropping a frame because the reception of the previous one has not been finished yet, right?
(In reply to Ioannis from comment #20) > (In reply to sebastien.deronne from comment #19) > > Created attachment 2469 [details] > > patch to increase time accuracy in wifiphy > > I agree with the patch to increase time accuracy in wifiphy, but > this time we'll get a 3 fs difference instead. Of course, 3 fs is > significant small number, but I'm not sure if it increases with the > number of A-MPDU frames or the data rate. We cannot be more accurate than that, and I think we can live with it, unless we can prove in a given test case that it is causing an issue. I am not against refactoring that would avoid such rounding issues, but we worked for a lot of time to get a working design, so I don't think it will be obvious. Since we do not observe issues with the design, I do not plan to think about any refactoring. > > Out of curiosity, mobility affects the reception time > (StartReceivePreambleAndHeader), introducing time gaps between > the consecutive frames or even worse resulting in dropping a > frame because the reception of the previous one has not been > finished yet, right? I do not really understand your question.
This bug can be closed, other topics are being discussed by mails.
Pushed in changeset 12164:34b8e8073c11
Although, duration is not used for the last Ampdu and is set to zero, there is an issue on the way that duration for AMPDU is calculated. For example, by just adding NS_ASSERT (remainingAmpduDuration.IsPositive()); in MacLow::ForwardDown () and running he-wifi-network.cc, I get queueSize=1 ,remainingAmpduDuration=+0.0ns queueSize=3 ,remainingAmpduDuration=+3379830.0ns queueSize=2 ,remainingAmpduDuration=+1690668.0ns queueSize=1 ,remainingAmpduDuration=-9161.0ns assert failed. cond="remainingAmpduDuration.IsPositive()", file=../src/wifi/model/mac-low.cc, line=1321 terminate called without an active exception 9161 nanoseconds is way too much. The latest dev was used to produce these results.
This is expected at this point in time, because we call CalculateTxDuration with incFlag to 0. Once sent to the PHY, the last aggregate will have its duration reduced by 9161 nanoseconds. The remaining duration should be 0 for the last aggregate.