Bug 2222

Summary: incorrect EDCA behavior in case of internal collision
Product: ns-3 Reporter: levente.meszaros
Component: wifiAssignee: sebastien.deronne
Status: RESOLVED FIXED    
Severity: normal CC: matis18, ns-bugs, tomh
Priority: P3    
Version: ns-3.25   
Hardware: All   
OS: All   
Attachments: Proposed patch to fix
patch to handle virtual collisions
test case
test case

Description levente.meszaros 2015-11-20 05:40:16 EST
The current EDCA implementation doesn't increase the contention window and it also doesn't increment the retry counter when internal collision happens.

The 802.11-2012 standard explicitly states otherwise in 9.19.2.5 EDCA backoff procedure.
Comment 1 sebastien.deronne 2015-11-20 05:45:45 EST
Could you please detail a bit more?
Also, traces and/or a simulation script would be useful here in order to reproduce and investigate the problem.

But first of all, is this not a duplicate of bug 1205???
Comment 2 levente.meszaros 2015-11-20 06:05:59 EST
Thanks for the quick answer. No, it's not a duplicate of bug 1205.

More details: assume two or more EDCA txops contend for the channel and internal collision happens. The winner continues with the transmission and the loser starts the backoff procedure. According to the standard the contention window must be increased and the retry counter must be incremented  before the backoff procedure starts. This is the same procedure as if an external collision would have happened.

I'll try to cook something to reproduce.
Comment 3 sebastien.deronne 2016-03-31 16:55:11 EDT
(In reply to levente.meszaros from comment #2)
> Thanks for the quick answer. No, it's not a duplicate of bug 1205.
> 
> More details: assume two or more EDCA txops contend for the channel and
> internal collision happens. The winner continues with the transmission and
> the loser starts the backoff procedure. According to the standard the
> contention window must be increased and the retry counter must be
> incremented  before the backoff procedure starts. This is the same procedure
> as if an external collision would have happened.
> 
> I'll try to cook something to reproduce.

Did you prepare a test case in the meantime?
Comment 4 levente.meszaros 2016-04-04 03:56:15 EDT
No, I didn't. Unfortunately I don't have time for this, so it's very unlikely that I will, sorry.
Comment 5 sebastien.deronne 2016-04-24 06:00:13 EDT
Looking back at the description and the bug is quite clear. 
Current code looks indeed wrong: unlike what is mentioned in 9.19.2.5 EDCA backoff procedure, we do not increase the contention window once an internal collision has been detected.
Comment 6 sebastien.deronne 2016-04-24 06:45:18 EDT
It is stated the appropriate retry counter should be increased. But I have doubts on how to handle the case where we reach the maximum number of retries. 

Should we notify it toward the station manager? 
We currently should in order to increase the retry counter, but I am not sure whether the rate manager should also adapt its selected rate in case of internal collisions?

Also, a retry counter could be first incremented because of an internal collisions, and later on it might get increased because of an external collision. In that case, what should be notified to the rate manager?
Comment 7 levente.meszaros 2016-04-25 06:17:53 EDT
These are questions I didn't think of and I don't really have a good answer.
Comment 8 sebastien.deronne 2016-04-27 15:58:38 EDT
Matias maybe has a better view on this?
Comment 9 Matías Richart 2016-04-28 07:44:30 EDT
I think we must act the same way as any other type of retransmissions. I mean, if retry limit is reached, the frame should be discarded and inform this to the manager.

This is what I understand from this paragraph:
"For internal collisions occurring with the EDCA access method, the appropriate retry counters (short retry counter for MSDU, A-MSDU, or MMPDU and QSRC[AC] or long retry counter for MSDU, A-MSDU, or MMPDU and QLRC[AC]) are incremented. For transmissions that use Block Ack, the rules in 9.21.3 also apply. STAs shall retry failed transmissions until the transmission is successful or until the relevant retry limit is reached."

Regarding the last two question of Sebastien:
Deciding when to change rates depending on the type of loss (collision, low SNR, interference, etc) is one of the problems of rate adaptation mechanisms. Currently most mechanisms just consider frame losses, independent of the type of loss.
So, I think we should not distinguish between if it is an internal or external collision. As we do not distinguish if loss occur because of a collision or low SNR.
Comment 10 sebastien.deronne 2016-04-30 12:15:08 EDT
Created attachment 2407 [details]
Proposed patch to fix

Here is a patch that might address this bug.
I have not tested it at all (just checked regression), I rely on the submitter to have a look.
Comment 11 levente.meszaros 2016-05-02 05:19:29 EDT
I no longer work regularly with ns3, so I cannot find the time to check this patch. Sorry for not being helpful.
Comment 12 sebastien.deronne 2016-06-04 06:55:32 EDT
I believe what we have now is already better than what was previously handled in case of internal collision.

But how was this bug noticed? Don't you have any example we could include in our regression?
Comment 13 levente.meszaros 2016-06-13 04:11:05 EDT
This bug was found while I was comparing the 802.11 behavior to another simulator. Unfortunately I don't have a test case.
Comment 14 sebastien.deronne 2016-06-14 15:18:36 EDT
Created attachment 2470 [details]
patch to handle virtual collisions

fixes issues with previous patch
Comment 15 sebastien.deronne 2016-06-15 16:12:24 EDT
Created attachment 2474 [details]
test case

add test case
Comment 16 Tom Henderson 2016-06-23 12:24:25 EDT
(In reply to sebastien.deronne from comment #14)
> Created attachment 2470 [details]
> patch to handle virtual collisions
> 
> fixes issues with previous patch

Can you provide a few NS_LOG_DEBUG() statements in the heart of this code, to alert what is going on when logging is enabled?

https://www.nsnam.org/bugzilla/attachment.cgi?id=2470&action=diff#a/src/wifi/model/edca-txop-n.cc_sec4

Otherwise, patch looks good to me (haven't tested).
Comment 17 Tom Henderson 2016-06-23 12:39:51 EDT
(In reply to sebastien.deronne from comment #15)
> Created attachment 2474 [details]
> test case
> 
> add test case

Three main comments:

1) The test patch introduces dependency on internet and applications module, which must be avoided.  It is unnecessary; a PacketSocket will suffice for this.

2) Although the need for the new 'Tx' callback will be obviated once 1) is addresssed, I suggest to avoid modifying packets (tagging them) through a Tx callback.  

I know there is discussion of this in Packet::AddByteTag that this tagging is OK, but in this specific case, it seems to violate the logical constness of the packet since the model code relies on the QoS tag.

3) This test causes two packets to collide, but only one TxDataFailed callback is invoked.  Shouldn't two be invoked?
Comment 18 sebastien.deronne 2016-06-29 15:17:07 EDT
Tom, what is then the best way to set QoS when using a PacketSocket?

