Bugzilla – Bug 2373
CsmaNetDevice don't notify NetDeviceQueueInterface
Last modified: 2017-03-09 10:35:51 UTC
Considering the inclusion of the Traffic Control Layer in the ns-3.25 release, the NetDevice was enhanced with a NetDeviceQueueInterface object aggregated to it and used to notify the queue disc about device queue status. The PointToPointNetDevice uses this object to call Stop, Start and Wake methods. However, this is not implemented into CsmaNetDevice. Because of that, some packets may get stuck in the traffic control layer forever if the CsmaNetDevice queue gets full and packets are enqueued by queue disc.
Could you please provide an example that shows that packets actually get stuck in the traffic control layer forever?
Created attachment 2384 [details] Test case inducing the error
Created attachment 2385 [details] Proposed patch This is a proposed patch for the error, based on the changes introduced in the PointToPointNetDevice. It seams to solve the problem, but I'm not sure if it is ok.
(In reply to Luciano from comment #2) > Created attachment 2384 [details] > Test case inducing the error This test case shows two nodes connected over csma or p2p links (change the type with --useP2p command line argument). For p2p links everything works fine, but for csma links the packets get stucked in the queue discipline.
What happens here is that the queue disc stores the packets waiting to be transmitted to the netdevice (with traffic control, packets are not lost if the netdevice queue is full, but they are re-queued by the queue disc and re-transmitted). The transmission to a netdevice is triggered either by the reception of a packet from the upper layers or by the netdevice "waking up" the queue disc. Hence, when the application is done sending packets, the packets stored in the queue disc stay there unless the netdevice wakes up the queue disc. This requires that the netdevice supports flow control. We definitely want more netdevices to support flow control. Thank you for your patch. I will carefully review it as soon as possible.
(In reply to Stefano Avallone from comment #5) > We definitely want more netdevices to support flow control. Thank you for > your patch. I will carefully review it as soon as possible. Hi, I have two questions: *) It is possible that this bug is happening somewhere else (e.g. in any NetDevice which does not support flow control) ? and *) It is possible to have some kind of strategy to overcome this problem for non-tc NetDevices ? As a matter of fact, I'm guessing to disable packet re-queueing if the NetDevice does not support TC.
Hi, > *) It is possible that this bug is happening somewhere else (e.g. in any > NetDevice which does not support flow control) ? Yes. > *) It is possible to have some kind of strategy to overcome this problem for > non-tc NetDevices ? As a matter of fact, I'm guessing to disable packet > re-queueing if the NetDevice does not support TC. That may be a workaround for the time being (until all netdevices support flow control). I am going to post on the developers list to present an overview of the issues introduced by the requeuing mechanism and the workaround you mention. Thanks.
Hi all, I was thinking about this bug and maybe an workaround could be the selective install of queue disc only for NetDevices that already has traffic control support. This type of device could be filtered at AddInterface (). I believe that disabling the requeuing feature won't solve the problem.
I have mentioned this issue in a post I just sent to the developers mailing list: http://mailman.isi.edu/pipermail/ns-developers/2016-April/013524.html Let's see the outcome of the discussion that (hopefully) will come from it...
So, from what I got, the ns-3-dev commit "traffic-control: (fixes #2284) Never requeue a packet sent to a netdevice" also fixes this bug, right? Considering that the way that devices becomes aware of TC has changes (using the NotifyNewAggregate to get a pointer to the queue interface), the pending patch on this bug doesn't work anymore. I think that it is possible to close this bug.
I would leave this bug open as a reminder that flow control support must be added to CsmaNetDevice. Unfortunately, it will not make it for ns-3.26, but I plan to add flow control support for more devices (including CsmaNetDevice) for the next release cycle.
Created attachment 2635 [details] Updated patch for ns-3.26 Considering the new ns-3.26 version, I have updated the patch for the CsmaNetDevice and Traffic Control module compatibility. The patch follows the same logic that is applied to the PointToPointNetDevice, and works ok with the test cased available here.
Thank you for working on this. I am sorry, I actually added (and tested a bit) the support for flow control and bql to csma net devices at the end of July, but didn't publish the patch yet. Now I published the patch: https://codereview.appspot.com/314910043/ Your patch is very close to mine, the most important differences being the absence of an assert and different calls to bql functions. I would be very grateful to you if you could test my patch as well.
Hi, Thank you for publishing your patch. When I was reviewing it I figured out that the patch I attached here is calling the bql functions in the wrong way (some places should be NotifyTransmittedBytes instead of NotifyQueuedBytes). Anyway, I've just placed some comments on your patch review. I'm also testing your patch and if I find a problem I'll report it here. Thank you again for your work on this module! Cheers Luciano.
Created attachment 2749 [details] Revised patch for ns-3.26 This is the most recent revised patch for this bug. It was obtained from the code review https://codereview.appspot.com/314910043/, and slightly modified just to ensure ns-3 coding style (no changes on code semantic). This patch can be applied on top of ns-3.26.
Proper support for flow control and BQL in csma devices finally landed to ns-3-dev in the context of the effort to make Queue a template class.
*** Bug 2610 has been marked as a duplicate of this bug. ***