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

Issue 322140043: DCTCP Patch

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 4 weeks ago by Shravya
Modified:
1 week, 6 days ago
Reviewers:
Tom Henderson
CC:
ns-3-reviews_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: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+1938 lines, -57 lines) Patch
A examples/tcp/dctcp-dumbbell-example.cc View 1 chunk +273 lines, -0 lines 4 comments Download
M examples/tcp/wscript View 1 chunk +5 lines, -0 lines 0 comments Download
M src/internet/model/tcp-congestion-ops.h View 3 chunks +32 lines, -4 lines 1 comment 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 chunk +154 lines, -0 lines 1 comment Download
A src/internet/model/tcp-dctcp.cc View 1 chunk +262 lines, -0 lines 1 comment 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 11 chunks +87 lines, -4 lines 0 comments Download
M src/internet/model/tcp-socket-base.cc View 45 chunks +400 lines, -47 lines 0 comments Download
A src/internet/test/tcp-dctcp-test.cc View 1 chunk +687 lines, -0 lines 1 comment 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 3 chunks +3 lines, -0 lines 0 comments Download

Messages

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

1) the test suite doesn't exercise DCTCP over the lifetime of a realistic flow
in a data center; can you define a canonical flow and a way to measure that the
essential DCTCP properties (scalable cong. control) are accomplished?

2) missing .rst documentation

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: */
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?).

https://codereview.appspot.com/322140043/diff/1/examples/tcp/dctcp-dumbbell-e...
examples/tcp/dctcp-dumbbell-example.cc:168: // 42 = headers size
why 42?  I would have expected more with TCP timestamp usage.

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));
Please add comments about where these values come from (85, 500, 1 ...)

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

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
occurrence of

I think this may be Ankit's previous patch proposal?

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",
Why do these attribute names need to be prefixed by 'Dctcp'?  Can they be
aligned with the paper or internet draft names?

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
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?)

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"
can this include path be changed 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