Scalable TCP implementation

Scalable TCP implementation in |ns3|
------------------------------------------------

This chapter describes Scalable TCP implementation in |ns3|. This implementation is contained in the following files:

.. sourcecode:: text

    src/internet/model/tcp-scalable.{cc,h}

Model Description
*****************
Scalable improves TCP performance to better utilize the available bandwidth of a highspeed wide area network by altering NewReno congestion window adjustment
algorithm.  When congestion has not been detected, for each ACK received in an RTT, Scalable increases its cwnd per:
      cwnd = cwnd + 0.01                  (1)
Following Linux implementation of Scalable, we use 50 instead of 100 to account for delayed ACK.

On the first detection of congestion in a given RTT, cwnd is reduced based on the following equation
      cwnd = cwnd - ceil(0.125 * cwnd)       (2)

References
==========

.. [Kelly2003] Tom Kelly. 2003. Scalable TCP: improving performance in highspeed wide area networks. SIGCOMM Comput. Commun. Rev. 33, 2 (April 2003), 83-91. DOI=http://dx.doi.org/10.1145/956981.956989 

Validation
==========
The Scalable TCP model is tested using :cpp:class:`TcpScalableTestSuite` class defined in `src/internet/test/tcp-scalable-test.cc`.  This test suit can be run using the following commands:

::

  $ ./waf configure --enable-examples --enable-tests
  $ ./waf build
  $ ./test.py -s tcp-scalable-test
  
Example
=======
Scalable TCP can be simulated using the example `tcp-variants-comparison.cc` located in ``examples/tcp``.

[n.p, 2016-04-05 15:08:50]

Generally I like it, and I do not spot any critical issue. There are some minor
comments inline. Good job!

https://codereview.appspot.com/285540043/diff/20001/examples/tcp/tcp-variants...
File examples/tcp/tcp-variants-comparison.cc (right):

https://codereview.appspot.com/285540043/diff/20001/examples/tcp/tcp-variants...
examples/tcp/tcp-variants-comparison.cc:242: {
This would not apply to master ... since hybla and highspeed are already
included in the example

https://codereview.appspot.com/285540043/diff/20001/examples/tcp/tcp-variants...
examples/tcp/tcp-variants-comparison.cc:375: || transport_prot.compare
("TcpHybla") == 0
Yes I noticed there is a typo.. bic and cubic are included but not yet in the
internet module

https://codereview.appspot.com/285540043/diff/20001/src/internet/model/tcp-sc...
File src/internet/model/tcp-scalable.cc (right):

https://codereview.appspot.com/285540043/diff/20001/src/internet/model/tcp-sc...
src/internet/model/tcp-scalable.cc:106: m_ackCnt -= delta * w;
can m_ackCnt be simply 0 instead of doing this calculation ? Probably the
compiler will optimize this, but anyway it's for better readability.

https://codereview.appspot.com/285540043/diff/20001/src/internet/model/tcp-sc...
src/internet/model/tcp-scalable.cc:138: return ssThresh * tcb->m_segmentSize;
looks ok

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
File src/internet/test/tcp-scalable-test.cc (right):

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
src/internet/test/tcp-scalable-test.cc:78: uint32_t aiFactor = 50;
Get this factor from the default value of the instance, otherwise if in the
future we will change the default it will crash

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
src/internet/test/tcp-scalable-test.cc:169: } g_tcpScalableTest;
looks generally fine

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
File src/internet/test/tcp-scalable-test.h (right):

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
src/internet/test/tcp-scalable-test.h:11: #endif // TCPSCALABLETEST_H
This is not used inside the .cc , better to remove

[n.p, 2016-04-05 15:09:46]

On 2016/04/05 15:08:50, n.p wrote:
> Generally I like it, and I do not spot any critical issue. There are some
minor
> comments inline. Good job!

Ah, I forgot that would be nice to have the documentation that you put at the
beginning of this review inside some .rst

Nat

[trucanh524, 2016-04-09 22:18:45]

https://codereview.appspot.com/285540043/diff/20001/examples/tcp/tcp-variants...
File examples/tcp/tcp-variants-comparison.cc (right):

https://codereview.appspot.com/285540043/diff/20001/examples/tcp/tcp-variants...
examples/tcp/tcp-variants-comparison.cc:242: {
On 2016/04/05 15:08:49, n.p wrote:
> This would not apply to master ... since hybla and highspeed are already
> included in the example

Done.

https://codereview.appspot.com/285540043/diff/20001/examples/tcp/tcp-variants...
examples/tcp/tcp-variants-comparison.cc:375: || transport_prot.compare
("TcpHybla") == 0
On 2016/04/05 15:08:49, n.p wrote:
> Yes I noticed there is a typo.. bic and cubic are included but not yet in the
> internet module

Done.

https://codereview.appspot.com/285540043/diff/20001/src/internet/model/tcp-sc...
File src/internet/model/tcp-scalable.cc (right):

https://codereview.appspot.com/285540043/diff/20001/src/internet/model/tcp-sc...
src/internet/model/tcp-scalable.cc:106: m_ackCnt -= delta * w;
On 2016/04/05 15:08:49, n.p wrote:
> can m_ackCnt be simply 0 instead of doing this calculation ? 

Yes. No calculation needed. 

> Probably the
> compiler will optimize this, but anyway it's for better readability.

https://codereview.appspot.com/285540043/diff/20001/src/internet/model/tcp-sc...
src/internet/model/tcp-scalable.cc:138: return ssThresh * tcb->m_segmentSize;
On 2016/04/05 15:08:49, n.p wrote:
> looks ok

Done.

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
File src/internet/test/tcp-scalable-test.cc (right):

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
src/internet/test/tcp-scalable-test.cc:78: uint32_t aiFactor = 50;
On 2016/04/05 15:08:49, n.p wrote:
> Get this factor from the default value of the instance, otherwise if in the
> future we will change the default it will crash

Done.

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
src/internet/test/tcp-scalable-test.cc:169: } g_tcpScalableTest;
On 2016/04/05 15:08:49, n.p wrote:
> looks generally fine

Done.

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
File src/internet/test/tcp-scalable-test.h (right):

https://codereview.appspot.com/285540043/diff/20001/src/internet/test/tcp-sca...
src/internet/test/tcp-scalable-test.h:11: #endif // TCPSCALABLETEST_H
On 2016/04/05 15:08:50, n.p wrote:
> This is not used inside the .cc , better to remove

Done.

[trucanh524, 2016-04-09 22:28:35]

On 2016/04/05 15:09:46, n.p wrote:
> On 2016/04/05 15:08:50, n.p wrote:
> > Generally I like it, and I do not spot any critical issue. There are some
> minor
> > comments inline. Good job!
> 
> Ah, I forgot that would be nice to have the documentation that you put at the
> beginning of this review inside some .rst
> 
> Nat

I've submitted scalable.rst.  

Thank you for reviewing the code. 

Anh