TCP Westwood CRB

This patch contains the source code and test suite for TCP Westwood CRB. The tcp-variants-comparison.cc example has been modified to include this TCP variant.

[Mohit Tahiliani, 2017-04-15 07:36:47]

Hi Chirag,

Thanks for your contributions.

Two major suggestions:

1. The test suite should contain respective comments or description about each
test case.

2. tcp.rst file should mention the test cases developed to verify the
functionality of Westwood CRB.

Other suggestions are in line.

I'll do a second round of review in the next week.

Thanks,
Mohit P. Tahiliani

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

https://codereview.appspot.com/316520043/diff/1/examples/tcp/tcp-variants-com...
examples/tcp/tcp-variants-comparison.cc:228: "TcpBic, TcpYeah, TcpIllinois,
TcpWestwood, TcpWestwoodPlus, TcpWestwoodCRB, TcpLedbat ", transport_prot);
Should be TcpWestwoodCrb

https://codereview.appspot.com/316520043/diff/1/examples/tcp/tcp-variants-com...
examples/tcp/tcp-variants-comparison.cc:328: else if (transport_prot.compare
("TcpWestwoodCRB") == 0)
TcpWestwoodCrb

https://codereview.appspot.com/316520043/diff/1/examples/tcp/tcp-variants-com...
examples/tcp/tcp-variants-comparison.cc:440: || transport_prot.compare
("TcpWestwoodCRB") == 0
TcpWestwoodCrb

https://codereview.appspot.com/316520043/diff/1/src/internet/doc/tcp.rst
File src/internet/doc/tcp.rst (right):

https://codereview.appspot.com/316520043/diff/1/src/internet/doc/tcp.rst#newc...
src/internet/doc/tcp.rst:414: Westwood, WestwoodCRB and Westwood+ employ the
AIAD (Additive Increase/Adaptive Decrease)
Westwood CRB (Combined Rate and Bandwidth estimation)

https://codereview.appspot.com/316520043/diff/1/src/internet/doc/tcp.rst#newc...
src/internet/doc/tcp.rst:421: these two values to use in cwnd calculation, based
on whether the loss was random or congestion-based.
Lines 418 to 421 are ambiguous. Please make them clear.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
File src/internet/model/tcp-westwood.cc (right):

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:59: MakeEnumChecker(TcpWestwood::WESTWOOD,
"Westwood",TcpWestwood::WESTWOODPLUS, "WestwoodPlus",TcpWestwood::WESTWOODCRB,
"WestwoodCRB"))
One space before TcpWestwood::WESTWOODCRB

and

it should be WestwoodCrb instead of WestwoodCRB

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:74: m_tvalue (Seconds (0.4)),
m_tValue

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:108: 
Remove blank line

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:114: 
Remove both blank lines

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:156: {
Indent according to ns-3 standards

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:158: {
Indent according to ns-3 standards

Follow this wherever applicable

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:162: this, m_tvalue, tcb);
m_tValue

reflect this change at all other places also.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:163: }
trailing white space

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:164: EstimateBW(rtt,tcb);
trailing white spaces

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:166: 
Remove both blank lines

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:174: 
trailing white space

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:176: 
remove all trailing white spaces

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:189: double alpha = 0.9;
Give a reference for choosing 0.9

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:193: 
white spaces

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:197: double sample_ree = m_currentRE;
any reason for "ee" in "ree"?

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:202: 
blank line

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:219: }
Avoid changing existing code

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:254: // in TCPW-CRB, if the min_RTT is zero,
we return the BW estimate instead of the RE estimate
in Westwood CRB

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:255: if(m_minRtt.GetSeconds () !=0 &&
m_currentRE!=0 && (tcb->m_cWnd/(uint32_t (m_currentRE * static_cast<double>
(m_minRtt.GetSeconds ()))) < theta))
Give proper spaces, for example, it should be:

