Feng's Adaptive RED Implementation for ns-3

Feng's Adaptive RED [1] is variant of RED with a feature that it adapts maximum drop probability. 

This patch provides an implementation of Feng's Adaptive RED in ns-3 along with documentation, test cases and an example program.

This implementation is a port of ns-2 Feng's Adaptive RED model.

Your feedback would be highly appreciated.

Regards,
Sourabh Jain

[1] Feng, W. C., Kandlur, D. D., Saha, D., & Shin, K. G. (1999, March). A self-configuring RED gateway. In INFOCOM'99. Eighteenth Annual Joint Conference of the IEEE Computer and Communications Societies. Proceedings. IEEE (Vol. 3, pp. 1320-1328). IEEE.

[Stefano Avallone, 2017-01-27 14:29:50]

Inline are a couple of comments.

Also, I would like to ask you to move the example to examples/traffic-control
(due to dependency on point-to-point, internet and applications).

Thanks.

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

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:304: if (m_a != 3)
No need for this warning. 3 is the default value, so if the user changed it,
he/she should be aware of what he/she is doing.

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:633: m_status = Between;
m_status should be initialized in the InitializeParams () method

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:689: else if (m_isFengAdaptive)
maybe this should be checked first? i.e.,

if (m_isFengadaptive)
  {
    UpdateMaxPFeng (newAve);  // Update MaxP in MIMD fashion.
  }
else if (m_isAdaptMaxP && now > m_lastSet + m_interval)
  {
    UpdateMaxP(newAve);
  }

Otherwise, UpdateMaxPFeng might not be called always. Agree?

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

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.h:105: typedef enum
enum FengStatus would be more aligned to ns3 style

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.h:286: void UpdateMaxPFeng (double
newAve);
the description should state that this method updates maxP according to Feng's
algorithm. Currently, it is the same description as UpdateMaxP

[Sourabh Jain, 2017-01-31 13:28:33]

Thank you very much for the review and suggestions.

I have incorporated most of them in the modified patch (Patch Set 2).

For some suggestions, I've queries which are mentioned in line. Please let me
know your views on the same.

Thanks again.

Sourabh Jain

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

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:304: if (m_a != 3)
On 2017/01/27 14:29:50, Stefano Avallone wrote:
> No need for this warning. 3 is the default value, so if the user changed it,
> he/she should be aware of what he/she is doing.

I checked this with Mohit and he said that Tommaso once suggested (during ARED
code review) that we leave a warning message for the user in case it was done
unknowingly.

Please let me know what you think.

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:633: m_status = Between;
On 2017/01/27 14:29:50, Stefano Avallone wrote:
> m_status should be initialized in the InitializeParams () method

Done.

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:689: else if (m_isFengAdaptive)
On 2017/01/27 14:29:50, Stefano Avallone wrote:
> maybe this should be checked first? i.e.,
> 
> if (m_isFengadaptive)
>   {
>     UpdateMaxPFeng (newAve);  // Update MaxP in MIMD fashion.
>   }
> else if (m_isAdaptMaxP && now > m_lastSet + m_interval)
>   {
>     UpdateMaxP(newAve);
>   }
> 
> Otherwise, UpdateMaxPFeng might not be called always. Agree?

I think the current code is correct because UpdateMaxPFeng should be called only
when m_isFengAdaptive is set to true, never otherwise.

UpdateMaxPFeng and UpdateMaxP are implemented independently. 

Only one of them should be invoked depending on whether the user turned on ARED
or FengAdaptive (perhaps I should add some code to catch an exception if user
turns on both ARED and FengAdaptive?)

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

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.h:105: typedef enum
On 2017/01/27 14:29:50, Stefano Avallone wrote:
> enum FengStatus would be more aligned to ns3 style

Done.

https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.h:286: void UpdateMaxPFeng (double
newAve);
On 2017/01/27 14:29:50, Stefano Avallone wrote:
> the description should state that this method updates maxP according to Feng's
> algorithm. Currently, it is the same description as UpdateMaxP

Done.

[Sourabh Jain, 2017-03-11 10:55:31]

