Bugzilla – Bug 2224
scope of GetAmpduExist() in EdcaTxopN
Last modified: 2016-02-22 05:06:24 UTC
there is a single flag for m_ampduExist but block ack state exists for multiple recipients when an AP is involved. The problem is summarized in this post (November 13): https://groups.google.com/forum/#!topicsearchin/ns-3-users/Current$20packet$20is$20not$20Qos$20Data/ns-3-users/XoAYLcIpChE this can result in NS_FATAL_ERROR() being called in EdcaTxopN::MissedAck: "Current packet is not Qos Data" when block ack agreement exists for one station but not another, as described in the post. A workaround is in the following patch. However, should m_ampduExist be extended to be a map of flags keyed on recipient address? Scenarios where this occurs tend to be complicated since it typically happens when short retry loss occurs, which may be many packets into a normal simulation. A test case may be to set up an AP, and get a block ack agreement established with one station, but upon the second station's agreement, force the short retry loss of the add block ack agreement so that GetAmpduExist() is already true. What to do in this case should be defined, and also, what to do when ADDBA fails should be handled (I think it may not be protected against this loss).
Created attachment 2190 [details] temporary workaround to disable NS_FATAL_ERROR condition
setting to NEEDINFO to get a good testcase
Using a map iso a flag sounds indeed better. Tom, did you already check this solution?
(In reply to sebastien.deronne from comment #3) > Using a map iso a flag sounds indeed better. > Tom, did you already check this solution? No, I just use the workaround patch in ns-3-lbt repo that I am using. I would like to provide a good testcase for this in ns-3-dev first; I think we ought to have a breaking testcase before evaluating possible better patches.
I agree. I had started to include tests for A-MPDU; I should continue on that and include a case for this.
Created attachment 2215 [details] testcase that crashes when the bug is not fixed Here is a patch to add some unit tests for A-MPDU. I also added a testcase to reproduce this bug. A-MPDU unit tests still needs further extension, but I miss time to add more for the upcoming release, so I propose to extend those later.
Created attachment 2216 [details] use a map iso a flag to indicate that MPDU aggregation is set for a given destination address I implemented the map solution to replace the existing flag, which is set per destination address.
Any updates on this bug so far?
(In reply to sebastien.deronne from comment #8) > Any updates on this bug so far? I have looked it over and think it will fix the issue and suggest to commit it now. I suggest one minor change; there is an unused typedef of AmpduEnabled which could be safely deleted.
(In reply to Tom Henderson from comment #9) > (In reply to sebastien.deronne from comment #8) > > Any updates on this bug so far? > > > I have looked it over and think it will fix the issue and suggest to commit > it now. > > I suggest one minor change; there is an unused typedef of AmpduEnabled which > could be safely deleted. Tom, I do not see the unused typedef, the one defined in EdcaTxopN is actually used for defining the map m_aMpduEnabled.
> > Tom, I do not see the unused typedef, the one defined in EdcaTxopN is > actually used for defining the map m_aMpduEnabled. What I meant by this was that the typedef is only used to declare the variable on the next line; it is not used anywhere else in the code.
fixed in changeset 11892:02ada4d0519e