DCTCP Patch

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

[Shravya, 2017-06-21 17:18:46]

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

[Tom Henderson, 2017-08-06 17:33:08]

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

[Shravya, 2017-08-26 08:25:13]

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"