Byte queue limits

Byte queue limits

[Pasquale Imputato, 2016-05-20 17:14:13]

This is a review request about the introduction of Byte queue limits (BQL) in
ns-3. This patch can also be viewed on github in [1].
Such as in Linux, BQL builds on top of Dynamic queue limits (DQL) library.  

Further information about DQL and BQL are in [2].

We would be happy to get feedbacks/comments about our implementation.

Thanks,

[1] https://github.com/pasquimp/ns-3-dev-git/commits/dql-new-rev
[2] https://lwn.net/Articles/454390/

[Tom Henderson, 2016-07-15 14:26:20]

Please see inline comments.  IMO, still needs 1) sphinx documentation (including
guidance on how users should tune the attributes), 2) a working example program
highlighting how it operates (perhaps hooking the Limit trace source), and 3)
test suite coverage that covers the core logic of the dynamic queue limit and
perhaps varies the attribute values to make sure they work as expected.

https://codereview.appspot.com/298220043/diff/1/src/network/utils/queue-limits.h
File src/network/utils/queue-limits.h (right):

https://codereview.appspot.com/298220043/diff/1/src/network/utils/queue-limit...
src/network/utils/queue-limits.h:29: class QueueLimits : public Object {
missing class doxygen.

Why is there a need for a base class?  There is one subclass with the same
public API.  What other subclasses are envisioned?  (if you decide to keep the
base class, mention in the class Doxygen what the overall design strategy is for
inheritance).

https://codereview.appspot.com/298220043/diff/1/src/network/utils/queue-limit...
src/network/utils/queue-limits.h:52: * \return the number of objects can be
queued
explain what return of negative integer means

s/number of objects can/number of objects that can/

https://codereview.appspot.com/298220043/diff/1/src/network/utils/queue-limit...
src/network/utils/queue-limits.h:54: virtual int32_t Avail () const = 0;
s/Avail/Available/

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
File src/network/model/net-device.h (right):

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
src/network/model/net-device.h:196: void SentQueue (uint32_t bytes);
SentToQueue ?

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
src/network/model/net-device.h:200: * \param pkts actual number of packets sent
over the medium
pkts is unused argument; remove?

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
src/network/model/net-device.h:203: void CompletedQueue (uint32_t pkts, uint32_t
bytes);
CompletedByQueue ?

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
src/network/model/net-device.h:208: void ResetQueue ();
ResetQueueLimits ?

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
File src/network/utils/dynamic-queue-limits.cc (right):

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
src/network/utils/dynamic-queue-limits.cc:60: MakeUintegerChecker<uint32_t> ())
should this check (0,DQL_MAX_LIMIT)?

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
src/network/utils/dynamic-queue-limits.cc:102: DynamicQueueLimits::Completed
(uint32_t count)
this method could use some NS_LOG_DEBUG embedded within the logic below.

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
File src/network/utils/dynamic-queue-limits.h (right):

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
src/network/utils/dynamic-queue-limits.h:29: #include "queue-limits.h"
queue-limits.{cc,h} are missing from this patchset

[Pasquale Imputato, 2016-07-27 14:43:14]

Here it is an updated version that addresses your comments.
Other notes:

- added the sphinx documentation with a guidance on how the users should tune
the attribute.

- added a benchmark-queue-disc example that uses two TCP flows with ping as in
flent tcp_bidirectional test.
The idea is that this example serves as a means to stress test all the queue
discs with BQL enabled or not.

Thanks for your comments.

[Pasquale Imputato, 2016-07-27 14:45:26]

https://codereview.appspot.com/298220043/diff/1/src/network/utils/queue-limits.h
File src/network/utils/queue-limits.h (right):

