Introduce the traffic control layer

Introduce the traffic control layer

[Tom Henderson, 2016-02-17 06:28:18]

I haven't reviewed all files, but I agree with the overall approach and think it
is close.  Please see a few scattered comments so far.  Main ones:

- should PointToPointNetDevice Drop trace be hit for a packet that fails to
enqueue but that Tc layer requeues?  Can we avoid this drop trace?

- the traffic control example could be expanded to trace also the device queue
layer in a similar manner, could perhaps have an option for UDP instead of TCP,
and could add CommandLine argument handling

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
File examples/traffic-control/traffic-control.cc (right):

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:29: // This simple example shows how
to use TrafficContorlHelper to install a QueueDisc on a device.
Control

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:46: }
consider to rename the above to 'TcPacketsInQueueTrace' and add also:

+void
+DevicePacketsInQueueTrace (uint32_t oldValue, uint32_t newValue)
+{
+  std::cout << "DevicePacketsInQueue " << oldValue << " to " << newValue << std
::endl;
+}

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:52: 
consider to add 

+ CommandLine cmd;

suggest to add option for UDP or TCP

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:70: tch.AddInternalQueues (handle,
3, "ns3::DropTailQueue", "MaxPackets", UintegerValue (1000));
add a comment here about the value 3 corresponding the three bands

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:74: Ptr<QueueDisc> q = qdiscs.Get
(0);
why not Get (1) (to get the queue on n1, which is the source of the application
data)?

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:76: 
Can add here also (to trigger the new callback above):

+  Ptr<NetDevice> nd = devices.Get (1);
+  Ptr<PointToPointNetDevice> ptpnd = DynamicCast<PointToPointNetDevice> (nd);
+  Ptr<Queue> queue = ptpnd->GetQueue ();
+  queue->TraceConnectWithoutContext ("PacketsInQueue", MakeCallback
(&DevicePacketsInQueueTrace));

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:98: onoff.SetAttribute ("DataRate",
DataRateValue (1000000000)); //bit/s
data rate is pretty high, especially for UDP-- consider to lower it.

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device.cc
File src/network/model/net-device.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device....
src/network/model/net-device.cc:100: NS_LOG_FUNCTION_NOARGS ();
NS_LOG_FUNCTION (this);

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device....
src/network/model/net-device.cc:135: " a queue disc on the device.");
what is the right behavior here, to continue with a warning or to exit the
simulation (configuration error)?

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device.h
File src/network/model/net-device.h (right):

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device....
src/network/model/net-device.h:84: inline bool IsStopped (void) const;
why are these inline?  We typically do not inline unless we have profiling data
that supports doing so.

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
File src/point-to-point/model/point-to-point-net-device.cc (left):

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
src/point-to-point/model/point-to-point-net-device.cc:554: m_macTxDropTrace
(packet);
here, the packet is requeued by tc layer, but there is a drop trace logged.  I
think this may be confusing to users.

Could we stop the queue when full (i.e. one Enqueue before this one) to avoid
sending to a full queue?  May work in packets mode, possibly harder in bytes
mode.

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
File src/point-to-point/model/point-to-point-net-device.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
src/point-to-point/model/point-to-point-net-device.cc:526: NS_LOG_WARN ("Send
should not be called when the device is stopped");
should this cause a drop and return?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pac...
File src/traffic-control/model/packet-filter.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pac...
src/traffic-control/model/packet-filter.h:45: #define PF_NO_MATCH -1
better as 'static const int PF_NO_MATCH'

https://codereview.appspot.com/284530043/diff/1/src/wifi/model/regular-wifi-m...
File src/wifi/model/regular-wifi-mac.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/wifi/model/regular-wifi-m...
src/wifi/model/regular-wifi-mac.cc:445: if (m_qosSupported)
add braces

[Tommaso Pecorella, 2016-02-17 20:54:21]

A couple of comments before starting to dissect the code.

1) The DropTailQueue seems to be left "alone" by the refactoring, while RED has
been moved. In my opinion DropTail should be the most stupid queue discipline
available, isn't it ?

2) Making Node::ProtocolHandlerEntry public and using it "directly" in the
TrafficControl class doesn't seems to be wise. As a matter of fact
Node::ProtocolHandlerEntry is an implementation detail, and the two classes
should have separate implementation.
Duplicating the structure isn't dramatic, it's just a few fields.

3) Triple check Doxygen please, I already spotted more than one undocumented
thing. I assume that you can find all the Doxygen problems by yourself by using
the doxygen.warnings.report.sh command, so I'll not bother to comment them.

[Stefano Avallone, 2016-02-17 22:50:05]

Tom, thanks a lot for the review.

On 2016/02/17 06:28:18, Tom Henderson wrote:
> I haven't reviewed all files, but I agree with the overall approach and think
it
> is close.  Please see a few scattered comments so far.  Main ones:
> 
> - should PointToPointNetDevice Drop trace be hit for a packet that fails to
> enqueue but that Tc layer requeues?  Can we avoid this drop trace?

If you think it is confusing that a drop event is traced while the packet is not
actually lost (because it is requeued by the queue disc), we might avoid calling
m_macTxDropTrace here.

Preventing droppings due to overflow of the device queue is surely something we
should
aim for, but it should be designed carefully in my opinion. I think it would be
good to provide a more general strategy, such as stopping the queue when its
occupancy is above a configurable threshold (to emulate what some device drivers
do in Linux). But this requires a bit of effort to design it well. So, I would
postpone this to after the release of ns-3.25.

> - the traffic control example could be expanded to trace also the device queue
> layer in a similar manner, could perhaps have an option for UDP instead of
TCP,
> and could add CommandLine argument handling

Done.

[Stefano Avallone, 2016-02-17 22:50:43]

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
File examples/traffic-control/traffic-control.cc (right):

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:29: // This simple example shows how
to use TrafficContorlHelper to install a QueueDisc on a device.
On 2016/02/17 06:28:17, Tom Henderson wrote:
> Control

Done.

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:46: }
On 2016/02/17 06:28:17, Tom Henderson wrote:
> consider to rename the above to 'TcPacketsInQueueTrace' and add also:
> 
> +void
> +DevicePacketsInQueueTrace (uint32_t oldValue, uint32_t newValue)
> +{
> +  std::cout << "DevicePacketsInQueue " << oldValue << " to " << newValue <<
std
> ::endl;
> +}

