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

Issue 322140043: DCTCP Patch

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by Shravya
Modified:
1 month, 3 weeks ago
Reviewers:
Tom Henderson
CC:
ns-3-reviews_googlegroups.com, ns-3reviews_googlegroups.com
Visibility:
Public.

Description

Data Center TCP (DCTCP) [1] is a variant of TCP designed to work well in Data Center environments. It relies on Explicit Congestion Notification (ECN) to inform the sender about amount of congestion in the network. The sender then uses this information to reduce the congestion window proportionately. This patch provides an implementation of DCTCP in ns-3 along with documentation, test-suite and example program. It has been developed as a part of Google Summer of Code 2017 project [2]. Any suggestions / reviews would be much appreciated. Thanks, Shravya K. S. [1] Alizadeh, Mohammad, Albert Greenberg, David A. Maltz, Jitendra Padhye, Parveen Patel, Balaji Prabhakar, Sudipta Sengupta, and Murari Sridharan. "Data center tcp (dctcp)." In ACM SIGCOMM computer communication review, vol. 40, no. 4, pp. 63-74. ACM, 2010. [2] https://www.nsnam.org/wiki/GSOC2017Tcp

Patch Set 1 #

Total comments: 16

Patch Set 2 : DCTCP Patch 2 #

Patch Set 3 : Patch set 3: Taken on ns-3 dev with changeset 13050 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2845 lines, -106 lines) Patch
A examples/tcp/dctcp-example.cc View 1 1 chunk +266 lines, -0 lines 0 comments Download
M examples/tcp/wscript View 1 1 chunk +5 lines, -0 lines 0 comments Download
M src/internet/doc/tcp.rst View 1 5 chunks +213 lines, -3 lines 0 comments Download
M src/internet/model/tcp-congestion-ops.h View 3 chunks +32 lines, -4 lines 0 comments Download
M src/internet/model/tcp-congestion-ops.cc View 2 chunks +9 lines, -1 line 0 comments Download
A src/internet/model/tcp-dctcp.h View 1 1 chunk +155 lines, -0 lines 0 comments Download
A src/internet/model/tcp-dctcp.cc View 1 1 chunk +262 lines, -0 lines 0 comments Download
M src/internet/model/tcp-header.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/internet/model/tcp-socket-base.h View 1 2 11 chunks +86 lines, -4 lines 0 comments Download
M src/internet/model/tcp-socket-base.cc View 1 2 52 chunks +470 lines, -60 lines 0 comments Download
A src/internet/test/tcp-dctcp-test.cc View 1 1 chunk +687 lines, -0 lines 0 comments Download
A src/internet/test/tcp-ecn-test.cc View 1 2 1 chunk +567 lines, -0 lines 0 comments Download
M src/internet/test/tcp-general-test.h View 1 chunk +8 lines, -0 lines 0 comments Download
M src/internet/test/tcp-general-test.cc View 1 chunk +17 lines, -0 lines 0 comments Download
M src/internet/wscript View 1 2 3 chunks +4 lines, -0 lines 0 comments Download
M src/traffic-control/examples/adaptive-red-tests.cc View 1 2 9 chunks +37 lines, -13 lines 0 comments Download
M src/traffic-control/examples/wscript View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M src/traffic-control/test/examples-to-run.py View 1 2 1 chunk +24 lines, -20 lines 0 comments Download

Messages

Total messages: 3
Shravya
Data Center TCP (DCTCP) [1] is a variant of TCP designed to work well in ...
4 months ago (2017-06-21 17:18:46 UTC) #1
Tom Henderson
This looks good but I would recommend (besides addressing my inline comments): 1) the test ...
2 months, 2 weeks ago (2017-08-06 17:33:08 UTC) #2
Shravya
1 month, 3 weeks ago (2017-08-26 08:25:13 UTC) #3
Sir,

Thank you for your review. I have addressed all the queries and I have added a
new example in-line with the one mentioned in DCTCP SIGCOMM paper. Also, I have
added documentation for DCTCP.

https://codereview.appspot.com/322140043/diff/1/examples/tcp/dctcp-dumbbell-e...
File examples/tcp/dctcp-dumbbell-example.cc (right):