On 2017/01/31 13:28:33, Sourabh Jain wrote:
> Thank you very much for the review and suggestions.
> 
> I have incorporated most of them in the modified patch (Patch Set 2).
> 
> For some suggestions, I've queries which are mentioned in line. Please let me
> know your views on the same.
> 
> Thanks again.
> 
> Sourabh Jain
> 
>
https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
> File src/traffic-control/model/red-queue-disc.cc (right):
> 
>
https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
> src/traffic-control/model/red-queue-disc.cc:304: if (m_a != 3)
> On 2017/01/27 14:29:50, Stefano Avallone wrote:
> > No need for this warning. 3 is the default value, so if the user changed it,
> > he/she should be aware of what he/she is doing.
> 
> I checked this with Mohit and he said that Tommaso once suggested (during ARED
> code review) that we leave a warning message for the user in case it was done
> unknowingly.
> 
> Please let me know what you think.
> 
>
https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
> src/traffic-control/model/red-queue-disc.cc:633: m_status = Between;
> On 2017/01/27 14:29:50, Stefano Avallone wrote:
> > m_status should be initialized in the InitializeParams () method
> 
> Done.
> 
>
https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
> src/traffic-control/model/red-queue-disc.cc:689: else if (m_isFengAdaptive)
> On 2017/01/27 14:29:50, Stefano Avallone wrote:
> > maybe this should be checked first? i.e.,
> > 
> > if (m_isFengadaptive)
> >   {
> >     UpdateMaxPFeng (newAve);  // Update MaxP in MIMD fashion.
> >   }
> > else if (m_isAdaptMaxP && now > m_lastSet + m_interval)
> >   {
> >     UpdateMaxP(newAve);
> >   }
> > 
> > Otherwise, UpdateMaxPFeng might not be called always. Agree?
> 
> I think the current code is correct because UpdateMaxPFeng should be called
only
> when m_isFengAdaptive is set to true, never otherwise.
> 
> UpdateMaxPFeng and UpdateMaxP are implemented independently. 
> 
> Only one of them should be invoked depending on whether the user turned on
ARED
> or FengAdaptive (perhaps I should add some code to catch an exception if user
> turns on both ARED and FengAdaptive?)
> 
>
https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
> File src/traffic-control/model/red-queue-disc.h (right):
> 
>
https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
> src/traffic-control/model/red-queue-disc.h:105: typedef enum
> On 2017/01/27 14:29:50, Stefano Avallone wrote:
> > enum FengStatus would be more aligned to ns3 style
> 
> Done.
> 
>
https://codereview.appspot.com/319970043/diff/1/src/traffic-control/model/red...
> src/traffic-control/model/red-queue-disc.h:286: void UpdateMaxPFeng (double
> newAve);
> On 2017/01/27 14:29:50, Stefano Avallone wrote:
> > the description should state that this method updates maxP according to
Feng's
> > algorithm. Currently, it is the same description as UpdateMaxP
> 
> Done.

Hello Stefano,

I have uploaded a new patch (Patch 3) prepared on the latest ns-3-dev to account
for recent changes in the Queue class.

Thanks,
Sourabh Jain

[Stefano Avallone, 2017-03-15 08:39:23]

> I have uploaded a new patch (Patch 3) prepared on the latest ns-3-dev to
account
> for recent changes in the Queue class.
 
Thanks a lot. I will push this patch in time for ns-3.27.

Bests,
Stefano

[Sourabh Jain, 2017-03-16 05:35:58]

On 2017/03/15 08:39:23, Stefano Avallone wrote:
> > I have uploaded a new patch (Patch 3) prepared on the latest ns-3-dev to
> account
> > for recent changes in the Queue class.
>  
> Thanks a lot. I will push this patch in time for ns-3.27.
> 
> Bests,
> Stefano

Thanks for your support.

Regards,
Sourabh Jain

[Stefano Avallone, 2017-05-28 11:10:33]

Pushed to ns-3-dev with changeset 12890:e427aefe81f0.

Thanks!

PS Could you please close this code review?