Done.

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:52: 
On 2016/02/17 06:28:17, Tom Henderson wrote:
> consider to add 
> 
> + CommandLine cmd;
> 
> suggest to add option for UDP or TCP

Done.

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:70: tch.AddInternalQueues (handle,
3, "ns3::DropTailQueue", "MaxPackets", UintegerValue (1000));
On 2016/02/17 06:28:17, Tom Henderson wrote:
> add a comment here about the value 3 corresponding the three bands

Done.

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:74: Ptr<QueueDisc> q = qdiscs.Get
(0);
On 2016/02/17 06:28:17, Tom Henderson wrote:
> why not Get (1) (to get the queue on n1, which is the source of the
application
> data)?

Done.

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:76: 
On 2016/02/17 06:28:17, Tom Henderson wrote:
> Can add here also (to trigger the new callback above):
> 
> +  Ptr<NetDevice> nd = devices.Get (1);
> +  Ptr<PointToPointNetDevice> ptpnd = DynamicCast<PointToPointNetDevice> (nd);
> +  Ptr<Queue> queue = ptpnd->GetQueue ();
> +  queue->TraceConnectWithoutContext ("PacketsInQueue", MakeCallback
> (&DevicePacketsInQueueTrace));

Done.

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:98: onoff.SetAttribute ("DataRate",
DataRateValue (1000000000)); //bit/s
On 2016/02/17 06:28:17, Tom Henderson wrote:
> data rate is pretty high, especially for UDP-- consider to lower it.

Done.

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device.cc
File src/network/model/net-device.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device....
src/network/model/net-device.cc:100: NS_LOG_FUNCTION_NOARGS ();
On 2016/02/17 06:28:17, Tom Henderson wrote:
> NS_LOG_FUNCTION (this);

Done.

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device....
src/network/model/net-device.cc:135: " a queue disc on the device.");
On 2016/02/17 06:28:17, Tom Henderson wrote:
> what is the right behavior here, to continue with a warning or to exit the
> simulation (configuration error)?

Okay, I used a NS_ABORT_MSG_IF instead

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device.h
File src/network/model/net-device.h (right):

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device....
src/network/model/net-device.h:84: inline bool IsStopped (void) const;
On 2016/02/17 06:28:18, Tom Henderson wrote:
> why are these inline?  We typically do not inline unless we have profiling
data
> that supports doing so.

Done.

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
File src/point-to-point/model/point-to-point-net-device.cc (left):

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
src/point-to-point/model/point-to-point-net-device.cc:554: m_macTxDropTrace
(packet);
On 2016/02/17 06:28:18, Tom Henderson wrote:
> here, the packet is requeued by tc layer, but there is a drop trace logged.  I
> think this may be confusing to users.
> Could we stop the queue when full (i.e. one Enqueue before this one) to avoid
> sending to a full queue?  May work in packets mode, possibly harder in bytes
> mode.

If you think it is confusing that a drop event is traced while the packet is not
actually lost (because it is requeued by the queue disc), we might avoid calling
m_macTxDropTrace here.

Preventing droppings due to overflow of the device queue is surely something we
aim for, but it should be designed carefully in my opinion. I think it would be
good to provide a more general strategy, such as stopping the queue when its
occupancy is above a configurable threshold (to emulate what some device drivers
do in Linux). But this requires a bit of effort to design it well. So, I would
postpone this to after the release of ns-3.25.

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
File src/point-to-point/model/point-to-point-net-device.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
src/point-to-point/model/point-to-point-net-device.cc:526: NS_LOG_WARN ("Send
should not be called when the device is stopped");
On 2016/02/17 06:28:18, Tom Henderson wrote:
> should this cause a drop and return?

Even worse... The traffic control layer should not send a packet when a device
is stopped. So, if that happens, it means that something went wrong.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pac...
File src/traffic-control/model/packet-filter.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pac...
src/traffic-control/model/packet-filter.h:45: #define PF_NO_MATCH -1
On 2016/02/17 06:28:18, Tom Henderson wrote:
> better as 'static const int PF_NO_MATCH'

Done.

https://codereview.appspot.com/284530043/diff/1/src/wifi/model/regular-wifi-m...
File src/wifi/model/regular-wifi-mac.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/wifi/model/regular-wifi-m...
src/wifi/model/regular-wifi-mac.cc:445: if (m_qosSupported)
On 2016/02/17 06:28:18, Tom Henderson wrote:
> add braces

Done.

[Stefano Avallone, 2016-02-17 23:57:52]

Tommaso,

thanks a lot for the review.

On 2016/02/17 20:54:21, Tommaso Pecorella wrote:
> A couple of comments before starting to dissect the code.
> 
> 1) The DropTailQueue seems to be left "alone" by the refactoring, while RED
has
> been moved. In my opinion DropTail should be the most stupid queue discipline
> available, isn't it ?

If necessary, we might add a Fifo queue disc (just like in Linux). DropTail
should remain a subclass of Queue.

> 2) Making Node::ProtocolHandlerEntry public and using it "directly" in the
> TrafficControl class doesn't seems to be wise. As a matter of fact
> Node::ProtocolHandlerEntry is an implementation detail, and the two classes
> should have separate implementation.
> Duplicating the structure isn't dramatic, it's just a few fields.

I removed the commit that made Node::ProtocolHandlerEntry public and duplicated
this struct in TrafficControlLayer, thanks.

> 3) Triple check Doxygen please, I already spotted more than one undocumented
> thing. I assume that you can find all the Doxygen problems by yourself by
using
> the doxygen.warnings.report.sh command, so I'll not bother to comment them.

I fixed many of these warnings (I think all those in the traffic control
module). Will double check tomorrow, thanks.

[Tommaso Pecorella, 2016-02-18 00:00:36]

On 2016/02/17 23:57:52, Stefano Avallone wrote:
> Tommaso,
> 
> thanks a lot for the review.

Wait to see the rest...

> On 2016/02/17 20:54:21, Tommaso Pecorella wrote:
> > 1) The DropTailQueue seems to be left "alone" by the refactoring, while RED
> has
> > been moved. In my opinion DropTail should be the most stupid queue
discipline
> > available, isn't it ?
> 
> If necessary, we might add a Fifo queue disc (just like in Linux). DropTail
> should remain a subclass of Queue.

