ns3 RED Request for Comments

[Mathieu Lacage, 2010-10-13 09:14:32]

looks nice. minor comments.

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc
File src/node/red-queue.cc (right):

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc#newcode47
src/node/red-queue.cc:47: .SetParent<Queue> ()
I don't know what the style checking script would do but, I would expect it to
not indent the calls to SetParent et al. by more than 2 spaces. Doing so would
make the code much more readable

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc#newcode52
src/node/red-queue.cc:52: MakeEnumAccessor (&RedQueue::SetMode),
There is a GetMode too.

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc#newcode360
src/node/red-queue.cc:360: if ( m_idleStart.GetNanoSeconds () != 0)
use IsZero instead

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc#newcode724
src/node/red-queue.cc:724: packetSize= p->GetSize ();
indent

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.h
File src/node/red-queue.h (right):

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.h#newcode61
src/node/red-queue.h:61: #ifndef RED_H
REQ_QUEUE_H

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.h#newcode71
src/node/red-queue.h:71: typedef std::vector< uint32_t> Uint32tVector;
move to be a member type of ReQueue:
class RedQueue ...{
private:
  typedef std::vector<uint32_t> Uint32tVector;
};

and then you can refer to it with RedQueue::Uint32tVector

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.h#newcode73
src/node/red-queue.h:73: struct RedStats
same as above. Probably should be named simply Stats so that you can refer to it
with RedEueue::Stats

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.h#newcode237
src/node/red-queue.h:237: #endif /* RED_H */
RED_QUEUE_H

[Tom Henderson, 2010-10-22 04:53:40]

Duy,
Please add ns-3-reviews@googlegroups.com to the cc list in this review issue.

My main comment is that more unit and flow-based tests are needed.  The existing
test just copies from DropTail and has nothing to do with RED.  

Have a look at this paper:
http://icir.org/floyd/red.html#ns

I think it would be a good goal to try to reproduce those tests for this
implementation, and document the tests in a tech report like Sally Floyd did. 
One thing that the data shows is a queue monitor with average and instantaneous
queue length.  

- Tom

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc
File src/node/red-queue.cc (right):

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc#newcode75
src/node/red-queue.cc:75: .AddAttribute ("m_qthMin",
again (see comment in the header file), I would try to align this name with what
is in the paper and make the member variable prefixed with "m_".  As per our
coding style, that probably would mean calling this attribute "MinTh" and the
variable m_minTh.

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc#newcode524
src/node/red-queue.cc:524: return m_rmask & rand ();
do not use rand()-- use a random number from an ns-3 RandomVariable.

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.cc#newcode637
src/node/red-queue.cc:637: //      m_qavg = AvgCalc (m_packets.size ());
this needs to be finished off

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.h
File src/node/red-queue.h (right):

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.h#newcode57
src/node/red-queue.h:57: * based on the Kuznetsov's implementation of Red in
Linux
I thought that this was implementation of the original RED from the paper, but
this comment suggests that it is taken partly from Linux?  Please clarify what
comes from where.

http://codereview.appspot.com/2405042/diff/1/src/node/red-queue.h#newcode215
src/node/red-queue.h:215: uint32_t m_qthMin; ///> Min avg length threshold
(bytes), should be < qthMax/2
To the extent possible, if you are copying algorithm from a paper, try to align
variable names.  For example, the original RED paper used min_th, but here you
have m_qthMin.