Add support for flow control and BQL to CsmaNetDevice

Add support for flow control and BQL to CsmaNetDevice

[Luciano Chaves, 2016-10-25 15:54:23]

I've just reviewed this patch, and it seems to be working ok on my initial
tests.

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
File src/csma/model/csma-net-device.cc (right):

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:615: txq->Start ();
Is there any problem on calling the txq->Start () for a qdisc that is already
started? On the P2pNetDevice this call is only performed if txq->IsStopped () is
true.

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:1041: 
Following the same code on P2pNetDevice, I would place an assert here to ensure
that the queue is not stopped.

NS_ASSERT_MSG (!txq || !txq->IsStopped (), "Send should not be called when the
device is stopped");

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:1104: if ((m_queue->GetMode () ==
Queue::QUEUE_MODE_PACKETS &&
This is working ok, but there's no need to check if there is still room for
another packet if the queue is in packet mode (only in byte mode the enqueued
packet might be larger than the dequeued packet, thus leaving no room for
another packet).

[Luciano Chaves, 2016-10-25 18:02:17]

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
File src/csma/model/csma-net-device.cc (right):

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:220: m_queueInterface = 0;
It also be good to initialize the pointer on constructor with m_queueInterface =
0;
This can prevent problems with the pointer test at NotifyNewAggregate ()

[Stefano Avallone, 2016-10-31 18:59:19]

Thanks a lot for the review!

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
File src/csma/model/csma-net-device.cc (right):

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:220: m_queueInterface = 0;
Ptr<>s should be automatically initialized to zero.

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:615: txq->Start ();
There is no problem in calling txq->Start for a qdisc that is not stopped.
Anyway, the check if txq->IsStopped () can be safely added.

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:1041: 
Actually, it would be better to remove such assert from p2p devices. The reason
is that there are non-IP packets (such as ARP packets) that do not pass through
the traffic control and hence could be sent while the device queue is stopped.

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:1104: if ((m_queue->GetMode () ==
Queue::QUEUE_MODE_PACKETS &&
You are right, but probably, for future reference, it is better to explicitly
leave the check also in the case the queue is in packet mode.

[Luciano Chaves, 2016-11-02 17:18:14]

Hi,

I've been using this patch on my experiments and it is working ok. I think you
could push it forward to be include in the next ns-3 release.
Thanks for your work on this!

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
File src/csma/model/csma-net-device.cc (right):

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:615: txq->Start ();
If there is no problem on calling txq->Start for a stopped qdisc a don't think
that it is necessary to check for txq->IsStopped ().
On 2016/10/31 18:59:18, Stefano Avallone wrote:
> There is no problem in calling txq->Start for a qdisc that is not stopped.
> Anyway, the check if txq->IsStopped () can be safely added.

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:1041: 
Oh, I didn't know about that. Only IP packets pass through the TC?
Anyway, in this case it is really necessary to remove this assert from
P2pNetDevice.

On 2016/10/31 18:59:18, Stefano Avallone wrote:
> Actually, it would be better to remove such assert from p2p devices. The
reason
> is that there are non-IP packets (such as ARP packets) that do not pass
through
> the traffic control and hence could be sent while the device queue is stopped.

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:1104: if ((m_queue->GetMode () ==
Queue::QUEUE_MODE_PACKETS &&
It seems ok to me.
On 2016/10/31 18:59:18, Stefano Avallone wrote:
> You are right, but probably, for future reference, it is better to explicitly
> leave the check also in the case the queue is in packet mode.

[Stefano Avallone, 2016-11-07 09:33:57]

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
File src/csma/model/csma-net-device.cc (right):

https://codereview.appspot.com/314910043/diff/1/src/csma/model/csma-net-devic...
src/csma/model/csma-net-device.cc:1041: 
Yes, only Ipv{4,6}L3Protocol send packets to the traffic control.