if (m_minRtt.GetSeconds () != 0 && m_currentRE != 0 && (tcb->m_cWnd / (uint32_t
(m_c
     urrentRE * static_cast<double> (m_minRtt.GetSeconds ()))) < theta))

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:257: return std::max (2*tcb->m_segmentSize,
Spaces before and after *

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:260: 
blank line

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.cc:263: return std::max (2*tcb->m_segmentSize,
Spaces before and after *

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
File src/internet/model/tcp-westwood.h (right):

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:50: * \brief An implementation of TCP
Westwood, WestwoodCRB and Westwood+.
Westwood CRB

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:52: * Westwood, WestwoodCRB and Westwood+
employ the AIAD (Additive Increase/Adaptive Decrease)
Westwood CRB

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:57: * Westwood+ samples the bandwidth every
RTT, and WestwoodCRB samples the bandwidth every RTT,
Westwood CRB

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:59: * these two values to use in cwnd
calculation, based on whether the loss was random or congestion-based.
These lines are same as mentioned in the .rst. So please make these clear as
well.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:67: * and the sampling interval (either last T
or last RTT for WestwoodCRB, based on loss type)
Westwood CRB

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:88: * \brief Protocol variant (Westwood,
Westwood+, WestwoodCRB)
Westwood CRB

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:131: 
Please remove the trailing white spaces.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:139: 
Please remove the trailing white spaces.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:140: 
Please remove the trailing white spaces.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:144: TracedValue<double>    m_currentRE;      
       //!< Current value of the estimated RE
Please remove the trailing white spaces.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:145: Time                   m_tvalue;         
       //!< T value used in Westwood CRB
Should be m_tValue

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:146: 
Please remove the trailing white spaces.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:149: 
Please remove the trailing white spaces.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:152: 
Please remove the trailing white spaces.

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:157: int                    m_ackedSinceT;    
       //!< The number of segments ACKed between T values in WestwoodCRB
Westwood CRB

https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
src/internet/model/tcp-westwood.h:160: EventId                m_reEstimateEvent;
       //!< The RE estimation event for WestwoodCRB
Westwood CRB

https://codereview.appspot.com/316520043/diff/1/src/internet/test/tcp-westwoo...
File src/internet/test/tcp-westwood-test.cc (right):

https://codereview.appspot.com/316520043/diff/1/src/internet/test/tcp-westwoo...
src/internet/test/tcp-westwood-test.cc:60: */
/**
 * \brief Execute the test
 */

[chirag.jamadagni, 2017-04-15 14:22:34]

On 2017/04/15 07:36:47, Mohit Tahiliani wrote:
> Hi Chirag,
> 
> Thanks for your contributions.
> 
> Two major suggestions:
> 
> 1. The test suite should contain respective comments or description about each
> test case.
> 
> 2. tcp.rst file should mention the test cases developed to verify the
> functionality of Westwood CRB.
> 
> Other suggestions are in line.
> 
> I'll do a second round of review in the next week.
> 
> Thanks,
> Mohit P. Tahiliani
> 
> 
https://codereview.appspot.com/316520043/diff/1/src/internet/doc/tcp.rst#newc...
> src/internet/doc/tcp.rst:414: Westwood, WestwoodCRB and Westwood+ employ the
> AIAD (Additive Increase/Adaptive Decrease)
> Westwood CRB (Combined Rate and Bandwidth estimation)
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/doc/tcp.rst#newc...
> src/internet/doc/tcp.rst:421: these two values to use in cwnd calculation,
based
> on whether the loss was random or congestion-based.
> Lines 418 to 421 are ambiguous. Please make them clear.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> File src/internet/model/tcp-westwood.cc (right):
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:59: MakeEnumChecker(TcpWestwood::WESTWOOD,
> "Westwood",TcpWestwood::WESTWOODPLUS, "WestwoodPlus",TcpWestwood::WESTWOODCRB,
> "WestwoodCRB"))
> One space before TcpWestwood::WESTWOODCRB
> 
> and
> 
> it should be WestwoodCrb instead of WestwoodCRB
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:74: m_tvalue (Seconds (0.4)),
> m_tValue
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:108: 
> Remove blank line
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:114: 
> Remove both blank lines
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:156: {
> Indent according to ns-3 standards
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:158: {
> Indent according to ns-3 standards
> 
> Follow this wherever applicable
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:162: this, m_tvalue, tcb);
> m_tValue
> 
> reflect this change at all other places also.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:163: }
> trailing white space
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:164: EstimateBW(rtt,tcb);
> trailing white spaces
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:166: 
> Remove both blank lines
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:174: 
> trailing white space
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:176: 
> remove all trailing white spaces
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:189: double alpha = 0.9;
> Give a reference for choosing 0.9
> 
> 
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.cc:263: return std::max (2*tcb->m_segmentSize,
> Spaces before and after *
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> File src/internet/model/tcp-westwood.h (right):
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:50: * \brief An implementation of TCP
> Westwood, WestwoodCRB and Westwood+.
> Westwood CRB
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:52: * Westwood, WestwoodCRB and Westwood+
> employ the AIAD (Additive Increase/Adaptive Decrease)
> Westwood CRB
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:57: * Westwood+ samples the bandwidth every
> RTT, and WestwoodCRB samples the bandwidth every RTT,
> Westwood CRB
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:59: * these two values to use in cwnd
> calculation, based on whether the loss was random or congestion-based.
> These lines are same as mentioned in the .rst. So please make these clear as
> well.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:67: * and the sampling interval (either last
T
> or last RTT for WestwoodCRB, based on loss type)
> Westwood CRB
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:88: * \brief Protocol variant (Westwood,
> Westwood+, WestwoodCRB)
> Westwood CRB
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:131: 
> Please remove the trailing white spaces.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:139: 
> Please remove the trailing white spaces.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:140: 
> Please remove the trailing white spaces.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:144: TracedValue<double>    m_currentRE;    
 
>        //!< Current value of the estimated RE
> Please remove the trailing white spaces.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:145: Time                   m_tvalue;       
 
>        //!< T value used in Westwood CRB
> Should be m_tValue
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:146: 
> Please remove the trailing white spaces.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:149: 
> Please remove the trailing white spaces.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:152: 
> Please remove the trailing white spaces.
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:157: int                    m_ackedSinceT;  
 
>        //!< The number of segments ACKed between T values in WestwoodCRB
> Westwood CRB
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/model/tcp-westwo...
> src/internet/model/tcp-westwood.h:160: EventId               
m_reEstimateEvent;
>        //!< The RE estimation event for WestwoodCRB
> Westwood CRB
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/test/tcp-westwoo...
> File src/internet/test/tcp-westwood-test.cc (right):
> 
>
https://codereview.appspot.com/316520043/diff/1/src/internet/test/tcp-westwoo...
> src/internet/test/tcp-westwood-test.cc:60: */
> /**
>  * \brief Execute the test
>  */

Hello Mohit Sir,

Thank you for the in-depth review!

I've completed all the suggested changes.

Could you please review the code once again?

Thanks,
Chirag Jamadagni