Bug 2224

Summary:	scope of GetAmpduExist() in EdcaTxopN
Product:	ns-3	Reporter:	Tom Henderson <tomh>
Component:	wifi	Assignee:	sebastien.deronne
Status:	RESOLVED FIXED
Severity:	normal	CC:	ns-bugs
Priority:	P5
Version:	pre-release
Hardware:	PC
OS:	Linux
Attachments:	temporary workaround to disable NS_FATAL_ERROR condition testcase that crashes when the bug is not fixed use a map iso a flag to indicate that MPDU aggregation is set for a given destination address

Description Tom Henderson 2015-11-20 20:13:58 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).

Comment 1 Tom Henderson 2015-11-20 20:52:46 UTC

Created attachment 2190 [details]
temporary workaround to disable NS_FATAL_ERROR condition

Comment 2 Tom Henderson 2015-11-20 20:53:10 UTC

setting to NEEDINFO to get a good testcase

Comment 3 sebastien.deronne 2015-11-26 07:49:52 UTC

Using a map iso a flag sounds indeed better.
Tom, did you already check this solution?

Comment 4 Tom Henderson 2015-11-26 11:37:14 UTC

(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.

Comment 5 sebastien.deronne 2015-11-26 11:43:30 UTC

I agree.
I had started to include tests for A-MPDU; I should continue on that and include a case for this.

Comment 6 sebastien.deronne 2015-12-24 11:02:51 UTC

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.

Comment 7 sebastien.deronne 2015-12-24 11:05:04 UTC

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.

Comment 8 sebastien.deronne 2016-01-21 11:48:29 UTC

Any updates on this bug so far?

Comment 9 Tom Henderson 2016-02-21 14:48:55 UTC

(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.

Comment 10 sebastien.deronne 2016-02-21 15:53:51 UTC

(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.

Comment 11 Tom Henderson 2016-02-21 17:55:31 UTC

> 
> 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.

Comment 12 sebastien.deronne 2016-02-22 05:06:24 UTC

fixed in changeset 11892:02ada4d0519e