Since this is internal collision, this means the packet with higher priority will be sent, and that internal collision is triggered for packets with lower priorities (so here, only one packet failed)
Comment 19 Tom Henderson 2016-06-29 16:38:31 EDT
(In reply to sebastien.deronne from comment #18)
> Tom, what is then the best way to set QoS when using a PacketSocket?


With the existing codebase, the test case could subclass PacketSocketClient and create a new virtual StartApplication and (non-virtual) SendWithQos() method; the latter of which could be the same as Send() but also attach the QosTag.

With possible 802.1p (bug 2241) support, then that solution could be used.

> 
> Since this is internal collision, this means the packet with higher priority
> will be sent, and that internal collision is triggered for packets with
> lower priorities (so here, only one packet failed)

OK
Comment 20 sebastien.deronne 2016-07-01 14:31:14 EDT
Tom, I guess the same solution as for the TXOP example should be used?
In that case, I am waiting for Stefano's solution.
Comment 21 sebastien.deronne 2016-07-31 04:10:36 EDT
Created attachment 2506 [details]
test case

test case updated
Comment 22 sebastien.deronne 2016-07-31 04:11:34 EDT
This fix should be included in the upcoming release. 
Any comments/remarks before pushing?
Comment 23 sebastien.deronne 2016-08-05 17:06:47 EDT
pushed in changesets 12241:ddaf3c4a8e4e and 12242:8b197f75774b