Re: TCP Cubic for ns-3 (issue 87740043)

[tomh...@gmail.com]

Looks like a good start on this implementation.

With additional TCP congestion control variants coming, we ought to
refactor congestion control behavior from the rest of the TCP socket
behavior, possibly similar to how RttEstimator is factored out.


https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc
File src/internet/model/tcp-cubic.cc (right):

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode48
src/internet/model/tcp-cubic.cc:48: __val > __max ? __max : __val; })
This is copied or adapted from Linux kernel, in which case the license
from that file should also appear. But I do not generally support this
type of logic done in macros until/unless profiles show it is a
bottleneck, and then there are perhaps other ways to do it. The other
reason to possibly copy this kind of macro into ns-3 is if it is central
to the code in some way (like a pervasive BSD list) but this is not the
case here.

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode57
src/internet/model/tcp-cubic.cc:57: #define HYSTART_DELAY_MAX
MilliSeconds (1000)
All of these magic numbers probably would be better off exposed as
attributes.

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode58
src/internet/model/tcp-cubic.cc:58: #define HYSTART_DELAY_THRESH(x)
clamp (x, HYSTART_DELAY_MIN, HYSTART_DELAY_MAX)
All of these magic numbers are probably better off exposed as
attributes.

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode73
src/internet/model/tcp-cubic.cc:73: MakeIntegerChecker <int> ())
If we turn 1024 to an attribute, this should be turned into a fraction
so as to avoid attributes depending on one another (although it seems
that integer arithmetic is preferred).

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode226
src/internet/model/tcp-cubic.cc:226: TcpCubic::bictcpUpdate ()
BicTcpUpdate

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode264
src/internet/model/tcp-cubic.cc:264: delta = 0.4 * std::pow (offs, 3);
expose constant as attribute?

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode293
src/internet/model/tcp-cubic.cc:293: m_cnt = 20; /* When no losses are
detected, grow up fast */
magic numbers (20)

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode323
src/internet/model/tcp-cubic.cc:323: m_cWndCnt = 0;
consider NS_LOG_DEBUG statements when state is changing.

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode358
src/internet/model/tcp-cubic.cc:358: if (m_hystart && m_cWnd.Get () <=
m_ssThresh && m_cWnd.Get () >= (uint32_t) m_hystartLowWindow)
please use c++ style casts

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode415
src/internet/model/tcp-cubic.cc:415: TcpCubic::bictcpRecalcSsthresh ()
BicTcpRecalcSsthresh

https://codereview.appspot.com/87740043/diff/40001/src/internet/model/tcp-cubic.cc#newcode449
src/internet/model/tcp-cubic.cc:449: m_cWnd = 1U * m_segmentSize;
should this constant be an attribute?

https://codereview.appspot.com/87740043/