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

Issue 316520043: TCP Westwood CRB

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by chirag.jamadagni
Modified:
4 months ago
Reviewers:
Mohit Tahiliani
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 44

Patch Set 2 : This patch contains the source code and test suite for TCP Westwood CRB. The tcp-variants-compariso… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+381 lines, -25 lines) Patch
M examples/tcp/tcp-variants-comparison.cc View 1 4 chunks +9 lines, -2 lines 0 comments Download
M src/internet/doc/tcp.rst View 1 1 chunk +11 lines, -6 lines 0 comments Download
M src/internet/model/tcp-westwood.h View 1 4 chunks +34 lines, -10 lines 0 comments Download
M src/internet/model/tcp-westwood.cc View 1 8 chunks +69 lines, -7 lines 0 comments Download
A src/internet/test/tcp-westwood-test.cc View 1 1 chunk +257 lines, -0 lines 0 comments Download
M src/internet/wscript View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 2
Mohit Tahiliani
Hi Chirag, Thanks for your contributions. Two major suggestions: 1. The test suite should contain ...
4 months ago (2017-04-15 07:36:47 UTC) #1
chirag.jamadagni
4 months ago (2017-04-15 14:22:34 UTC) #2
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
Sign in to reply to this message.

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