That's exactly what I didn't get. if you want you can explain to me in a direct
e-mail or a Skype call.

[Tommaso Pecorella, 2016-02-18 00:01:19]

Second batch of comments (still very partial). More tomorrow.

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
File examples/traffic-control/traffic-control.cc (right):

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:98: onoff.SetAttribute ("DataRate",
DataRateValue (1000000000)); //bit/s
Better to use the alphanumeric format, it's more readable ("1Gbps")

https://codereview.appspot.com/284530043/diff/1/src/internet/model/arp-cache.cc
File src/internet/model/arp-cache.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/arp-cache....
src/internet/model/arp-cache.cc:404: ArpCache::Entry::UpdateWaitReply
(PacketWithIpv4Header waiting)
Also this function must not be called with a null packet.
You can add an assert and remove all the checks in the calling code.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/arp-cache....
src/internet/model/arp-cache.cc:420: ArpCache::Entry::MarkWaitReply
(PacketWithIpv4Header waiting)
All this is a complex nightmare, only based on the assumption that "waiting"
could be null.
The problem is: it must NOT be null, you can safely add an assert and check that
it's not null.

As a consequence, all the checks that the ArpCache could contain a null packet
are simply useless.

Note, there is ONE place in the code where this function is called with a null
argument (one of the LTE tests).
The solution for that case in in another comment.

https://codereview.appspot.com/284530043/diff/1/src/lte/test/epc-test-s1u-upl...
File src/lte/test/epc-test-s1u-uplink.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/lte/test/epc-test-s1u-upl...
src/lte/test/epc-test-s1u-uplink.cc:405: 
Nope, the right thing to do is:
+         arpCacheEntry->SetMacAddresss (Mac48Address::GetBroadcast ());
+         arpCacheEntry->MarkPermanent ();
-         arpCacheEntry->MarkWaitReply(0);
-         arpCacheEntry->MarkAlive (Mac48Address::GetBroadcast ()); 

The original lines had a bug too, i.e., when the entry will expire, it will not
be replaced or refreshed.

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device.h
File src/network/model/net-device.h (right):

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device....
src/network/model/net-device.h:102: virtual void Wake (void);
It is totally unclear (and counterintuitive) that this function will try to send
an enqueued packet in the NetDevice.

