PIE Queue Implementation in ns-3.24

This patch provides an implementation of Proportional Integral controller Enhanced (PIE) in ns-3.24 along with one example program and few tests.

PIE algorithm is described in the following paper:
https://www.ietf.org/mail-archive/web/iccrg/current/pdfB57AZSheOH.pdf

This implementation is a port of ns-2's PIE code.

Any suggestions/reviews would be much appreciated.

Thanks,
Shravya K. S.
Smriti Murali
Mohit P. Tahiliani

P.S.: Patch 2 works with traffic-control layer which was introduced from ns-3.25 onwards.

[Tommaso Pecorella, 2016-02-12 23:35:58]

Nice work, but I have found a strange thing (see my comments) that makes me
doubt that even the original code wasn't bug-free.

Mind to double check wit the paper also the rest of the code ?

Thanks,

T.

https://codereview.appspot.com/277610043/diff/1/src/network/doc/queue.rst
File src/network/doc/queue.rst (right):

https://codereview.appspot.com/277610043/diff/1/src/network/doc/queue.rst#new...
src/network/doc/queue.rst:79: The PIE model does not support DOCSIS mode.
This "limitation" is quite cryptic, mind to explain a bit better ?

https://codereview.appspot.com/277610043/diff/1/src/network/doc/queue.rst#new...
src/network/doc/queue.rst:89: "PIE: A Lightweight Control Scheme to Address the
Bufferbloat Problem"
Add a link to the paper if possible, or a DOI, or both :)

https://codereview.appspot.com/277610043/diff/1/src/network/examples/pie-test...
File src/network/examples/pie-tests.cc (right):

https://codereview.appspot.com/277610043/diff/1/src/network/examples/pie-test...
src/network/examples/pie-tests.cc:20: *          Virang vyas
<virangnitk@gmail.com>,
Shouldn't it be "Vyas" (capital V) ?

https://codereview.appspot.com/277610043/diff/1/src/network/examples/pie-test...
src/network/examples/pie-tests.cc:580: 
There's the very same comment made for ARED examples... if you want to use it
also as a test, you should return -1 if something isn't "right".

https://codereview.appspot.com/277610043/diff/1/src/network/test/pie-queue-te...
File src/network/test/pie-queue-test-suite.cc (right):

https://codereview.appspot.com/277610043/diff/1/src/network/test/pie-queue-te...
src/network/test/pie-queue-test-suite.cc:20: *          Virang vyas
<virangnitk@gmail.com>,
Copy-Paste, isn't it ?

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.cc
File src/network/utils/pie-queue.cc (right):

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:113: "Tupdate",
Describe a bit what Tupdate and Supdate is

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:123: "Queue limit in packets",
In packets or in bytes ? Clarify.
I suspect that when the queue is in bytes mode, the queue max size is m_qLim *
m_meanPktSize, but it's MY guess.

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:125: MakeUintegerAccessor (&PieQueue::m_qLim_),
Why the underscore at the end ?

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:130: MakeUintegerAccessor (&PieQueue::m_curq),
This doesn't really seems an attribute. Maybe a Trace value (and even in this
case, you should triple check its unit, packets or bytes ?)

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:144: DoubleValue (0.1),
0.1 of what ?
If it's a burst size, I'd expect a number of packets or a burst size in bytes.
In other terms, explain.

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:200: double PieQueue::GetQueueDelay (void)
Any specific reason to return the delay in seconds and not in Time ?

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:235: m_rtrsEvent = Simulator::Schedule
(m_sUpdate, &PieQueue::CalculateP, this);
Potential bug: cancel the old m_rtrsEvent if it's running. Otherwise you'll end
up with two events running.

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:257: InitializeParams ();
Why you do this here ?
Shouldn't it be done in the constructor AND in the reset function ?

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:265: if (QLen >= QLim)
This doesn't consider if the queue is in packet or bytes mode.
Please revise the code.

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:328: int earlyDrop = 1;
int ?
Avoid "int", use base types. Moreover, it seems a boolean.

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.c...
src/network/utils/pie-queue.cc:384: double tmp = now - m_dqStart;
tmp could be 0 in some cases, and later on it's used as a dividend.
Please triple check the algorithm.

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

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.h...
src/network/utils/pie-queue.h:56: *
Check with Tom Henderson if this is legal (I'm not an expert). I guess it is,
since it was in ns2.

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.h...
src/network/utils/pie-queue.h:86: * \brief A PIE packet queue
Expand the description, this is going to be in Doxygen.

https://codereview.appspot.com/277610043/diff/1/src/network/utils/pie-queue.h...
src/network/utils/pie-queue.h:196: * \param qlim queue limit
there's no 3rd param

[Stefano Avallone, 2016-04-27 17:24:24]

Hi Mohit,

are you willing to port this patch to the current traffic control infrastructure
and address the comments raised by Tommaso?

Thanks,
Stefano

[Mohit Tahiliani, 2016-04-27 17:37:30]

Hi Stefano,

Thanks for your mail.

Yes, we have included Tommaso's suggestions, and are planning to port it
now to work with traffic control layer.

I'll try to complete it by this weekend.

Regards,
Mohit P. Tahiliani
On 27-Apr-2016 22:54, <stavallo@gmail.com> wrote:

> Hi Mohit,
>
> are you willing to port this patch to the current traffic control
> infrastructure and address the comments raised by Tommaso?
>
> Thanks,
> Stefano
>
>
> https://codereview.appspot.com/277610043/
>

[Stefano Avallone, 2016-08-09 17:03:32]

Hi Mohit,

I just pushed PIE to ns-3-dev with some minor improvements (most notably, I
removed the LinkBandwidth and LinkDelay attributes, which are never used).

Bests,
Stefano


On 2016/04/27 17:37:30, Mohit Tahiliani wrote:
> Hi Stefano,
> 
> Thanks for your mail.
> 
> Yes, we have included Tommaso's suggestions, and are planning to port it
> now to work with traffic control layer.
> 
> I'll try to complete it by this weekend.
> 
> Regards,
> Mohit P. Tahiliani
> On 27-Apr-2016 22:54, <mailto:stavallo@gmail.com> wrote:
> 
> > Hi Mohit,
> >
> > are you willing to port this patch to the current traffic control
> > infrastructure and address the comments raised by Tommaso?
> >
> > Thanks,
> > Stefano
> >
> >
> > https://codereview.appspot.com/277610043/
> >