Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(425)

Issue 326150043: Implementation of Token Bucket Filter in ns3

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 8 months ago by Surya Seetharaman
Modified:
6 years, 6 months ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

Implementation of Token Bucket Filter in ns3

Patch Set 1 #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+1415 lines, -0 lines) Patch
A examples/traffic-control/tbf-example.cc View 1 chunk +156 lines, -0 lines 3 comments Download
M examples/traffic-control/wscript View 1 chunk +4 lines, -0 lines 0 comments Download
A src/traffic-control/doc/tbf.rst View 1 chunk +137 lines, -0 lines 4 comments Download
A src/traffic-control/model/tbf-queue-disc.h View 1 chunk +214 lines, -0 lines 1 comment Download
A src/traffic-control/model/tbf-queue-disc.cc View 1 chunk +442 lines, -0 lines 3 comments Download
A src/traffic-control/test/tbf-queue-disc-test-suite.cc View 1 chunk +459 lines, -0 lines 0 comments Download
M src/traffic-control/wscript View 3 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 2
Surya Seetharaman
This patch contains the implementation of the Token Bucket Filter QueueDisc in ns3 along with ...
6 years, 8 months ago (2017-07-28 17:28:56 UTC) #1
Tom Henderson
6 years, 6 months ago (2017-09-04 14:44:11 UTC) #2
https://codereview.appspot.com/326150043/diff/1/examples/traffic-control/tbf-...
File examples/traffic-control/tbf-example.cc (right):

https://codereview.appspot.com/326150043/diff/1/examples/traffic-control/tbf-...
examples/traffic-control/tbf-example.cc:37: // of 10000 packets.
Why such a large value?  It does not seem operationally realistic.

https://codereview.appspot.com/326150043/diff/1/examples/traffic-control/tbf-...
examples/traffic-control/tbf-example.cc:105: "QueueLimit", UintegerValue
(6553500),
Why are you using this unusual constant '65535000'?  Where does the value come
from?  Please document.

https://codereview.appspot.com/326150043/diff/1/examples/traffic-control/tbf-...
examples/traffic-control/tbf-example.cc:110: // Add the internal queue used by
Tbf
Please add a comment that this second statement is optional, only if user wants
to separately provide and configure the internal queue.

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/doc/tbf.rst
File src/traffic-control/doc/tbf.rst (right):

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/doc/tbf.r...
src/traffic-control/doc/tbf.rst:11: TBF is a simple classless qdisc that allows
controlling the bandwidth of the throughput according
output

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/doc/tbf.r...
src/traffic-control/doc/tbf.rst:22: 2. Data rate < Token rate : Very less number
of tokens are depleted.
s/Very less number of/Fewer

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/doc/tbf.r...
src/traffic-control/doc/tbf.rst:94: The key attributes that the CoDelQueue class
holds include the following:
s/CoDelQueue/TbfQueueDisc

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/doc/tbf.r...
src/traffic-control/doc/tbf.rst:102: 
Please add a section

TraceSources
============

to document them

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

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/model/tbf...
src/traffic-control/model/tbf-queue-disc.cc:70: UintegerValue (0),
Add a comment that the special value of zero, if used with a PeakRate of zero,
will disable the second bucket.

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/model/tbf...
src/traffic-control/model/tbf-queue-disc.cc:79: "Speed at which tokens enter the
second bucket in bps or Bps.",
Add a documentation string that the special value of data rate == 0 is
interpreted as infinite.

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/model/tbf...
src/traffic-control/model/tbf-queue-disc.cc:399: m_mtu = GetNetDevice ()->GetMtu
();
CheckConfig() should not perform queue configuration in general, because it is
called once at initialization-- try to make it 'check' but not 'configure'.

What happens if m_peakRate is set after object Initialize time?  Will the queue
work correctly?  Try to move code like this into the setter methods instead.

Or, you could consider to add CheckConfig() statement at the end of each setter
method; will that work?

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

https://codereview.appspot.com/326150043/diff/1/src/traffic-control/model/tbf...
src/traffic-control/model/tbf-queue-disc.h:141: * \brief Set the speed of the
tokens entering the first bucket in bps or Bps.
Please delete 'in bps or Bps' when referring to DataRate objects (here, and in
the methods below).

DataRate::GetBitRate() returns a rate in bps, but DataRate is an object
representing the data rate.

Also, globally, s/speed/rate
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b