It's not a problem for me, but it must be documented. Moreover, assuming I'm
right, it's unclear why you're calling a dequeue and then a wake (e.g., in
PointToPointnetDevice)

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
File src/point-to-point/model/point-to-point-net-device.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
src/point-to-point/model/point-to-point-net-device.cc:217: m_queue = 0;
I don't understand (anymore) the difference between this queue and the TC queue.
One seems redundant, mind to explain ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/pfifo...
File src/traffic-control/doc/pfifo-fast.rst (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/pfifo...
src/traffic-control/doc/pfifo-fast.rst:21: provided, three DropTail queues
having each a capacity equal to limit are
Why 3 ?
And why they MUST be 3 ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/pfifo...
src/traffic-control/doc/pfifo-fast.rst:34: References
Avoid empty sections

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
File src/traffic-control/doc/queue-discs.rst (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:15: A netdevice can have a single (root)
queue disc installed on it.
NetDevice or ns3::NetDevice (dunno what's the correct one to have a link)

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:35: classes). More recently, after the
appearance of multi-queue devices (such as Wifi),
Wi-Fi

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:90: instance, for Ipv4 and Ipv6 packets
it is needed to separately store the header
IPv4 and IPv6 (the ns-3 classes are CamelCase, the normal names aren't)

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:131: Back to the qdisc field of the
struct netdev_queue in Linux. Such a field cannot be
The first sentence is too "Italian". Please rephrase.
Moreover, it's not *that* necessary to explain in detail all the design choices
and why something as been done in a particular way. It's enough to say "Contrary
to Linux, ns-3 [does something]. This is to avoid unnecessary modules
dependencies."

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:186: the legacy behavior (i.e., no queue
disc) and to install a different queue disc
Is it possible *at all* to restore the legacy behaviour ?
After the changes in the NetDevices, I have some doubts.
Moreover, do we *want* to restore the legacy behaviour ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:200: tch.AddInternalQueues (handle, 3,
"ns3::DropTailQueue", "MaxPackets", UintegerValue (1000));
Explain that you did set 3 queues in this way.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/traff...
File src/traffic-control/doc/traffic-control-layer.rst (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/traff...
src/traffic-control/doc/traffic-control-layer.rst:15:
**************************************************************
Please do not confuse the users.
Consider that the documentation is to be read by the users and it will be there
for a long time.
Users are interested into the actual behaviour, not in the old one.

A section about the differences between the new and the old call chains is
totally useful, but it should be presented as the last thing, and just for the
interested people (e.g., the ones doing a forward-port).

On the opposite, you have spent a lot of space to describe the old (and it's a
very good description, mind) but almost nothing to describe the new one.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
File src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:36: *                 
                                                   -----------------
Spurious spaces ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:38: * (2) PfifoFast,
CoDel, FqCoDel, SfqCoDel, etc.
Do we have all these queueing disciplines ?
Remember the user... they don't read, but when they do they expect that
everything is exact.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:250: cmd.AddValue
("routerWanQueueType", "Router WAN net device queue type", routerWanQueueType);
Mention the two allowed types.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:360: }
else ? Error ?
Remember the drunk monkey test.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:434:
outputConfig.ConfigureAttributes ();
Are you sure the user wants the list of all the attributes, the defaults, etc. ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
File src/traffic-control/examples/pfifo-vs-red.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/pfifo-vs-red.cc:16: * Author: John Abraham
<john.abraham@gatech.edu>
John ?
Double check the copyrights (not just this one)

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
File src/traffic-control/model/pfifo-fast-queue-disc.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:57:
PfifoFastQueueDisc::DoEnqueue (Ptr<QueueDiscItem> item)
I haven't read in the documentation what is the effect of setting an internal
queue size greater than 1/3 of the total pool.
E.g., If Pfifo size is 1000 and internal queues are respectively 333, 333, 334,
then all is obvious, they have separate pools.
If internal queues are 1000, 1000 and 1000, then they have a shared buffer, and
the highest priority sub-queue can occupy all the resources.
Settings in between lead to a mixed system (e.g., 400, 400 and 400 means that
all of them have at least 200 packets guaranteed).
This should be mentioned (in my opinion).

Still, why 3 ?
No, it's not a magic number. that's 42.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:73: band = 1;
Why using band = 1 for unclassifiable packets ?
If you have 3 bands, there's a 0 band and a 2 band. Using the "middle" one is...
puzzling ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:81: if
(!GetInternalQueue(band)->Enqueue (item))
What happens if you have less queues than filters ?
I know that it shouldn't be possible, but...

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:101: if ((item =
DynamicCast<QueueDiscItem> (GetInternalQueue (i)->Dequeue ())) != 0)
DynamicCast is a slow operation. is there a way to avoid it ?
Edit: What the *** could return Dequeue if not a QueueDiscItem ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:122: item = DynamicCast<const
QueueDiscItem> (GetInternalQueue (i)->Peek ());
Same as above. Don't use DynamicCast if it's not necessary.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
File src/traffic-control/model/pfifo-fast-queue-disc.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.h:31: * to three priority bands
based on their Type of Service bits or DSCP bits.
Again the 3 number. Problem is: TOS and DSCP is not just 3. Please explain a bit
more.
Remember: the normal user is barely reading the documentation, don't assume that
he/she also knows about DiffServ classes.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.h:49: * service).
Earlier you mentions DSCP. In DSCP the "no classification" is BE, as is 2.
Explain (not only to me, to the user)

[Stefano Avallone, 2016-02-18 00:03:18]

Patch set 2 is the latest version taking into account Tom's and Tommaso's recent
comments. I also rebased my branch on the latest ns-3-dev. Due to this,
unfortunately patch set 2 contains some changes that went into ns-3-dev
recently. I hope they can be easily recognized. Tomorrow I will see if I can
solve this issue.

Anyway, I also updated the github branch
https://github.com/stavallo/ns-3-dev-git/commits/queue-disc-exp

Thanks,
Stefano

[Stefano Avallone, 2016-02-18 16:44:28]

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
File examples/traffic-control/traffic-control.cc (right):

https://codereview.appspot.com/284530043/diff/1/examples/traffic-control/traf...
examples/traffic-control/traffic-control.cc:98: onoff.SetAttribute ("DataRate",
DataRateValue (1000000000)); //bit/s
On 2016/02/18 00:01:18, Tommaso Pecorella wrote:
> Better to use the alphanumeric format, it's more readable ("1Gbps")

Done.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/arp-cache.cc
File src/internet/model/arp-cache.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/arp-cache....
src/internet/model/arp-cache.cc:420: ArpCache::Entry::MarkWaitReply
(PacketWithIpv4Header waiting)
Sorry, I don't get your point. Maybe it is due to my limited knowledge of this
code, but I don't see any nightmare...

Anyway, I think what you refer to is not strictly related to the changes I am
introducing here. So, given that this review request is already extremely large,
I would move the changes you mention to a separate patch.

https://codereview.appspot.com/284530043/diff/1/src/lte/test/epc-test-s1u-upl...
File src/lte/test/epc-test-s1u-uplink.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/lte/test/epc-test-s1u-upl...
src/lte/test/epc-test-s1u-uplink.cc:405: 
Given that this solves an existing bug, would you mind to commit this change
before my traffic-control work? In this way, my patch would touch one less file.
Thanks.

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device.h
File src/network/model/net-device.h (right):

https://codereview.appspot.com/284530043/diff/1/src/network/model/net-device....
src/network/model/net-device.h:102: virtual void Wake (void);
I have slightly modified the documentation here to try to be more clear. Let me
explain better (might be of help for other comments you made). With the
introduction of the traffic control, we have two layers of queueing: queue discs
and netdevices. Here, when a netdevice notices that its own queue (assume one
queue for simplicity) is empty, it "wakes" the queue disc so that packets are
dequeued from the queue disc and sent down to the netdevice. So, in case of the
point-to-point netdevice, if a dequeue from the netdevice queue returns no
packet, the device wakes the queue disc to get packets to transmit. Not sure if
this is now more clear, I hope to be able to explain better if we manage to make
a skype call.

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
File src/point-to-point/model/point-to-point-net-device.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/point-to-point/model/poin...
src/point-to-point/model/point-to-point-net-device.cc:217: m_queue = 0;
Actually, the queuing at the queue disc layer is necessary to perform packet
scheduling (traffic prioritization, dropping, marking, etc) while the queuing at
the network device is necessary to buffer packets waiting for transmission.
Network device queues are usually simple FIFO queues, but there are exceptions.
Wi-Fi, for instance, performs its own traffic differentiation strategy (EDCA)
within the network device queues.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/pfifo...
File src/traffic-control/doc/pfifo-fast.rst (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/pfifo...
src/traffic-control/doc/pfifo-fast.rst:21: provided, three DropTail queues
having each a capacity equal to limit are
pfifo_fast in Linux has 3 queues and the number of queues is not configurable.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/pfifo...
src/traffic-control/doc/pfifo-fast.rst:34: References
On 2016/02/18 00:01:18, Tommaso Pecorella wrote:
> Avoid empty sections

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
File src/traffic-control/doc/queue-discs.rst (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:35: classes). More recently, after the
appearance of multi-queue devices (such as Wifi),
On 2016/02/18 00:01:18, Tommaso Pecorella wrote:
> Wi-Fi

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:90: instance, for Ipv4 and Ipv6 packets
it is needed to separately store the header
On 2016/02/18 00:01:18, Tommaso Pecorella wrote:
> IPv4 and IPv6 (the ns-3 classes are CamelCase, the normal names aren't)

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:131: Back to the qdisc field of the
struct netdev_queue in Linux. Such a field cannot be
I rephrased the first sentence. Did not change the rest of the paragraph,
because it clarifies how the wake callbacks are set. This paragraph can be
simply skipped by users that are not interested in such details.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:200: tch.AddInternalQueues (handle, 3,
"ns3::DropTailQueue", "MaxPackets", UintegerValue (1000));
On 2016/02/18 00:01:18, Tommaso Pecorella wrote:
> Explain that you did set 3 queues in this way.

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/traff...
File src/traffic-control/doc/traffic-control-layer.rst (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/traff...
src/traffic-control/doc/traffic-control-layer.rst:15:
**************************************************************
This piece of documentation was written by Natale. I agree with you, and tried
to improve this page a bit.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
File src/traffic-control/model/pfifo-fast-queue-disc.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:57:
PfifoFastQueueDisc::DoEnqueue (Ptr<QueueDiscItem> item)
The buffer must be always shared. So, I realized I made a mistake. The capacity
of each internal queue cannot be less than the limit of the queue disc. That's
because we might enqueue all the packets in the same internal queue, which
therefore needs to be at least as capacious as the queue disc. This is now
fixed.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:73: band = 1;
I followed the initial Tom's implementation in this regard. Let's see if Tom
replies before I investigate it better...

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:81: if
(!GetInternalQueue(band)->Enqueue (item))
No, no, it is possible indeed (if the user selects a filter -- not existing at
the moment -- that returns values not in {0,1,2}). I modified the previous
if-else to account for this situation.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:101: if ((item =
DynamicCast<QueueDiscItem> (GetInternalQueue (i)->Dequeue ())) != 0)
You're right, a dynamic cast is not necessary. I used a static cast instead.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.cc:122: item = DynamicCast<const
QueueDiscItem> (GetInternalQueue (i)->Peek ());
Same as above.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
File src/traffic-control/model/pfifo-fast-queue-disc.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.h:31: * to three priority bands
based on their Type of Service bits or DSCP bits.
I changed this paragraph into:

 * Linux pfifo_fast is the default priority queue enabled on Linux
 * systems. Packets are enqueued in three FIFO droptail queues according
 * to three priority bands based on the classification returned by
 * the configured packet filters.

which is more correct.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/pfi...
src/traffic-control/model/pfifo-fast-queue-disc.h:49: * service).
I changed this paragraph into:

 * It is necessary to provide at least one packet filter. To simulate the
 * Linux behavior, the PfifoFastIpv4PacketFilter and/or the
PfifoFastIpv6PacketFilter
 * shall be provided. These filters classify packets based on their Type of
 * Service bits or DSCP bits. If the filters are unable to classify a packet,
 * i.e., they return -1 (PF_NO_MATCH), that packet is enqueued into band 1
 * (normal service).

I hope that it is clear that classifying packets *based* on the ToS or DSCP does
not mean that the ToS or DSCP value is returned, but a value which is a function
of them is returned.

[Tom Henderson, 2016-02-18 17:18:38]

On 2016/02/17 22:50:05, Stefano Avallone wrote:
> Tom, thanks a lot for the review.
> 
> On 2016/02/17 06:28:18, Tom Henderson wrote:
> > I haven't reviewed all files, but I agree with the overall approach and
think
> it
> > is close.  Please see a few scattered comments so far.  Main ones:
> > 
> > - should PointToPointNetDevice Drop trace be hit for a packet that fails to
> > enqueue but that Tc layer requeues?  Can we avoid this drop trace?
> 
> If you think it is confusing that a drop event is traced while the packet is
not
> actually lost (because it is requeued by the queue disc), we might avoid
calling
> m_macTxDropTrace here.
> 
> Preventing droppings due to overflow of the device queue is surely something
we
> should
> aim for, but it should be designed carefully in my opinion. I think it would
be
> good to provide a more general strategy, such as stopping the queue when its
> occupancy is above a configurable threshold (to emulate what some device
drivers
> do in Linux). But this requires a bit of effort to design it well. So, I would
> postpone this to after the release of ns-3.25.

It seems to me that enhanced stopping behaviour could still be added later while
suppressing failed Enqueue() (where the fail can be easily predicted) could be
done now anyway.

Part of Natale's patch was to provide Queue::Available(); could it not be used
in QueueDisc::Transmit() to check (if IsStopped () == true || Available () == 0
packets)?

[Stefano Avallone, 2016-02-18 17:39:44]

On Thu, Feb 18, 2016 at 6:18 PM, <tomh.org@gmail.com> wrote:

>
> It seems to me that enhanced stopping behaviour could still be added
> later while suppressing failed Enqueue() (where the fail can be easily
> predicted) could be done now anyway.
>
> Part of Natale's patch was to provide Queue::Available(); could it not
> be used in QueueDisc::Transmit() to check (if IsStopped () == true ||
> Available () == 0 packets)?
>
>
I am (re-)preparing the second version of the patch and I'll try to
implement your suggestion. I should be able to post the new version in a
few hours.

Thanks,
Stefano


>
> https://codereview.appspot.com/284530043/
>

[Stefano Avallone, 2016-02-18 18:12:46]

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
File src/traffic-control/doc/queue-discs.rst (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:15: A netdevice can have a single (root)
queue disc installed on it.
Hmm... I only found a way to link to another model's page and it looks like
there is no page for Netdevice.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/doc/queue...
src/traffic-control/doc/queue-discs.rst:186: the legacy behavior (i.e., no queue
disc) and to install a different queue disc
Well, it is possible to install no queue disc, so that the additional queuing at
the traffic control layer is avoided. The current implementation of the flow
control (the upper layers are stopped when the device queue is full -- only for
the poit-to-point netdevice) should not affect that much (if at all) the
resulting behavior.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
File src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:36: *                 
                                                   -----------------
Fixed.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:38: * (2) PfifoFast,
CoDel, FqCoDel, SfqCoDel, etc.
Fixed.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:250: cmd.AddValue
("routerWanQueueType", "Router WAN net device queue type", routerWanQueueType);
On 2016/02/18 00:01:18, Tommaso Pecorella wrote:
> Mention the two allowed types.

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:360: }
On 2016/02/18 00:01:18, Tommaso Pecorella wrote:
> else ? Error ?
> Remember the drunk monkey test.

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:434:
outputConfig.ConfigureAttributes ();
The original examples had this stuff.

[Stefano Avallone, 2016-02-18 22:26:37]

On Thu, Feb 18, 2016 at 6:18 PM, <tomh.org@gmail.com> wrote:

> It seems to me that enhanced stopping behaviour could still be added
> later while suppressing failed Enqueue() (where the fail can be easily
> predicted) could be done now anyway.
>
> Part of Natale's patch was to provide Queue::Available(); could it not
> be used in QueueDisc::Transmit() to check (if IsStopped () == true ||
> Available () == 0 packets)?
>

If we consider that:

- not all the NetDevices use a Queue to buffer their packets
- the QueueDiscs do not know if a device queue is operating in byte mode or
packet mode

I think that a way to achieve what Tom suggests is to define a

bool NetDevice::HasAvailableSpace (uint32_t txq, uint32_t size)

where txq is the index of the device transmission queue and size is the
size of the packet that we are trying to enqueue.
The implementation of this method for the base class always returns true.
Subclasses supporting flow control (i.e., the PointToPointNetDevice,
currently) redefine this method so that it  returns false if there is not
enough space for a packet (of the given size, depending on the operation
mode). Maybe the same logic can be adopted for the new Queue method, i.e.

bool Queue::HasAvailableSpace (uint32_t size)

instead of

uint32_t Queue::Available (void)

so as to avoid that the caller has to check whether the queue is operating
in byte mode or packet mode.

This is the only solution I came up with. Opinions?

Cheers,
Stefano


> https://codereview.appspot.com/284530043/
>

[Tommaso Pecorella, 2016-02-19 00:51:14]

Here's the second (and last) batch of comments.

Cheers,

T.

https://codereview.appspot.com/284530043/diff/1/src/internet/helper/internet-...
File src/internet/helper/internet-stack-helper.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/helper/internet-...
src/internet/helper/internet-stack-helper.cc:500: }
A doubt: what happens if you install Internet and *after that* yo install the
NetDevices ?

Note. this is legal, and it works. IP automatically adds the proper interfaces
to the newly added NetDevices.
What about TC ?

https://codereview.appspot.com/284530043/diff/1/src/internet/model/arp-cache.cc
File src/internet/model/arp-cache.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/arp-cache....
src/internet/model/arp-cache.cc:420: ArpCache::Entry::MarkWaitReply
(PacketWithIpv4Header waiting)
Never mind.

I was referring to the checks on DequeuePending. Sadly they are necessary
because the return value is used to signal that the queue is empty.

A better alternative would be to avoid all this by having a proper iterator, but
it's something we can add in the future.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-inter...
File src/internet/model/ipv4-interface.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-inter...
src/internet/model/ipv4-interface.cc:210: Ptr<TrafficControlLayer> tc =
m_node->GetObject<TrafficControlLayer> ();
Isn't worth saving tc in a member variable ?
A GetObject for each Send is... slow.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-l3-pr...
File src/internet/model/ipv4-l3-protocol.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:388: tc->RegisterProtocolHandler
(MakeCallback (&ArpL3Protocol::Receive, PeekPointer (GetObject<ArpL3Protocol>
())),
Here it is. Pass it to the interface and have the interface memorize it. You'll
be able to use it later during the Send, avoiding a GetObject.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:684: Ipv4L3Protocol::copyPacketAndTrace
(const Ipv4Header & ipHeader, Ptr<Packet> packet,
CamelCase

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv6-inter...
File src/internet/model/ipv6-interface.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv6-inter...
src/internet/model/ipv6-interface.cc:384: Ptr<TrafficControlLayer> tc =
m_node->GetObject<TrafficControlLayer> ();
Same observation as for IPv4

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv6-inter...
src/internet/model/ipv6-interface.cc:386: NS_ASSERT (tc != 0);
Err... no.

The Loopback doesn't have tc, and it's legal. Move the assert *after* the
LoopbackNetDevice "if".
Note: do the same for IPv4

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ndisc-cach...
File src/internet/model/ndisc-cache.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ndisc-cach...
src/internet/model/ndisc-cache.cc:228: // m_waiting.remove (0);
Please restore the finite queue behaviour. I'm not sure if removing the comments
is enough, but the queue isn't infinite.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/helper/qu...
File src/traffic-control/helper/queue-disc-container.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/helper/qu...
src/traffic-control/helper/queue-disc-container.h:28: * \brief holds a vector of
ns3::QueueDisc pointers
Capitals (and punctuation at the end)

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/helper/tr...
File src/traffic-control/helper/traffic-control-helper.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/helper/tr...
src/traffic-control/helper/traffic-control-helper.h:36: * its components (packet
filters, internal queues, classes).
Please use \ingroup (check also the other classes)

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
File src/traffic-control/model/queue-disc.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.cc:358: QueueDisc::Drop (Ptr<QueueDiscItem>
item)
What does it happens if one sub-class goes nuts ?
I'd add an assert about packets and bytes being not-negative (err... beware, do
the check before the subtraction). I.e.:

NS_ASSERT_MSG (m_nPackets >= 1, "One of the sub-queues is gone fishing' mate.").
NS_ASSERT_MSG (m_nBytes >= item->GetPacketSize (), "One of the sub-queues is
gone fishing' mate.").

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.cc:381: }
Just a note: I guess there's no way to avoid this check, right ?
No, otherwise you'd have removed it.
Still, since QueueDisc (and its derivate) is an Object, why not use
Object::DoInitialize ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.cc:509: item = Dequeue ();
What happens if there are multiple Tx queues and they're all stopped ?
Answer: Dequeue will handle it and it will return a null item. Right ?

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
File src/traffic-control/model/queue-disc.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:41: * protocol number and the
transmission queue index,
Add the level of the protocol (L4, L3, etc.). Even better, an example.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:58: Address GetAddress (void) const;
Please add the "normal" description. I know it seems redundant, but if we use
just the \return element, the documentation looks strange.

As a side note, I think we have policy about doxygen elements capitalization and
punctuation.
Right now I don't think we have the time to triple check all, but I'd add it as
a todo (in your mind).

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:68: uint8_t GetTxq (void) const;
Mind to use a more descriptive name (also for the setter) ?
"Txq" doesn't really make me think to an index. GetTxqIndex sounds better, or
even GetTxQueueIndex

As a side note, I remember that in some place in the documentation is said that
TC is also for Rx queue (maybe not now, but it could).
As a consequence, I'd avoid the "Tx" in the name, it could be a poor choice in
the future.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:113: uint8_t m_txq;
Document these three please.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:149: Ptr<QueueDisc> m_queueDisc;       
//!< Queue disc attached to this class
This is made "public" by the Attribute MakePointerAccessor (as far as I know).
Suggestion: use an explicit Set.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:208: * Note that the number of packets
stored by the queue disc is updated as soon
If you don't have a \brief element, the first sentence will be the function's
Doxygen description.
... add a \brief.

[Tom Henderson, 2016-02-19 14:40:45]

On 2016/02/18 22:26:37, Stefano Avallone wrote:
> On Thu, Feb 18, 2016 at 6:18 PM, <mailto:tomh.org@gmail.com> wrote:
> 
> > It seems to me that enhanced stopping behaviour could still be added
> > later while suppressing failed Enqueue() (where the fail can be easily
> > predicted) could be done now anyway.
> >
> > Part of Natale's patch was to provide Queue::Available(); could it not
> > be used in QueueDisc::Transmit() to check (if IsStopped () == true ||
> > Available () == 0 packets)?
> >
> 
> If we consider that:
> 
> - not all the NetDevices use a Queue to buffer their packets
> - the QueueDiscs do not know if a device queue is operating in byte mode or
> packet mode
> 
> I think that a way to achieve what Tom suggests is to define a
> 
> bool NetDevice::HasAvailableSpace (uint32_t txq, uint32_t size)
> 
> where txq is the index of the device transmission queue and size is the
> size of the packet that we are trying to enqueue.
> The implementation of this method for the base class always returns true.
> Subclasses supporting flow control (i.e., the PointToPointNetDevice,
> currently) redefine this method so that it  returns false if there is not
> enough space for a packet (of the given size, depending on the operation
> mode). Maybe the same logic can be adopted for the new Queue method, i.e.
> 
> bool Queue::HasAvailableSpace (uint32_t size)
> 
> instead of
> 
> uint32_t Queue::Available (void)
> 
> so as to avoid that the caller has to check whether the queue is operating
> in byte mode or packet mode.
> 
> This is the only solution I came up with. Opinions?

I do not like bool Queue::HasAvailablespace () as much because I can't determine
the actual size I am working with unless I probe it with different sizes.

There is another solution that is less intrusive to NetDevice which is to
implement the helper method bool DeviceHasAvailableSpace(device, txq, size) that
does what you describe (checks mode) within the tc module somewhere (such as
could be private: member to class QueueDisc).

[Stefano Avallone, 2016-02-21 23:25:58]

https://codereview.appspot.com/284530043/diff/1/src/internet/helper/internet-...
File src/internet/helper/internet-stack-helper.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/helper/internet-...
src/internet/helper/internet-stack-helper.cc:500: }
Yes, this is something I noticed and didn't like. I have taken another approach
in the revised version that I will publish tomorrow (Monday) morning. I will
describe the details in the email I will send to ns-developers.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-inter...
File src/internet/model/ipv4-interface.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-inter...
src/internet/model/ipv4-interface.cc:210: Ptr<TrafficControlLayer> tc =
m_node->GetObject<TrafficControlLayer> ();
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> Isn't worth saving tc in a member variable ?
> A GetObject for each Send is... slow.

Done.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-l3-pr...
File src/internet/model/ipv4-l3-protocol.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:388: tc->RegisterProtocolHandler
(MakeCallback (&ArpL3Protocol::Receive, PeekPointer (GetObject<ArpL3Protocol>
())),
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> Here it is. Pass it to the interface and have the interface memorize it.
You'll
> be able to use it later during the Send, avoiding a GetObject.

Done.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:684: Ipv4L3Protocol::copyPacketAndTrace
(const Ipv4Header & ipHeader, Ptr<Packet> packet,
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> CamelCase

Done.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv6-inter...
File src/internet/model/ipv6-interface.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv6-inter...
src/internet/model/ipv6-interface.cc:384: Ptr<TrafficControlLayer> tc =
m_node->GetObject<TrafficControlLayer> ();
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> Same observation as for IPv4

Done.

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ipv6-inter...
src/internet/model/ipv6-interface.cc:386: NS_ASSERT (tc != 0);
Well, a tc object is aggregated to the node and it is not tied to a specific
device. So, even a node with just a loopback interface will have an aggregated
tc object. Anyway, I moved the assert after the LoopbackNetDevice "if".

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ndisc-cach...
File src/internet/model/ndisc-cache.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/internet/model/ndisc-cach...
src/internet/model/ndisc-cache.cc:228: // m_waiting.remove (0);
I think there was a bug: remove (0) removes from the list all the Ptr<Packet>
that are null, not the first (oldest) element in the list.

I replaced it with m_waiting.pop_front ()

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
File src/traffic-control/examples/pfifo-vs-red.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/examples/...
src/traffic-control/examples/pfifo-vs-red.cc:16: * Author: John Abraham
<john.abraham@gatech.edu>
On 2016/02/18 00:01:18, Tommaso Pecorella wrote:
> John ?
> Double check the copyrights (not just this one)

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/helper/qu...
File src/traffic-control/helper/queue-disc-container.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/helper/qu...
src/traffic-control/helper/queue-disc-container.h:28: * \brief holds a vector of
ns3::QueueDisc pointers
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> Capitals (and punctuation at the end)

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/helper/tr...
File src/traffic-control/helper/traffic-control-helper.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/helper/tr...
src/traffic-control/helper/traffic-control-helper.h:36: * its components (packet
filters, internal queues, classes).
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> Please use \ingroup (check also the other classes)

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
File src/traffic-control/model/queue-disc.cc (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.cc:358: QueueDisc::Drop (Ptr<QueueDiscItem>
item)
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> What does it happens if one sub-class goes nuts ?
> I'd add an assert about packets and bytes being not-negative (err... beware,
do
> the check before the subtraction). I.e.:
> 
> NS_ASSERT_MSG (m_nPackets >= 1, "One of the sub-queues is gone fishing'
mate.").
> NS_ASSERT_MSG (m_nBytes >= item->GetPacketSize (), "One of the sub-queues is
> gone fishing' mate.").

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.cc:381: }
Because queue discs are created one piece at a time: first, you create the root
queue disc, then you add the packet filters, then you add the internal queues
(possibly one at a time) or the classes (and then the attached queue discs...).
So you cannot check whether the configuration is correct just after creating the
root queue disc. We wait until the first packet is enqueued, at which point we
assume the configuration is completed and we can check it.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.cc:509: item = Dequeue ();
Correct. Also, if the Dequeue (actually, the DoDequeue of the subclass) does not
check whether the queues are stopped and returns a packet, this packet is sent
to Transmit, which checks the status of the device queue the packet is destined
to and, if stopped, re-queues the packet.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
File src/traffic-control/model/queue-disc.h (right):

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:41: * protocol number and the
transmission queue index,
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> Add the level of the protocol (L4, L3, etc.). Even better, an example.

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:58: Address GetAddress (void) const;
On 2016/02/19 00:51:14, Tommaso Pecorella wrote:
> Please add the "normal" description. I know it seems redundant, but if we use
> just the \return element, the documentation looks strange.
> 
> As a side note, I think we have policy about doxygen elements capitalization
and
> punctuation.
> Right now I don't think we have the time to triple check all, but I'd add it
as
> a todo (in your mind).

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:68: uint8_t GetTxq (void) const;
On 2016/02/19 00:51:14, Tommaso Pecorella wrote:
> Mind to use a more descriptive name (also for the setter) ?
> "Txq" doesn't really make me think to an index. GetTxqIndex sounds better, or
> even GetTxQueueIndex

Done.

> As a side note, I remember that in some place in the documentation is said
that
> TC is also for Rx queue (maybe not now, but it could).
> As a consequence, I'd avoid the "Tx" in the name, it could be a poor choice in
> the future.

Yes, the queue disc "ingress" is left for future work. I doubt this field will
be used to process incoming traffic, so I left the "Tx" in the name for the
moment.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:113: uint8_t m_txq;
On 2016/02/19 00:51:13, Tommaso Pecorella wrote:
> Document these three please.

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:149: Ptr<QueueDisc> m_queueDisc;       
//!< Queue disc attached to this class
On 2016/02/19 00:51:14, Tommaso Pecorella wrote:
> This is made "public" by the Attribute MakePointerAccessor (as far as I know).
> Suggestion: use an explicit Set.

Done.

https://codereview.appspot.com/284530043/diff/1/src/traffic-control/model/que...
src/traffic-control/model/queue-disc.h:208: * Note that the number of packets
stored by the queue disc is updated as soon
On 2016/02/19 00:51:14, Tommaso Pecorella wrote:
> If you don't have a \brief element, the first sentence will be the function's
> Doxygen description.
> ... add a \brief.

Done.

[Mohit P. Tahiliani, 2016-02-25 15:05:09]

Hi,

I found some places where we can use "queue disc" instead of "queue".

May be you can check once and if required, change them.

- Mohit

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
File src/traffic-control/examples/pfifo-vs-red.cc (right):

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
src/traffic-control/examples/pfifo-vs-red.cc:56: cmd.AddValue ("queueDiscType",
"Set Queue type to PfifoFast or RED", queueDiscType);
Would it be better to say "Set Queue disc type to ..." ?

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
src/traffic-control/examples/pfifo-vs-red.cc:59: cmd.AddValue ("modeBytes", "Set
Queue mode to Packets <0> or bytes <1>", modeBytes);
"Set Queue disc mode to ..."

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
src/traffic-control/examples/pfifo-vs-red.cc:67: NS_ABORT_MSG ("Invalid queue
type: Use --queueDiscType=RED or --queueDiscType=PfifoFast");
Here "Invalid queue disc type: ..."

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
src/traffic-control/examples/pfifo-vs-red.cc:176: NS_LOG_UNCOND
("----------------------------\nQueue Type:"
Here "Queue disc Type:"

[Stefano Avallone, 2016-02-25 17:59:34]

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
File src/traffic-control/examples/pfifo-vs-red.cc (right):

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
src/traffic-control/examples/pfifo-vs-red.cc:56: cmd.AddValue ("queueDiscType",
"Set Queue type to PfifoFast or RED", queueDiscType);
On 2016/02/25 15:05:09, Mohit P. Tahiliani wrote:
> Would it be better to say "Set Queue disc type to ..." ?

Done.

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
src/traffic-control/examples/pfifo-vs-red.cc:59: cmd.AddValue ("modeBytes", "Set
Queue mode to Packets <0> or bytes <1>", modeBytes);
On 2016/02/25 15:05:09, Mohit P. Tahiliani wrote:
> "Set Queue disc mode to ..."

Done.

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
src/traffic-control/examples/pfifo-vs-red.cc:67: NS_ABORT_MSG ("Invalid queue
type: Use --queueDiscType=RED or --queueDiscType=PfifoFast");
On 2016/02/25 15:05:09, Mohit P. Tahiliani wrote:
> Here "Invalid queue disc type: ..."

Done.

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/exam...
src/traffic-control/examples/pfifo-vs-red.cc:176: NS_LOG_UNCOND
("----------------------------\nQueue Type:"
On 2016/02/25 15:05:09, Mohit P. Tahiliani wrote:
> Here "Queue disc Type:"

Done.

[Mohit P. Tahiliani, 2016-03-03 04:53:45]

Hi,

I came across this doubt.

Can you please check?

Thanks and Regards,
Mohit P. Tahiliani

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/test...
File src/traffic-control/test/red-queue-disc-test-suite.cc (right):

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/test...
src/traffic-control/test/red-queue-disc-test-suite.cc:80: queue->SetTh (minTh *
pktSize, maxTh * pktSize);
Should this be

queue->SetTh (minTh * modeSize, maxTh * modeSize); ?

[Stefano Avallone, 2016-03-03 08:49:53]

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/test...
File src/traffic-control/test/red-queue-disc-test-suite.cc (right):

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/test...
src/traffic-control/test/red-queue-disc-test-suite.cc:80: queue->SetTh (minTh *
pktSize, maxTh * pktSize);
I think so, because you have to consider the IP header size, too.

[Stefano Avallone, 2016-03-03 08:59:34]

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/test...
File src/traffic-control/test/red-queue-disc-test-suite.cc (right):

https://codereview.appspot.com/284530043/diff/110001/src/traffic-control/test...
src/traffic-control/test/red-queue-disc-test-suite.cc:80: queue->SetTh (minTh *
pktSize, maxTh * pktSize);
Fixed on my queue-disc-ndq-aggr branch. Thanks!