https://codereview.appspot.com/298220043/diff/1/src/network/utils/queue-limit...
src/network/utils/queue-limits.h:29: class QueueLimits : public Object {
We envision the introduction of other queue length controller for wifi case.
Added design strategy and class doxygen.

https://codereview.appspot.com/298220043/diff/1/src/network/utils/queue-limit...
src/network/utils/queue-limits.h:52: * \return the number of objects can be
queued
On 2016/07/15 14:26:20, Tom Henderson wrote:
> explain what return of negative integer means
> 
> s/number of objects can/number of objects that can/

Done.

https://codereview.appspot.com/298220043/diff/1/src/network/utils/queue-limit...
src/network/utils/queue-limits.h:54: virtual int32_t Avail () const = 0;
On 2016/07/15 14:26:20, Tom Henderson wrote:
> s/Avail/Available/

Done.

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
File src/network/model/net-device.h (right):

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
src/network/model/net-device.h:196: void SentQueue (uint32_t bytes);
Now we use NotifyQueuedBytes.

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
src/network/model/net-device.h:200: * \param pkts actual number of packets sent
over the medium
Removed.

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
src/network/model/net-device.h:203: void CompletedQueue (uint32_t pkts, uint32_t
bytes);
Now we use NotifyQueuedBytes

https://codereview.appspot.com/298220043/diff/80001/src/network/model/net-dev...
src/network/model/net-device.h:208: void ResetQueue ();
On 2016/07/15 14:26:20, Tom Henderson wrote:
> ResetQueueLimits ?

Done.

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
File src/network/utils/dynamic-queue-limits.cc (right):

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
src/network/utils/dynamic-queue-limits.cc:60: MakeUintegerChecker<uint32_t> ())
On 2016/07/15 14:26:20, Tom Henderson wrote:
> should this check (0,DQL_MAX_LIMIT)?

Done.

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
src/network/utils/dynamic-queue-limits.cc:102: DynamicQueueLimits::Completed
(uint32_t count)
On 2016/07/15 14:26:20, Tom Henderson wrote:
> this method could use some NS_LOG_DEBUG embedded within the logic below.

Done.

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
File src/network/utils/dynamic-queue-limits.h (right):

https://codereview.appspot.com/298220043/diff/80001/src/network/utils/dynamic...
src/network/utils/dynamic-queue-limits.h:29: #include "queue-limits.h"
You are right. I used an incorrect commit id in the calculation of the patch.

[Tom Henderson, 2016-08-01 04:51:56]

This version looks ready to merge.

https://codereview.appspot.com/298220043/diff/180001/src/network/utils/queue-...
File src/network/utils/queue-limits.h (right):

https://codereview.appspot.com/298220043/diff/180001/src/network/utils/queue-...
src/network/utils/queue-limits.h:41: * For more details, see the queue limits
model page.
see the queue limits Sphinx documentation.

https://codereview.appspot.com/298220043/diff/180001/src/network/utils/queue-...
src/network/utils/queue-limits.h:60: * \brief Record number of completed objects
and recalculate the limit
I was a bit confused by the use of the term 'objects' in many places here and in
the Sphinx documentation, instead of 'bytes'.  Why not say 'bytes'?

[Pasquale Imputato, 2016-08-16 08:03:50]

Your comments addressed in the latest version.

Thanks for review

https://codereview.appspot.com/298220043/diff/180001/src/network/utils/queue-...
File src/network/utils/queue-limits.h (right):

https://codereview.appspot.com/298220043/diff/180001/src/network/utils/queue-...
src/network/utils/queue-limits.h:41: * For more details, see the queue limits
model page.
On 2016/08/01 04:51:56, Tom Henderson wrote:
> see the queue limits Sphinx documentation.

Done.

https://codereview.appspot.com/298220043/diff/180001/src/network/utils/queue-...
src/network/utils/queue-limits.h:60: * \brief Record number of completed objects
and recalculate the limit
On 2016/08/01 04:51:56, Tom Henderson wrote:
> I was a bit confused by the use of the term 'objects' in many places here and
in
> the Sphinx documentation, instead of 'bytes'.  Why not say 'bytes'?

You are right. Comes from Linux. Now we use bytes.