Bugzilla – Bug 2318
MPDU Aggregation fails with TCP
Last modified: 2016-03-09 15:57:05 UTC
Created attachment 2315 [details] Script File which generates the error I am doing some tests on TCP over 802.11n with MPDU aggregation enabled using the Wifi Module in the ns3-dev branch. However, I am getting the following error: assert failed. cond="tid < 8", msg="Tid 8 out of range", file=../src/wifi/model/qos-utils.cc, line=30 I applied the patches for bug 2315 and 2313 on ns3-dev and I am still getting the same error. The error just appears at a later time in the simulation. Please find the attached script for which I see the problem with its command line arguments: ./waf --run "test-ht --applicationType=bulk --socketType=ns3::TcpSocketFactory --payloadSize=1440 --simulationTime=100"
I did the attached patch which seems to solve the problem. The patch includes patches for bug 2315 and bug 2313. In addition, it solves the cases where the previous error appears in the FowardDown and AggregateToAmpdu Functions in mac-low.cc file.
Created attachment 2316 [details] patch file to solve tcp problem with ampdu
Hany, could you please refine a bit the general description of the bug? Thanks!
Sebastien, in the scenario I attached I use TCP instead of UDP with bulk application. After running the simulation using 802.11n with HtMcs7 for data and HtMcs0 for control, I get an error related to TID value being not in the correct range. I did multiple debug printing across mac-low.cc and I found that the problem happens at two places: 1. In the ForwardDown function, whenever you transmit BlockAckResponse, the a_mpdu flag is always set to 0. As a result, the YansWifiPhy transmit it directly it in the current implementation. However, there is one case in which the a_mpdu flag is set to 1 and we want to transmit BlockAckResponse, so in the ForwardDown function we start to send BlockAckResponse as part of aggregated MPDU which is not correct i.e. we do not aggregate BlockAckResponse with other MPDUs. This case cause the problem related to TID value being not in the correct range. So this is why I added the third check: if (!m_ampdu || hdr->IsRts () || hdr->IsBlockAck ()) I am not sure why the a_mpdu flag is set to 1 in this case and not updated to zero. 2. After solving the previous problem another bug appeared in AggregatedToAmpdu Function in mac-low.cc. In this bug, we send an BlockAckRequest frame and we try to aggregate it with the other MPDUs, however there are no packets in the queue related to AC_BE so the function StopMpduAggregation tries to check the header of peekedPacket which is 0. So this is why I added another check before entering the body of the AggregatedToAmpdu function as following: if (!hdr.GetAddr1 ().IsBroadcast () && (listenerIt->second->GetMpduAggregator () != 0) && (queue->GetNPacketsByTidAndAddress (tid, WifiMacHeader::ADDR1, hdr.GetAddr1 ()) != 0)) After adding the additional checks together with the previously mentioned patches, the scenario worked for me for long simulation time of 100 seconds with different seeds. If you need more information, please don't hesitate to ask me.
Hany, I mean the title is not ok, every bug needs to be more explicit
Created attachment 2319 [details] patch to fix I see indeed two issues when using TCP: 1/ As you did in your patch, m_phy->SendPacket (packet, txVector, preamble) should always be called for transmitting a Block ACK, and the latter should never be handled as an A-MPDU (here it is due to the previous A-MPDU transmission that is not terminated). 2/ In MacLow::StopMpduAggregation, we first check the TID of the peekedPacket. This is incorrect when there is no peeked packet. In that case, we should return immediately that A-MPDU aggregation can be stopped (no more packets). The attached patch addresses those two issues.
If patch confirmed ok, I will deliver in ns-3-dev by the end of the day.
Sebastien, I just tested your final patch and it is OK. In the second case, it is correct to check for the peeked packet at the beginning of MacLow::StopMpduAggregation function instead of what I proposed to check the queue size at the beginning of MacLow::AggregateToAmpdu function. I found some cases in my previous patch in which the module does not behave correctly. However, for the first case I was not able to find why a_mpdu was set to true and we are just sending BlockAckResponse. Was it set due to a previous transmission?
(In reply to Hany from comment #8) > Sebastien, I just tested your final patch and it is OK. In the second case, > it is correct to check for the peeked packet at the beginning of > MacLow::StopMpduAggregation function instead of what I proposed to check the > queue size at the beginning of MacLow::AggregateToAmpdu function. I found > some cases in my previous patch in which the module does not behave > correctly. However, for the first case I was not able to find why a_mpdu was > set to true and we are just sending BlockAckResponse. Was it set due to a > previous transmission? This was set to true because your station started to transmit an A-MPDU, but in the time waiting for its block ack it received a block ack request, so the A-MPDU exchange was not terminated (it is terminated once the block ack is either received or missed)
We should also consider to add this test program to examples/wireless and instrument it for some regression testing. Hany, could we put a GPL license on it and adapt it to become a wireless example?
Created attachment 2320 [details] Example file for TCP over Wifi Hi Tom, I modified the script and adapted it to be more like wireless example. Additionally, I added the GPL license to it.
Tom, do you prefer we add a check on the expected output?
Created attachment 2321 [details] Example of TCP over 802.11n with A-MPDU enabled Slightly reworked example. Tom, I suggest to make a check on the average throughput with default parameters and make regression fails if we are out of range.
fix pushed in changeset 11994:710403681408, example pushed in changeset 11995:2d105131a6cb