https://codereview.appspot.com/322140043/diff/1/examples/tcp/dctcp-dumbbell-e...
examples/tcp/dctcp-dumbbell-example.cc:31: */
On 2017/08/06 17:33:08, Tom Henderson wrote:
> I would like to see an example more in line with actual DCTCP deployment
> scenario in a data center, where RTTs are on orders of microseconds, not
> milliseconds.
> 
> For instance, the SIGCOMM DCTCP paper used a 10 Gbps, 100 us RTT bottleneck
> link; can this also be used in your canonical example?
> 
> Also, please add comments about what output data this example generates, and
> what it is supposed to be demonstrating (what is being plotted, and why?).


Yes Sir I have changed the example and the new example is in-line with the
example mentioned in the SIGCOMM DCTCP paper

https://codereview.appspot.com/322140043/diff/1/examples/tcp/dctcp-dumbbell-e...
examples/tcp/dctcp-dumbbell-example.cc:168: // 42 = headers size
On 2017/08/06 17:33:08, Tom Henderson wrote:
> why 42?  I would have expected more with TCP timestamp usage.

Yes I have corrected it in my new example

https://codereview.appspot.com/322140043/diff/1/examples/tcp/dctcp-dumbbell-e...
examples/tcp/dctcp-dumbbell-example.cc:178: Config::SetDefault
("ns3::RedQueueDisc::MinTh", DoubleValue (85));
On 2017/08/06 17:33:08, Tom Henderson wrote:
> Please add comments about where these values come from (85, 500, 1 ...)

Done.

https://codereview.appspot.com/322140043/diff/1/examples/tcp/dctcp-dumbbell-e...
examples/tcp/dctcp-dumbbell-example.cc:199: p2p.SetDeviceAttribute ("DataRate",
StringValue ("10Mbps"));
On 2017/08/06 17:33:08, Tom Henderson wrote:
> please also consider to update these NIC configurations, if bottleneck is
> upgraded to Gbps...

Done.

https://codereview.appspot.com/322140043/diff/1/src/internet/model/tcp-conges...
File src/internet/model/tcp-congestion-ops.h (right):

https://codereview.appspot.com/322140043/diff/1/src/internet/model/tcp-conges...
src/internet/model/tcp-congestion-ops.h:140: * \brief Trigger
events/calculations on occurance congestion window event
On 2017/08/06 17:33:08, Tom Henderson wrote:
> occurrence of
> 
> I think this may be Ankit's previous patch proposal?

Yes

https://codereview.appspot.com/322140043/diff/1/src/internet/model/tcp-dctcp.cc
File src/internet/model/tcp-dctcp.cc (right):

https://codereview.appspot.com/322140043/diff/1/src/internet/model/tcp-dctcp....
src/internet/model/tcp-dctcp.cc:46: .AddAttribute ("DctcpShiftG",
On 2017/08/06 17:33:08, Tom Henderson wrote:
> Why do these attribute names need to be prefixed by 'Dctcp'?  Can they be
> aligned with the paper or internet draft names?

Yes have corrected it

https://codereview.appspot.com/322140043/diff/1/src/internet/model/tcp-dctcp.h
File src/internet/model/tcp-dctcp.h (right):

https://codereview.appspot.com/322140043/diff/1/src/internet/model/tcp-dctcp....
src/internet/model/tcp-dctcp.h:32: * \brief An implementation of DCTCP
On 2017/08/06 17:33:08, Tom Henderson wrote:
> Please add more detail; what exactly is modelled (e.g. paper reference) and
what
> is not modelled (e.g. are there any missing parts of the implementation?)

Yes have added more detail about what exactly is modelled and what is yet to be
implemented

https://codereview.appspot.com/322140043/diff/1/src/internet/test/tcp-dctcp-t...
File src/internet/test/tcp-dctcp-test.cc (right):

https://codereview.appspot.com/322140043/diff/1/src/internet/test/tcp-dctcp-t...
src/internet/test/tcp-dctcp-test.cc:25: #include "../model/ipv6-end-point.h"
On 2017/08/06 17:33:08, Tom Henderson wrote:
> can this include path be changed to "ns3/ipv4-end-point.h"?

No Sir, it gave me an error message when I tried changing it to
"ns3/ipv4-end-point.h"
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted