New TCP architecture for NS-3

TcpSocketBase, a replacement for TcpSocketImpl, is proposed. This is doing the basic function of a TCP socket, such as the mechanics of its state machine and the sliding window. The new TCP architecture is modular and extensible. Two extensions are provided as examples: TcpNewReno and TcpRFC793. For more detail, please read http://www.nsnam.org/wiki/index.php/New_TCP_Socket_Architecture

[Adrian, 2010-06-16 19:00:35]

http://codereview.appspot.com/1702042/diff/1/2
File src/internet-stack/tcp-header.cc (left):

http://codereview.appspot.com/1702042/diff/1/2#oldcode23
src/internet-stack/tcp-header.cc:23: #include "tcp-socket-impl.h"
This include is indeed redundant. Thus I remove it.

http://codereview.appspot.com/1702042/diff/1/3
File src/internet-stack/tcp-l4-protocol.cc (right):

http://codereview.appspot.com/1702042/diff/1/3#newcode339
src/internet-stack/tcp-l4-protocol.cc:339: MakeTypeIdChecker ())
Taking the advice from tomh, a better design would be use TypeId in attributes
instead of using an entire object. Thus the "get default" function at the top is
removed, and the pluggable RttEstimator is now as a TypeIdValue. Similarly for
the pluggable TCP version.

http://codereview.appspot.com/1702042/diff/1/3#newcode428
src/internet-stack/tcp-l4-protocol.cc:428: Ptr<TcpSocketBase> socket =
socketFactory.Create<TcpSocketBase> ();
Now the object factory is created in realtime based on the TypeId specified as
attribute.

http://codereview.appspot.com/1702042/diff/1/6
File src/internet-stack/tcp-newreno.h (right):

http://codereview.appspot.com/1702042/diff/1/6#newcode36
src/internet-stack/tcp-newreno.h:36: class TcpNewReno : public TcpSocketBase
Example of extending TcpSocketBase to provide new TCP variant.

http://codereview.appspot.com/1702042/diff/1/8
File src/internet-stack/tcp-rfc793.h (right):

http://codereview.appspot.com/1702042/diff/1/8#newcode39
src/internet-stack/tcp-rfc793.h:39: class TcpRfc793 : public TcpSocketBase
Example of extending TcpSocketBase to provide a new TCP variant.

[Josh Pelkey, 2010-06-23 17:30:23]

Here is my initial review.  Overall, I really like it.  A few small comments on
coding style.  I recommend running check-style.py on these files,
see:http://www.nsnam.org/doxygen/group___check_style.html
Also need to remove those vim commands at the top (i'm a vim user too :)

My major concern is the correctness of new-reno code, specifically when in the
fast recovery phase.  I don't see where cwnd is modified.

We also need to address the various TODOs and XXX pieces of code, including the
#ifs that you have there to make the tests pass.

http://codereview.appspot.com/1702042/diff/1/3
File src/internet-stack/tcp-l4-protocol.cc (left):

http://codereview.appspot.com/1702042/diff/1/3#oldcode41
src/internet-stack/tcp-l4-protocol.cc:41: 
should the state machine be removed?

http://codereview.appspot.com/1702042/diff/1/5
File src/internet-stack/tcp-newreno.cc (right):

http://codereview.appspot.com/1702042/diff/1/5#newcode1
src/internet-stack/tcp-newreno.cc:1: /* vim:set cin cino=>4n-2f0{2^-2 sw=2
syn=cpp ru nu lcs=tab\:>- lbr ts=8 et: */
will have to take out these vim commands

http://codereview.appspot.com/1702042/diff/1/5#newcode31
src/internet-stack/tcp-newreno.cc:31: using namespace std;
don't generally see this in ns-3

http://codereview.appspot.com/1702042/diff/1/5#newcode83
src/internet-stack/tcp-newreno.cc:83: uint32_t  TcpNewReno::Window ()
minor coding style, new line needed after return type (and the others like it
below)...i'm sure this was probably not followed in the old tcp-socket-impl
file.  its probably worth running check-style.py on this code: see
http://www.nsnam.org/doxygen/group___check_style.html

http://codereview.appspot.com/1702042/diff/1/5#newcode108
src/internet-stack/tcp-newreno.cc:108: // Congestion avoidance mode, adjust by
(ackBytes*segSize) / cWnd XXX: How can you get ackBytes?
need to change something here?

http://codereview.appspot.com/1702042/diff/1/5#newcode118
src/internet-stack/tcp-newreno.cc:118: void TcpNewReno::DupAck (const TcpHeader&
t, uint32_t count)
shouldn't newreno inflate cwnd if in fast recovery?

http://codereview.appspot.com/1702042/diff/1/5#newcode132
src/internet-stack/tcp-newreno.cc:132: // For Tahoe, we also reset nextTxSeq
tahoe?

http://codereview.appspot.com/1702042/diff/1/7
File src/internet-stack/tcp-rfc793.cc (right):

http://codereview.appspot.com/1702042/diff/1/7#newcode27
src/internet-stack/tcp-rfc793.cc:27: using namespace std;
don't generally see this in ns-3

http://codereview.appspot.com/1702042/diff/1/9
File src/internet-stack/tcp-rx-buffer.cc (right):

http://codereview.appspot.com/1702042/diff/1/9#newcode3
src/internet-stack/tcp-rx-buffer.cc:3: //
* instead of //

http://codereview.appspot.com/1702042/diff/1/10
File src/internet-stack/tcp-rx-buffer.h (right):

http://codereview.appspot.com/1702042/diff/1/10#newcode3
src/internet-stack/tcp-rx-buffer.h:3: //
* instead of //

http://codereview.appspot.com/1702042/diff/1/11
File src/internet-stack/tcp-socket-base.cc (right):

http://codereview.appspot.com/1702042/diff/1/11#newcode580
src/internet-stack/tcp-socket-base.cc:580: // C.f.: tcp_rcv_state_process() in
tcp_input.c in Linux kernel
i like this in place of the state machine

http://codereview.appspot.com/1702042/diff/1/11#newcode800
src/internet-stack/tcp-socket-base.cc:800: #if 0
thank you for figuring this out and explaining on the wiki...in the end we will
need to make a decision and remove these ifdefs

http://codereview.appspot.com/1702042/diff/1/11#newcode822
src/internet-stack/tcp-socket-base.cc:822: // TODO: Need to check the ACK seq ==
SYN seq
is this something we need to do before merging?

http://codereview.appspot.com/1702042/diff/1/11#newcode887
src/internet-stack/tcp-socket-base.cc:887: // arrived.
this sounds similar to the recent issue that was resolved in tcp-socket-impl:
http://code.nsnam.org/ns-3-dev/rev/f03f4d82daaf

http://codereview.appspot.com/1702042/diff/1/13
File src/internet-stack/tcp-tx-buffer.cc (right):

http://codereview.appspot.com/1702042/diff/1/13#newcode3
src/internet-stack/tcp-tx-buffer.cc:3: //
* instead of //

http://codereview.appspot.com/1702042/diff/1/14
File src/internet-stack/tcp-tx-buffer.h (right):

http://codereview.appspot.com/1702042/diff/1/14#newcode3
src/internet-stack/tcp-tx-buffer.h:3: //
* instead of //

[Adrian, 2010-06-24 06:55:31]

I am uploading Patch Set 2. This is the update of the previous one. In summary,
two things are done: (1) coding style fix, and removed inaccurate or unnecessary
comments; (2) check against out-of-sequence FIN and SYN+ACK. I will send Patch
Set 3 in a day or two after I verify the NewReno implementation with the RFC.

http://codereview.appspot.com/1702042/diff/1/3
File src/internet-stack/tcp-l4-protocol.cc (left):

http://codereview.appspot.com/1702042/diff/1/3#oldcode41
src/internet-stack/tcp-l4-protocol.cc:41: 
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> should the state machine be removed?

In the update, I removed tcp-typedefs.h dependence. The only thing needed from
it is the names of TCP states. This enum set is now moved to tcp-l4-protocol.h

http://codereview.appspot.com/1702042/diff/1/5
File src/internet-stack/tcp-newreno.cc (right):

http://codereview.appspot.com/1702042/diff/1/5#newcode1
src/internet-stack/tcp-newreno.cc:1: /* vim:set cin cino=>4n-2f0{2^-2 sw=2
syn=cpp ru nu lcs=tab\:>- lbr ts=8 et: */
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> will have to take out these vim commands

Done.

http://codereview.appspot.com/1702042/diff/1/5#newcode31
src/internet-stack/tcp-newreno.cc:31: using namespace std;
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> don't generally see this in ns-3

Done.

http://codereview.appspot.com/1702042/diff/1/5#newcode83
src/internet-stack/tcp-newreno.cc:83: uint32_t  TcpNewReno::Window ()
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> minor coding style, new line needed after return type (and the others like it
> below)...i'm sure this was probably not followed in the old tcp-socket-impl
> file.  its probably worth running check-style.py on this code: see
> http://www.nsnam.org/doxygen/group___check_style.html

Done.

http://codereview.appspot.com/1702042/diff/1/7
File src/internet-stack/tcp-rfc793.cc (right):

http://codereview.appspot.com/1702042/diff/1/7#newcode27
src/internet-stack/tcp-rfc793.cc:27: using namespace std;
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> don't generally see this in ns-3

Done.

http://codereview.appspot.com/1702042/diff/1/9
File src/internet-stack/tcp-rx-buffer.cc (right):

http://codereview.appspot.com/1702042/diff/1/9#newcode3
src/internet-stack/tcp-rx-buffer.cc:3: //
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> * instead of //

Done.

http://codereview.appspot.com/1702042/diff/1/10
File src/internet-stack/tcp-rx-buffer.h (right):

http://codereview.appspot.com/1702042/diff/1/10#newcode3
src/internet-stack/tcp-rx-buffer.h:3: //
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> * instead of //

Done.

http://codereview.appspot.com/1702042/diff/1/11
File src/internet-stack/tcp-socket-base.cc (right):

http://codereview.appspot.com/1702042/diff/1/11#newcode822
src/internet-stack/tcp-socket-base.cc:822: // TODO: Need to check the ACK seq ==
SYN seq
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> is this something we need to do before merging?

Done.

http://codereview.appspot.com/1702042/diff/1/11#newcode887
src/internet-stack/tcp-socket-base.cc:887: // arrived.
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> this sounds similar to the recent issue that was resolved in tcp-socket-impl:
> http://code.nsnam.org/ns-3-dev/rev/f03f4d82daaf

Done.

http://codereview.appspot.com/1702042/diff/1/13
File src/internet-stack/tcp-tx-buffer.cc (right):

http://codereview.appspot.com/1702042/diff/1/13#newcode3
src/internet-stack/tcp-tx-buffer.cc:3: //
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> * instead of //

Done.

http://codereview.appspot.com/1702042/diff/1/14
File src/internet-stack/tcp-tx-buffer.h (right):

http://codereview.appspot.com/1702042/diff/1/14#newcode3
src/internet-stack/tcp-tx-buffer.h:3: //
On 2010/06/23 17:30:23, Josh Pelkey wrote:
> * instead of //

Done.

[Adrian, 2010-06-25 08:46:10]

I uploaded Patch sets 3 and 4. This includes four TCP variants, namely, DoD
(RFC793), Tahoe (partly RFC2001), Reno (RFC2581), and NewReno (RFC2582). I
commented on the code for explanations. The only thing that might further change
is the genealogy of NewReno. Maybe we can make it a direct descendent of
TcpSocketBase, I am not sure.

This is not yet compatible to the Linux pluggable congestion control
architecture but very close. Make it align to Linux is the work for next
release.

Now I need someone to test it out for me: Try to reproduce the figures in the
1996 CCR paper by K. Fall and S. Floyd to see if I am really doing
NewReno/Reno/Tahoe correctly.

http://codereview.appspot.com/1702042/diff/10002/14010
File src/internet-stack/tcp-socket-base.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14010#newcode340
src/internet-stack/tcp-socket-base.cc:340: }
This is removed in patch set 3: Initialization is best done when three-way
handshake completed

http://codereview.appspot.com/1702042/diff/16/16005
File src/internet-stack/tcp-newreno.h (right):

http://codereview.appspot.com/1702042/diff/16/16005#newcode36
src/internet-stack/tcp-newreno.h:36: class TcpNewReno : public TcpReno
Roll of the genealogy of TcpNewReno, son of Object, son of Socket.

Object fathered Socket, Socket fathered TcpSocket and his brothers.

TcpSocket fathered TcpSocketBase, TcpSocketBase fathered TcpTahoe, TcpTahoe
fathered TcpReno, TcpReno fathered TcpNewReno, who is called the default TCP
socket in TcpL4Protocol.

http://codereview.appspot.com/1702042/diff/16/16012
File src/internet-stack/tcp-socket-base.cc (right):

http://codereview.appspot.com/1702042/diff/16/16012#newcode653
src/internet-stack/tcp-socket-base.cc:653: ReceivedData (packet, tcpHeader);
Function name change: NewRx->ReceivedData

http://codereview.appspot.com/1702042/diff/16/16012#newcode815
src/internet-stack/tcp-socket-base.cc:815: ReceivedAck (packet, tcpHeader);
This was NewAck but now replaced with ReceivedAck as we may need to handle the
piggybacked data. For the same reason, m_delAckCount must set before calling
ReceivedAck to avoid delayed ACK

http://codereview.appspot.com/1702042/diff/16/16012#newcode976
src/internet-stack/tcp-socket-base.cc:976: m_rxBuffer.SetFinSeq
(tcpHeader.GetSequenceNumber () + SequenceNumber (p->GetSize ()));
In case FIN piggybacked with data, the correct FIN sequence number is
SeqNum+DataSize

http://codereview.appspot.com/1702042/diff/16/16012#newcode1391
src/internet-stack/tcp-socket-base.cc:1391: Retransmit ();
Put the variant-specific behaviour in Retransmit() so that it can be overloaded

http://codereview.appspot.com/1702042/diff/16/16012#newcode1445
src/internet-stack/tcp-socket-base.cc:1445: }
This is the baseline behaviour (as of RFC793, except m_rtt line). In case of
Reno, fast recovery is done.

http://codereview.appspot.com/1702042/diff/16/16012#newcode1449
src/internet-stack/tcp-socket-base.cc:1449: {
DoRetransmit blindly re-send the packet at m_nextTxSequence

http://codereview.appspot.com/1702042/diff/16/16013
File src/internet-stack/tcp-socket-base.h (right):

http://codereview.appspot.com/1702042/diff/16/16013#newcode55
src/internet-stack/tcp-socket-base.h:55: TIME_WAIT,   // 10
Change TIMED_WAIT to TIME_WAIT to match the state names in RFC

http://codereview.appspot.com/1702042/diff/16/16013#newcode138
src/internet-stack/tcp-socket-base.h:138: bool SendPendingData (bool withAck =
false); // Send as much as the window allows
For clarity reason, rearranged the order of function declarations in header
file.

http://codereview.appspot.com/1702042/diff/16/16014
File src/internet-stack/tcp-tahoe.cc (right):

http://codereview.appspot.com/1702042/diff/16/16014#newcode122
src/internet-stack/tcp-tahoe.cc:122: Retransmit ();
This should be DoRetransmit(), fix in Patch set 4

http://codereview.appspot.com/1702042/diff/16/16014#newcode141
src/internet-stack/tcp-tahoe.cc:141: Retransmit ();                           
// Retransmit the packet
In fact, I am not sure about Tahoe. I implemented RFC2001 up to sec.3 but omits
fast recovery algorithm. This is my best understanding according to Fall & Floyd
(1996) paper in CCR.

[Tom Henderson, 2010-06-26 00:54:33]

Please see comments against previous patchset although I think most or all still
apply.

I think this patchset improves our TCP model and support merging it in the next
week if it passes existing tests (even if the test output has to slightly align)
and there are no known major issues.  It should fix bug 824 and also provide
improved modeling of persist behavior.  The API impact to users should be
minimal to none.  There is a small change in API for selecting RttEstimatorType
that should not affect users since only one type is available (the new API for
this attribute is improved since it is TypeId-based).  There has been user
demand for this feature (more modern TCP congestion control).

We will need to document how users can select the different variants.  There are
two settings to deal with now.  InternetStackHelper::SetTcp() will control
whether TcpL4Protocol or NscTcpL4Protocol is used. Then, the attribute
TcpSocketType within TcpL4Protocol will govern the specific socket congestion
variant.

Suggested example program:  use a point-to-point link with a Receive Error Loss
model that induces a basic drop pattern that NewReno is designed to handle well,
and show how setting the attribute to the various settings will cause different
behavior.

Future work is to figure out how to easily add the currently popular Linux
variants (e.g. BIC, CUBIC) and to add SACK.

http://codereview.appspot.com/1702042/diff/10002/14004
File src/internet-stack/tcp-newreno.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14004#newcode109
src/internet-stack/tcp-newreno.cc:109: { // Congestion avoidance mode, adjust by
(ackBytes*segSize) / cWnd XXX: How can you get ackBytes?
XXX to resolve here.  Would the proposal to align to Linux congestion_ops
resolve this?

http://codereview.appspot.com/1702042/diff/10002/14004#newcode112
src/internet-stack/tcp-newreno.cc:112: m_cWnd += (uint32_t) adder;
we typically use C++ static_cast instead of C-style

http://codereview.appspot.com/1702042/diff/10002/14006
File src/internet-stack/tcp-rfc793.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14006#newcode76
src/internet-stack/tcp-rfc793.cc:76: {
These no-op setters might be candidate for NS_LOG_WARN() that the socket does
not implement them.

http://codereview.appspot.com/1702042/diff/10002/14008
File src/internet-stack/tcp-rx-buffer.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14008#newcode31
src/internet-stack/tcp-rx-buffer.cc:31: : nextRxSeq(n), gotFin(false), size(0),
maxBuffer(32768), availBytes(0)
similar maxBuffer comment (and should be m_maxBuffer)

http://codereview.appspot.com/1702042/diff/10002/14008#newcode36
src/internet-stack/tcp-rx-buffer.cc:36: {
are there no references needing to be released?

http://codereview.appspot.com/1702042/diff/10002/14008#newcode64
src/internet-stack/tcp-rx-buffer.cc:64: {
please check for NS_LOG_FUNCTION() calls throughout your code, for function
tracing

http://codereview.appspot.com/1702042/diff/10002/14010
File src/internet-stack/tcp-socket-base.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14010#newcode58
src/internet-stack/tcp-socket-base.cc:58: "Persist timeout to probe for rwin",
This is a candidate to move to TcpSocket class, since it is generic to all TCPs.
 Would then require some implementation support in NscTcp.

http://codereview.appspot.com/1702042/diff/10002/14010#newcode143
src/internet-stack/tcp-socket-base.cc:143: * Bind we allocate an Ipv4Endpoint. 
Immediately thereafter we always do
thank you for documenting this

http://codereview.appspot.com/1702042/diff/10002/14010#newcode329
src/internet-stack/tcp-socket-base.cc:329: /* From Socket class: Send a packet.
Parameter flags is not used. Packet has no
suggest you consider to add NS_ABORT_IF_MSG (flags, "flags unsupported") or
NS_LOG_WARN() in the below, to alert user that flags is not used.

http://codereview.appspot.com/1702042/diff/10002/14010#newcode375
src/internet-stack/tcp-socket-base.cc:375: NS_LOG_FUNCTION (this);
similar comment about flags here

http://codereview.appspot.com/1702042/diff/10002/14010#newcode639
src/internet-stack/tcp-socket-base.cc:639: m_state = SYN_RCVD;
this looks suspicious (ESTABLISHED->SYN_RCVD)

http://codereview.appspot.com/1702042/diff/10002/14010#newcode671
src/internet-stack/tcp-socket-base.cc:671: { // this must be an invalid flag,
send reset
NS_LOG_LOGIC...

http://codereview.appspot.com/1702042/diff/10002/14010#newcode807
src/internet-stack/tcp-socket-base.cc:807: m_delAckCount = m_delAckMaxCount;
can you fix this to be what you think is correct, or what NSC behavior is?

http://codereview.appspot.com/1702042/diff/10002/14010#newcode823
src/internet-stack/tcp-socket-base.cc:823: m_delAckCount = m_delAckMaxCount;
same comment

http://codereview.appspot.com/1702042/diff/10002/14010#newcode896
src/internet-stack/tcp-socket-base.cc:896: {
NS_LOG_LOGIC...

http://codereview.appspot.com/1702042/diff/10002/14012
File src/internet-stack/tcp-tx-buffer.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14012#newcode34
src/internet-stack/tcp-tx-buffer.cc:34: : firstByteSeq(n), size (0),
maxBuffer(32768), data (0), unused(true)
32768 is magic number in member initialization code-- suggest attribute here, or
if you do not want to make it into an ns3::Object at this time, at least use a
defined integer constant.  In Linux, this is much higher (8388608), is 32K wise
here?  What is the tradeoff of making it much larger?

http://codereview.appspot.com/1702042/diff/10002/14013
File src/internet-stack/tcp-tx-buffer.h (right):

http://codereview.appspot.com/1702042/diff/10002/14013#newcode44
src/internet-stack/tcp-tx-buffer.h:44: SequenceNumber const& HeadSeq () const {
return firstByteSeq; }
suggest "HeadSequence"

Note, in ns-3 we typically avoid implementation in header files unless inlining.

http://codereview.appspot.com/1702042/diff/10002/14013#newcode51
src/internet-stack/tcp-tx-buffer.h:51: * Append a data packet to the end of the
buffer
document this return value

http://codereview.appspot.com/1702042/diff/10002/14013#newcode56
src/internet-stack/tcp-tx-buffer.h:56: * Inquire available data from the
sequence number
Doxygen here needs more clarification-- is this the total number of bytes behind
the sequence number seq (including or excluding the byte numbered seq)?

I realize these method names are inherited from the existing implementation but
feel free to make more descriptive (SizeFromSequence) and improve the doxygen.

http://codereview.appspot.com/1702042/diff/10002/14013#newcode68
src/internet-stack/tcp-tx-buffer.h:68: * any data.
I suggest to split this into two methods:
void AssignInitialSequence (const SequenceNumber& seq); // only used initially,
an error to call twice
void FreeToSequence (const SequenceNumber& seq); // allow implementation to free
data in front of seq.

Note, I don't see why client needs a bool return value to know whether data was
actually freed or a pointer was moved?

http://codereview.appspot.com/1702042/diff/10002/14013#newcode79
src/internet-stack/tcp-tx-buffer.h:79: uint32_t size;                 //< Number
of data bytes
coding style:  member variables are prefixed with "m_"

[Adrian, 2010-06-28 21:14:51]

Patch set 5 uploaded. With various minor fixes (also detailed below). I feel
good about this version of code. But there are something to agree on before
commit:

1. Attributes. Currently an attribute is set in TcpSocketBase, I want to move it
to TcpSocket.
2. State transitions. The old NS-3 behavior of TcpSocketImpl is preserved in
TcpSocketBase. But some state transition is wrong. For example, ESTABLISHED
state can go back to SENT_RCVD state if SYN packet is received. I want to fix
this to align with the state machine in RFC793.

These two issues are not done in this patch set but if no body objects this, I
will do so and publish patch set 6 in the coming days.

http://codereview.appspot.com/1702042/diff/10002/14004
File src/internet-stack/tcp-newreno.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14004#newcode109
src/internet-stack/tcp-newreno.cc:109: { // Congestion avoidance mode, adjust by
(ackBytes*segSize) / cWnd XXX: How can you get ackBytes?
I checked the Linux code, unfortunately, no. Linux avoid this problem by keeping
the window size in units of MSS. Linux has an option to keep accurate byte count
so that cwnd increases by one MSS only if the accumulated byte count > cwnd. But
this is not the default behaviour because of the performance reason.

On 2010/06/26 00:54:33, Tom Henderson wrote:
> XXX to resolve here.  Would the proposal to align to Linux congestion_ops
> resolve this?

http://codereview.appspot.com/1702042/diff/10002/14004#newcode112
src/internet-stack/tcp-newreno.cc:112: m_cWnd += (uint32_t) adder;
On 2010/06/26 00:54:33, Tom Henderson wrote:
> we typically use C++ static_cast instead of C-style

Done.

http://codereview.appspot.com/1702042/diff/10002/14008
File src/internet-stack/tcp-rx-buffer.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14008#newcode36
src/internet-stack/tcp-rx-buffer.cc:36: {
On 2010/06/26 00:54:33, Tom Henderson wrote:
> are there no references needing to be released?

I don't think so. The data are in buffer are held as smart pointers stored in
the STL container (Rx buffer is map, Tx buffer is list). If the STL container is
destroyed by class destructor, the smart pointer should be able to handle the
memory release.

http://codereview.appspot.com/1702042/diff/10002/14008#newcode64
src/internet-stack/tcp-rx-buffer.cc:64: {
On 2010/06/26 00:54:33, Tom Henderson wrote:
> please check for NS_LOG_FUNCTION() calls throughout your code, for function
> tracing

Done.

http://codereview.appspot.com/1702042/diff/10002/14010
File src/internet-stack/tcp-socket-base.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14010#newcode58
src/internet-stack/tcp-socket-base.cc:58: "Persist timeout to probe for rwin",
On 2010/06/26 00:54:33, Tom Henderson wrote:
> This is a candidate to move to TcpSocket class, since it is generic to all
TCPs.
>  Would then require some implementation support in NscTcp.

Agree. But I would rather left it here at this moment to avoid touching the NSC.

http://codereview.appspot.com/1702042/diff/10002/14010#newcode329
src/internet-stack/tcp-socket-base.cc:329: /* From Socket class: Send a packet.
Parameter flags is not used. Packet has no
On 2010/06/26 00:54:33, Tom Henderson wrote:
> suggest you consider to add NS_ABORT_IF_MSG (flags, "flags unsupported") or
> NS_LOG_WARN() in the below, to alert user that flags is not used.

Done.

http://codereview.appspot.com/1702042/diff/10002/14010#newcode375
src/internet-stack/tcp-socket-base.cc:375: NS_LOG_FUNCTION (this);
On 2010/06/26 00:54:33, Tom Henderson wrote:
> similar comment about flags here

Done.

http://codereview.appspot.com/1702042/diff/10002/14010#newcode639
src/internet-stack/tcp-socket-base.cc:639: m_state = SYN_RCVD;
On 2010/06/26 00:54:33, Tom Henderson wrote:
> this looks suspicious (ESTABLISHED->SYN_RCVD)

Well, this is the behaviour of old NS-3. I don't think this is a legal state
transition, though. If someone second me, I would propose to make this illegal
(ignore SYN).

http://codereview.appspot.com/1702042/diff/10002/14010#newcode671
src/internet-stack/tcp-socket-base.cc:671: { // this must be an invalid flag,
send reset
On 2010/06/26 00:54:33, Tom Henderson wrote:
> NS_LOG_LOGIC...

Done.

http://codereview.appspot.com/1702042/diff/10002/14010#newcode807
src/internet-stack/tcp-socket-base.cc:807: m_delAckCount = m_delAckMaxCount;
On 2010/06/26 00:54:33, Tom Henderson wrote:
> can you fix this to be what you think is correct, or what NSC behavior is?

I removed "#if 0" in patch set 5

http://codereview.appspot.com/1702042/diff/10002/14010#newcode823
src/internet-stack/tcp-socket-base.cc:823: m_delAckCount = m_delAckMaxCount;
On 2010/06/26 00:54:33, Tom Henderson wrote:
> same comment

Done.

http://codereview.appspot.com/1702042/diff/10002/14010#newcode896
src/internet-stack/tcp-socket-base.cc:896: {
On 2010/06/26 00:54:33, Tom Henderson wrote:
> NS_LOG_LOGIC...

Done.

http://codereview.appspot.com/1702042/diff/10002/14012
File src/internet-stack/tcp-tx-buffer.cc (right):

http://codereview.appspot.com/1702042/diff/10002/14012#newcode34
src/internet-stack/tcp-tx-buffer.cc:34: : firstByteSeq(n), size (0),
maxBuffer(32768), data (0), unused(true)
On 2010/06/26 00:54:33, Tom Henderson wrote:
> 32768 is magic number in member initialization code-- suggest attribute here,
or
> if you do not want to make it into an ns3::Object at this time, at least use a
> defined integer constant.  In Linux, this is much higher (8388608), is 32K
wise
> here?  What is the tradeoff of making it much larger?

This is a bit confusing here. In a normal use, a user is allocating a Tx buffer
by way of TcpSocket, which it has attributes for Tx and Rx window size
(SndBufSize, RcvBufSize) with default both to 128KiByte. Because of the
attribute system, whoever use TcpTxBuffer normally must called
TcpSocketBase::SetSndBufSize(size) and in turn called
TcpTxBuffer::SetMaxBufferSize(size). Therefore the initial value of maxBuffer
here is insignificant. It is here simply for a safety reason.

http://codereview.appspot.com/1702042/diff/10002/14013
File src/internet-stack/tcp-tx-buffer.h (right):

http://codereview.appspot.com/1702042/diff/10002/14013#newcode44
src/internet-stack/tcp-tx-buffer.h:44: SequenceNumber const& HeadSeq () const {
return firstByteSeq; }
On 2010/06/26 00:54:33, Tom Henderson wrote:
> suggest "HeadSequence"
> 
> Note, in ns-3 we typically avoid implementation in header files unless
inlining.
>  

Done.

http://codereview.appspot.com/1702042/diff/10002/14013#newcode51
src/internet-stack/tcp-tx-buffer.h:51: * Append a data packet to the end of the
buffer
On 2010/06/26 00:54:33, Tom Henderson wrote:
> document this return value

Done.

http://codereview.appspot.com/1702042/diff/10002/14013#newcode56
src/internet-stack/tcp-tx-buffer.h:56: * Inquire available data from the
sequence number
On 2010/06/26 00:54:33, Tom Henderson wrote:
> Doxygen here needs more clarification-- is this the total number of bytes
behind
> the sequence number seq (including or excluding the byte numbered seq)?
> 
> I realize these method names are inherited from the existing implementation
but
> feel free to make more descriptive (SizeFromSequence) and improve the doxygen.
 

Done.

http://codereview.appspot.com/1702042/diff/10002/14013#newcode68
src/internet-stack/tcp-tx-buffer.h:68: * any data.
On 2010/06/26 00:54:33, Tom Henderson wrote:
> I suggest to split this into two methods:
> void AssignInitialSequence (const SequenceNumber& seq); // only used
initially,
> an error to call twice
> void FreeToSequence (const SequenceNumber& seq); // allow implementation to
free
> data in front of seq.
> 
> Note, I don't see why client needs a bool return value to know whether data
was
> actually freed or a pointer was moved?
> 

I took your advice to split this into SetHeadSequence(seq) and DiscardUpTo(seq).
And modified the relevant lines in TcpSocketBase. It makes the code easier to
read now. See the case in Patch set 5.

http://codereview.appspot.com/1702042/diff/10002/14013#newcode79
src/internet-stack/tcp-tx-buffer.h:79: uint32_t size;                 //< Number
of data bytes
On 2010/06/26 00:54:33, Tom Henderson wrote:
> coding style:  member variables are prefixed with "m_"

Done.

[Adrian, 2010-07-02 18:06:44]

I uploaded the latest update (patch set 6, recommended commit) on the TCP
architecture.

Personally, I recommend this version to be merged into the main branch of NS-3.
There are a few points to note:

1. The behaviour of this TCP and the old architecture is differed in two places:
Firstly, the initial data packet is always ACKed but the old architecture
doesn't. This would make the time values in packet trace vastly different.
Secondly, the final FIN packet, if piggybacked with data, only one ACK is
submitted instead of two.

2. After merging, all existing classes derived from TcpSocket has to be updated
because a new attribute for persist timeout is created and the relevant get/set
functions are declared as pure virtual in TcpSocket. The NscTcpSocketImpl is
updated accordingly. In case there are any user-created classes, similar change
have to be done.

3. NSC part is not changed. If excluding NSC, the following files can be
removed: tcp-typedef.h, tcp-socket-impl.cc, tcp-socket-impl.h

4. From a user's point of view, using the new TCP socket needs **no** change to
existing code. You are still calling the TcpSocketFactory as before, and
invoking the Send and Recv function as before.

[Adrian, 2010-07-30 01:45:35]

Patch set 12 is uploaded. This is a patch against ns-3-dev changeset
6470:cd04d218ba62 (most recent one as of this moment). I verified the TCP in my
best extent. The result is documented in
http://www.nsnam.org/wiki/index.php/New_TCP_Socket_Architecture

I checked this patch set on Mac OS X 10.6.4 (Intel) platform. It compiles and
runs smoothly. The test program may be removed when committed to the main source
tree.

If there is still any problems, please let me know.

[Tom Henderson, 2010-08-27 23:23:36]

I really like your wiki page and tests/examples.

I had a question about your One Loss results, which shows Reno performing the
best.  In this case, you explain as follows:

"The above three figures, are showing the behaviour of Tahoe, Reno and NewReno
respectively. The lost is causing duplicated acknowledgements of sequence number
8001 arrived at around 1.1s. In Tahoe, the congestion window is immediately
reduced to 1 MSS upon triple duplicated acknowledgement. In Reno, however, the
cwnd is reduced to only 7500 bytes, which is half of the old cwnd size plus 3
MSS. The subsequent received ACK raises the cwnd by one MSS until a full ACK is
received, which causing it operate in congestion avoidance thereafter. In
NewReno, the cwnd operation makes the TCP less bursty upon full ACK. Its cwnd
increases as in Reno after triple duplicated acknowledgement (from time 1.1s to
1.26s). Once a full ACK is received (around 1.26s), the cwnd is reset to 4500
bytes, which is half of the cwnd value when the triple dupack occured."

Now, in RFC 3782, ssthresh is set to max (FlightSize/2, 2*MSS) and then when
leaving fast recovery, cwnd is set to ssthresh.  So, I would expect that the
reason that cwnd is reset to 4500 bytes is because FlightSize (and not cwnd) was
9000 bytes when Fast Retransmit occurred.

Also, I would check why Reno and NewReno do not give identical results here.  Is
Reno based on RFC2581?  I think the behavior in this loss case should be the
same.

From a presentation standpoint, it would be nice to fix the graph axes to be the
same for all plots, to highlight differences, but this is just a suggestion if
you have time.

I would recommend to make regression test cases for your example program.  I
think this is close to merging, otherwise.

[Adrian, 2010-08-28 03:33:46]

Hi Tom,

> Now, in RFC 3782, ssthresh is set to max (FlightSize/2, 2*MSS) and then
> when leaving fast recovery, cwnd is set to ssthresh.  So, I would expect
> that the reason that cwnd is reset to 4500 bytes is because FlightSize
> (and not cwnd) was 9000 bytes when Fast Retransmit occurred.

I implemented Reno on RFC2581 and NewReno on RFC2582 (unfortunately,
Tahoe doesn't have an RFC). I totally forget about RFC3782! (I
finished college before it was published, so my old lecture notes
didn't base on that) I will read it again and upload a new patchset if
there shall be any change.

> From a presentation standpoint, it would be nice to fix the graph axes
> to be the same for all plots, to highlight differences, but this is just
> a suggestion if you have time.

That's no big deal. I kept all the script. I just need modification
and re-run. Let me do the graph-plotting again after I make the
corresponding RFC3782 change.

> I would recommend to make regression test cases for your example
> program.  I think this is close to merging, otherwise.

Regression is good. I didn't make any regression before. Probably I
need some time to learn from existing examples or I need someone to
guide me through.

Regards,
Adrian.

[Tom Henderson, 2010-08-28 16:23:12]

On 2010/08/28 03:33:46, Adrian wrote:
> Hi Tom,
> 
> > Now, in RFC 3782, ssthresh is set to max (FlightSize/2, 2*MSS) and then
> > when leaving fast recovery, cwnd is set to ssthresh.  So, I would
expect
> > that the reason that cwnd is reset to 4500 bytes is because FlightSize
> > (and not cwnd) was 9000 bytes when Fast Retransmit occurred.
> 
> I implemented Reno on RFC2581 and NewReno on RFC2582 (unfortunately,
> Tahoe doesn't have an RFC). I totally forget about RFC3782! (I
> finished college before it was published, so my old lecture notes
> didn't base on that) I will read it again and upload a new patchset if
> there shall be any change.

I am not too concerned about the difference between RFC2582 and 3782; the
differences should not manifest themselves in the tests that you ran.  I just
mentioned it since it is the most recent RFC.

However, I expect that the one loss response should be the same for Reno and
NewReno, and I looked at your code and it seemed like it should behave the same,
so I am wondering why you are getting different results.

> 
> Regression is good. I didn't make any regression before. Probably I
> need some time to learn from existing examples or I need someone to
> guide me through.

I think Josh can help here.

[Josh Pelkey, 2010-08-28 19:29:57]

Adrian,

Thank you very much for your work on TCP.  It is really great!  After reading
through your wiki and code, I only have one comment/suggestion.  I think it is a
little confusing that NewReno inherits from Reno and Reno inherits from Tahoe
and Tahoe inherits from TcpSocketBase.  I would have expected that all TCP
variants simply inherit from TcpSocketBase, and I think this might be more
clear.  Would you be opposed to changing to this design?  I realize this may add
extra code, but from a design perspective, I believe it might be easier to
understand.

Related to this, I think it is a bit unexpected that TcpNewReno::NewAck and
TcpReno::NewAck make a call to TcpTahoe::NewAck.  I would have expected either
for these methods to simply handle what TcpTahoe does (redundant code) or for
them to call TcpSocketBase::NewAck and have it taken care of there rather than
in TcpTahoe::NewAck.  What do you think?

[Adrian, 2010-08-30 04:13:34]

Hi Tom,

> I am not too concerned about the difference between RFC2582 and 3782;
> the differences should not manifest themselves in the tests that you
> ran.  I just mentioned it since it is the most recent RFC.
>
> However, I expect that the one loss response should be the same for Reno
> and NewReno, and I looked at your code and it seemed like it should
> behave the same, so I am wondering why you are getting different
> results.

The loss response is the same between Reno and NewReno. However, the
problem is what happened after the loss is recovered, i.e. a full ack
is received. In Reno, the operation is as follows, in RFC2581 Sec 3.2
bullet 5:

When the next ACK arrives that acknowledges new data, set cwnd to ssthresh

In NewReno, however, I chose option 1 in RFC3782 Sec 3 bullet 5:

If this ACK acknowledges all of the data up to and including
"recover", then the ACK acknowledges all the intermediate segments
sent between the original transmission of the lost segment and the
receipt of the third duplicate ACK.  Set cwnd to either (1) min
(ssthresh, FlightSize + SMSS) or (2) ssthresh

If I have chosen option 2, that result should be the same. To do so,
note about the following block in function TcpNewReno::NewAck
(tcp-newreno.cc)

else if (m_inFastRec && seq == m_recover)
  { // Full ACK (RFC2582 sec.3 bullet #5 paragraph 2)
    m_cWnd = std::min (m_ssThresh, BytesInFlight () + m_segmentSize);
    m_inFastRec = false;
    NS_LOG_INFO ("Received full ACK. Leaving fast recovery with cwnd
set to " << m_cWnd);
  }

change the line m_cWnd to the following:

m_cWnd = m_ssThresh;

Now, the question is: Should we use option 1 (which is less bursty) or
option 2 in the NewReno implementation?

Regards,
Adrian.

[Adrian, 2010-08-30 04:50:26]

Hi Josh,

> I think it is a little confusing that NewReno inherits from Reno and
> Reno inherits from Tahoe and Tahoe inherits from TcpSocketBase.  I would
> have expected that all TCP variants simply inherit from TcpSocketBase,
> and I think this might be more clear.  Would you be opposed to changing
> to this design?  I realize this may add extra code, but from a design
> perspective, I believe it might be easier to understand.

I agree that the hierarchy is a bit long in this way. However, this is
the best way to make code reuse into the max extent.

Now this is the dilemma: Change the code to make them directly
inherited from TcpSocketBase, but in case of change, different
variants might be inconsistent. Tomorrow, I will make a patchset in
parallel to the current one (which is easy) to use the direct
inheritance approach. Let's see how much difference between them. From
the code reading point of view, of course, a flat hierarchy is more
friendly.

> Related to this, I think it is a bit unexpected that TcpNewReno::NewAck
> and TcpReno::NewAck make a call to TcpTahoe::NewAck.  I would have
> expected either for these methods to simply handle what TcpTahoe does
> (redundant code) or for them to call TcpSocketBase::NewAck and have it
> taken care of there rather than in TcpTahoe::NewAck.  What do you think?

That is because of the slow-start / congestion avoidance operation. It
is unique to Tahoe/Reno/NewReno but not in some other variants, for
example BIC-TCP (which there are three phases of probing). However,
that piece of code is small (one if-else block). For
code-reading-friendly purpose, it can be integrated to one function
when we use the direct inheritance approach. Again, let's see how it
looks like when I upload the next patchset tomorrow.

Regards,
Adrian.

[Adrian, 2010-08-31 09:07:25]

Hi Josh,

I uploaded patchset 13, which is using a flat hierarchy for all TCP variants. It
seems the enlarged file is still manageable.

- Adrian.

On 2010/08/30 04:50:26, Adrian wrote:
> Hi Josh,
> 
> > I think it is a little confusing that NewReno inherits from Reno and
> > Reno inherits from Tahoe and Tahoe inherits from TcpSocketBase.  I
would
> > have expected that all TCP variants simply inherit from TcpSocketBase,
> > and I think this might be more clear.  Would you be opposed to changing
> > to this design?  I realize this may add extra code, but from a design
> > perspective, I believe it might be easier to understand.
> 
> I agree that the hierarchy is a bit long in this way. However, this is
> the best way to make code reuse into the max extent.
> 
> Now this is the dilemma: Change the code to make them directly
> inherited from TcpSocketBase, but in case of change, different
> variants might be inconsistent. Tomorrow, I will make a patchset in
> parallel to the current one (which is easy) to use the direct
> inheritance approach. Let's see how much difference between them. From
> the code reading point of view, of course, a flat hierarchy is more
> friendly.
> 
> > Related to this, I think it is a bit unexpected that TcpNewReno::NewAck
> > and TcpReno::NewAck make a call to TcpTahoe::NewAck.  I would have
> > expected either for these methods to simply handle what TcpTahoe does
> > (redundant code) or for them to call TcpSocketBase::NewAck and have it
> > taken care of there rather than in TcpTahoe::NewAck.  What do you
think?
> 
> That is because of the slow-start / congestion avoidance operation. It
> is unique to Tahoe/Reno/NewReno but not in some other variants, for
> example BIC-TCP (which there are three phases of probing). However,
> that piece of code is small (one if-else block). For
> code-reading-friendly purpose, it can be integrated to one function
> when we use the direct inheritance approach. Again, let's see how it
> looks like when I upload the next patchset tomorrow.
> 
> Regards,
> Adrian.
>

[Josh Pelkey, 2010-08-31 14:25:43]

Adrian,

Thank you very much for doing this!  I think patchset 13 is a bit easier to
follow, though I do see that it is a bit longer.  I prefer 13 though.

I think we would like to try and get this in to ns-3-dev very soon (in the next
week maybe).  Are there any issues you'd like to resolve before that (aside from
the NewReno issue being discussed)?  I assume that patchset 13 has identical
verification outputs/graphs.  I didn't get a chance to do it, but I can later in
the week.

When we get the NewReno issue solved and any others you may have, I will post on
ns-developers about our intention to merge and call for any last
reviews/comments/suggestions; otherwise, we'll merge.

I think we should also get some test cases in there for 3.10 like Tom suggested,
but I don't think this will postpone initial merge.

--
Josh

On 2010/08/31 09:07:25, Adrian wrote:
> Hi Josh,
> 
> I uploaded patchset 13, which is using a flat hierarchy for all TCP variants.
It
> seems the enlarged file is still manageable.
> 
> - Adrian.
> 
> On 2010/08/30 04:50:26, Adrian wrote:
> > Hi Josh,
> > 
> > > I think it is a little confusing that NewReno inherits from Reno and
> > > Reno inherits from Tahoe and Tahoe inherits from TcpSocketBase.  I
> would
> > > have expected that all TCP variants simply inherit from TcpSocketBase,
> > > and I think this might be more clear.  Would you be opposed to
changing
> > > to this design?  I realize this may add extra code, but from a design
> > > perspective, I believe it might be easier to understand.
> > 
> > I agree that the hierarchy is a bit long in this way. However, this is
> > the best way to make code reuse into the max extent.
> > 
> > Now this is the dilemma: Change the code to make them directly
> > inherited from TcpSocketBase, but in case of change, different
> > variants might be inconsistent. Tomorrow, I will make a patchset in
> > parallel to the current one (which is easy) to use the direct
> > inheritance approach. Let's see how much difference between them. From
> > the code reading point of view, of course, a flat hierarchy is more
> > friendly.
> > 
> > > Related to this, I think it is a bit unexpected that TcpNewReno::NewAck
> > > and TcpReno::NewAck make a call to TcpTahoe::NewAck.  I would have
> > > expected either for these methods to simply handle what TcpTahoe does
> > > (redundant code) or for them to call TcpSocketBase::NewAck and have it
> > > taken care of there rather than in TcpTahoe::NewAck.  What do you
> think?
> > 
> > That is because of the slow-start / congestion avoidance operation. It
> > is unique to Tahoe/Reno/NewReno but not in some other variants, for
> > example BIC-TCP (which there are three phases of probing). However,
> > that piece of code is small (one if-else block). For
> > code-reading-friendly purpose, it can be integrated to one function
> > when we use the direct inheritance approach. Again, let's see how it
> > looks like when I upload the next patchset tomorrow.
> > 
> > Regards,
> > Adrian.
> >

[Adrian, 2010-09-02 21:42:07]

Hi all,

I uploaded Patchset 14. I wish I have everything fixed.

Three changes in this patchset:

1. The tcp-loss-response script is updated to make it consistent with the test
in Fall & Floyd (1996) paper

2. TcpSocketBase::SetAllowBroadcast function has the NS_FATAL_ERROR removed. I
agree with Josh that a function not supported is not necessary to make it fatal

3. I changed the == to >= in the fast recovery exit condition in TcpNewReno,
this is a bug which makes the test shown previously in
http://www.nsnam.org/wiki/index.php/New_TCP_Socket_Architecture does not show
window deflation. Now the wiki page is updated as well.

The wikipage http://www.nsnam.org/wiki/index.php/New_TCP_Socket_Architecture has
my script to re-generate the plots, so you can test it out as well.

I wish I fixed everything, but if you still find some, please let me know. My
next step would be implementing TcpVegas. For TcpSack, I think I have to change
the TCP header definition.

- Adrian.

[Adrian, 2010-09-03 07:34:57]

I just uploaded patchset 15 --- now this can pass all test.py test cases

Here is a problem: for test suite routing-aodv-regression, the test case
Bug772ChainTest using TCP produces two pcap files. In this patchset, I just
include the pcap files that generated using the new TCP architecture. I am not a
wireless guy, so I cannot tell if this is a correct output that AODV expects.
Someone please double check this for me. Thanks a lot.

- Adrian.

On 2010/09/02 21:42:07, Adrian wrote:
> Hi all,
> 
> I uploaded Patchset 14. I wish I have everything fixed.
> 
> Three changes in this patchset:
> 
> 1. The tcp-loss-response script is updated to make it consistent with the test
> in Fall & Floyd (1996) paper
> 
> 2. TcpSocketBase::SetAllowBroadcast function has the NS_FATAL_ERROR removed. I
> agree with Josh that a function not supported is not necessary to make it
fatal
> 
> 3. I changed the == to >= in the fast recovery exit condition in TcpNewReno,
> this is a bug which makes the test shown previously in
> http://www.nsnam.org/wiki/index.php/New_TCP_Socket_Architecture does not show
> window deflation. Now the wiki page is updated as well.
> 
> The wikipage http://www.nsnam.org/wiki/index.php/New_TCP_Socket_Architecture
has
> my script to re-generate the plots, so you can test it out as well.
> 
> I wish I fixed everything, but if you still find some, please let me know. My
> next step would be implementing TcpVegas. For TcpSack, I think I have to
change
> the TCP header definition.
> 
> - Adrian.

[Adrian, 2010-09-06 16:18:05]

Patchset 16 is uploaded, a minor feature fix of patchset 15. The only change is
to separate the RTT estimation code from TcpSocketBase::ForwardUp(). This is to
make it convenient to implement delay-based congestion control (e.g. Vegas,
FAST).

[Tom Henderson, 2010-09-06 22:18:13]

On 2010/09/03 07:34:57, Adrian wrote:
> I just uploaded patchset 15 --- now this can pass all test.py test cases
> 
> Here is a problem: for test suite routing-aodv-regression, the test case
> Bug772ChainTest using TCP produces two pcap files. In this patchset, I just
> include the pcap files that generated using the new TCP architecture. I am not
a
> wireless guy, so I cannot tell if this is a correct output that AODV expects.
> Someone please double check this for me. Thanks a lot.
> 
> - Adrian.
> 

I checked this and the divergence in traces is due to the new delayed ack
behavior (new TCP does not wait for delayed ack on first segment), so I think it
is OK.

[Tom Henderson, 2010-09-06 22:19:33]

> 
> I think we should also get some test cases in there for 3.10 like Tom
suggested,
> but I don't think this will postpone initial merge.
> 

Can we make tcp-testcases into a test suite before merge?  I think that the act
of doing this may give us some additional ideas of events that ought to be
traceable.

[Adrian, 2010-09-06 22:53:09]

Including tcp-testcases as a test suite is a good idea. Unlike
tcp-loss-response, what tcp-testcases checks is the state machine (a
protected variable). I have no idea how to make it into a test suite.
What I did now is to print the packet trace AND log messages to stdout
and verify the state change outside ns3. I think there can be two ways
to verify this case:

1. Same as now, print everything into a text file and compare it with
a reference output

2. Make TcpSocketBase::m_state a trace variable

For (1), packet trace is easy (AsciiWriter) but how can we move the
log message to a file?

For (2), I don't feel good to make m_state a trace variable just
because we need a test suite

On Mon, Sep 6, 2010 at 6:19 PM,  <tomh.org@gmail.com> wrote:
>
>
>> I think we should also get some test cases in there for 3.10 like Tom
>
> suggested,
>>
>> but I don't think this will postpone initial merge.
>
>
> Can we make tcp-testcases into a test suite before merge?  I think that
> the act of doing this may give us some additional ideas of events that
> ought to be traceable.
>
> http://codereview.appspot.com/1702042/
>

[Tom Henderson, 2010-09-07 04:58:46]

On 2010/09/06 22:53:09, Adrian wrote:
> Including tcp-testcases as a test suite is a good idea. Unlike
> tcp-loss-response, what tcp-testcases checks is the state machine (a
> protected variable). I have no idea how to make it into a test suite.
> What I did now is to print the packet trace AND log messages to stdout
> and verify the state change outside ns3. I think there can be two ways
> to verify this case:
> 
> 1. Same as now, print everything into a text file and compare it with
> a reference output
> 
> 2. Make TcpSocketBase::m_state a trace variable
> 
> For (1), packet trace is easy (AsciiWriter) but how can we move the
> log message to a file?

We have had problems with ns-2 in dumping trace to a file because it is too hard
to maintain; traces change for unrelated reasons over time and maintainers lose
track of what the trace is trying to prevent from regressing.  

We also have a policy that NS_LOG output is not guaranteed to be stable across
releases so I don't think it is a good idea to rely on.

> 
> For (2), I don't feel good to make m_state a trace variable just
> because we need a test suite
> 

I think that the exercise of trying to turn the tcp-testcases into a test suite
will reveal that there may be other state variables that would be nice to
expose.  m_state is one, m_nextTxSequence is likely another.

The current design makes some variables hard to access as trace sources (e.g.
SND.UNA, RttEstimator).  I can anticipate that we will start getting requests to
trace these internal quantities over time; I don't think it is needed only for
the current regression test.

Here are SSFNet regression tests, which would be worth reviewing to see whether
there are other things we are forgetting.
http://www.ssfnet.org/Exchange/tcp/tcpTestPage.html#testlist

I'd recommend that Josh review this model from a requirements perspective for
tracing requirements, since he is designing courseware this fall using this
model, and then we could discuss how to handle them and how to write the tests.

[Adrian, 2010-09-07 16:16:13]

On Tue, Sep 7, 2010 at 12:58 AM,  <tomh.org@gmail.com> wrote:
>> For (1), packet trace is easy (AsciiWriter) but how can we move the
>> log message to a file?
>
> We have had problems with ns-2 in dumping trace to a file because it is
> too hard to maintain; traces change for unrelated reasons over time and
> maintainers lose track of what the trace is trying to prevent from
> regressing.
>
> We also have a policy that NS_LOG output is not guaranteed to be stable
> across releases so I don't think it is a good idea to rely on.
I agree. I found I broke a lot of traces in ns-3-dev-ref-traces when I
replaced the TCP code.

>> For (2), I don't feel good to make m_state a trace variable just
>> because we need a test suite
>
> I think that the exercise of trying to turn the tcp-testcases into a
> test suite will reveal that there may be other state variables that
> would be nice to expose.  m_state is one, m_nextTxSequence is likely
> another.
>
> The current design makes some variables hard to access as trace sources
> (e.g. SND.UNA, RttEstimator).  I can anticipate that we will start
> getting requests to trace these internal quantities over time; I don't
> think it is needed only for the current regression test.

If that's the case, I think this is the list of variables that someone
might want to traced (in TcpSocketBase or TcpTahoe unless otherwise
noted):

m_rto: Make a new Time variable to hold the most recent RTO value
m_rtt: Make a new Time variable to hold the most recent RTT value
m_nextTxSequence: Next sequence number to send (SequenceNumber32)
m_highTxMark: Highest sequence number ever sent (SequenceNumber32)
m_state: The state (enum States_t, aka int)
m_rxWindowSize: rwnd (uint32_t)
m_cwnd: cwnd (uint32_t)
m_ssthresh: ssthresh (uint32_t)
TcpTxBuffer::m_firstByteSeq: SND.UNA (SequenceNumber32)
TcpRxBuffer::m_nextRxSeq: RCV.NXT (SequenceNumber32)
RttEstimator::est: estimate of smoothed RTT (Time)

I skimmed through the SSF.OS.TCP page, with the two new variable
(m_rto, m_rtt) we can do all the tests on that page. But implementing
all those tests can take some time.

> Here are SSFNet regression tests, which would be worth reviewing to see
> whether there are other things we are forgetting.
> http://www.ssfnet.org/Exchange/tcp/tcpTestPage.html#testlist
>
> I'd recommend that Josh review this model from a requirements
> perspective for tracing requirements, since he is designing courseware
> this fall using this model, and then we could discuss how to handle them
> and how to write the tests.
>
> http://codereview.appspot.com/1702042/
>

I need an advice: To implement the SSF.OS.TCP regression tests, it
seems we need to trace a variable as the simulation goes but not to
verify its value when the simulation stops. So far, one way that I can
imagine to do this is to hook the trace variable into a function. In
that function, we build a state machine to track if the variable
change in an expected way. Set a global variable into true or false to
reflect the trail of the variable matches the expected or not. Is this
the correct way to do? It seems that there is no existing testcase in
ns3 doing the same thing.

[Tom Henderson, 2010-09-12 05:37:02]

> If that's the case, I think this is the list of variables that someone
> might want to traced (in TcpSocketBase or TcpTahoe unless otherwise
> noted):
> 
> m_rto: Make a new Time variable to hold the most recent RTO value
> m_rtt: Make a new Time variable to hold the most recent RTT value
> m_nextTxSequence: Next sequence number to send (SequenceNumber32)
> m_highTxMark: Highest sequence number ever sent (SequenceNumber32)
> m_state: The state (enum States_t, aka int)
> m_rxWindowSize: rwnd (uint32_t)
> m_cwnd: cwnd (uint32_t)
> m_ssthresh: ssthresh (uint32_t)
> TcpTxBuffer::m_firstByteSeq: SND.UNA (SequenceNumber32)
> TcpRxBuffer::m_nextRxSeq: RCV.NXT (SequenceNumber32)
> RttEstimator::est: estimate of smoothed RTT (Time)

I will send you a patch by email where I've converted m_state to a read-only
attribute and a trace source, and hooked it in a modified example.  The approach
should be clonable for many of the above.  

The main technical issue is how to trace SND.UNA and RCV.NXT which are private
variables in a private buffer.

Regarding RttEstimator::est, this may not need to be traceable if we just trace
m_rto and m_rtt in the TcpSocketBase class.

> 
> I skimmed through the SSF.OS.TCP page, with the two new variable
> (m_rto, m_rtt) we can do all the tests on that page. But implementing
> all those tests can take some time.

I am not advocating to do that before merging.

> 
> I need an advice: To implement the SSF.OS.TCP regression tests, it
> seems we need to trace a variable as the simulation goes but not to
> verify its value when the simulation stops. So far, one way that I can
> imagine to do this is to hook the trace variable into a function. In
> that function, we build a state machine to track if the variable
> change in an expected way. Set a global variable into true or false to
> reflect the trail of the variable matches the expected or not. Is this
> the correct way to do? It seems that there is no existing testcase in
> ns3 doing the same thing.

To take m_state as an example, one way to do it would be to code a state machine
to make sure the traced variable steps through the right sequence, as you
suggest.  Another way would be to just spot check the variable such as "Is
m_state >= ESTABLISHED by time 1.0s?", is m_state >= TIME_WAIT by 1.4s?", etc. 
I don't really have a preference although I would just generally suggest to find
a signature event or events in the test case that can be isolated and directly
tested for regression purposes.  

I would like to see this merged soon and would like to avoid imposing additional
requirements, so I'll list what I'd like to see to get a +1 from me, and maybe
anyone else interested can list what they are still waiting to see:

1) most or all of the variables listed above are converted to read-only
attributes and trace sources, and at least we have no showstoppers to finish all
of the above list off at some future date (i.e. explain how SND.UNA and RCV.NXT
could be handled)

2) the tcp-testcases program is converted also to a regression test suite with
most or all testcases converted to an equivalent TestCase such as described
above

3) the usual coding-style/doxygen requirements are met, and the old
implementation is removed

[joao.taveira, 2010-10-24 18:11:49]

Hi,

Here are a few comments from having tried the TCP refactoring out. I only
focused on the TCP state machine bit as I have not looked at the individual CC
algorithms. Overall this is a huge improvement, so I hope it will be merged
soon.

Joao.

http://codereview.appspot.com/1702042/diff/168002/src/internet-stack/tcp-sock...
File src/internet-stack/tcp-socket-base.cc (right):

http://codereview.appspot.com/1702042/diff/168002/src/internet-stack/tcp-sock...
src/internet-stack/tcp-socket-base.cc:258: 
A socket may be reused after CLOSED, so really we should make sure we initialize
some of the variables here. Some of the variables which would have be reset
include:
m_rxBuffer
m_rtt
m_highTxMark
m_cnCount
m_cnTimeout

..and maybe a few more.

http://codereview.appspot.com/1702042/diff/168002/src/internet-stack/tcp-sock...
src/internet-stack/tcp-socket-base.cc:973: // but this move is not yet
implemeneted. Is this necessary?
Yes, this is necessary otherwise the socket will not be destroyed, for long sims
with lots of flows this may seriously degrade performance. I'm not too fussed if
this moves directly to close tbh (so the socket can be reused), but keeping it
in TIME_WAIT might not be what applications expect.

http://codereview.appspot.com/1702042/diff/168002/src/internet-stack/tcp-sock...
src/internet-stack/tcp-socket-base.cc:1154: m_cnTimeout = m_cnTimeout +
m_cnTimeout;
Since m_cnTimeout is set externally, some nicer way of setting m_rto without
losing its value would be neater. That way if we reopen a socket after CLOSED,
we can just reset the value.

My suggestion would be to leave m_cnTimeout and m_cnCount as the default values,
and keep new counter (m_cnNum) which is incremented for every SYN transmitted.

m_cnNum++
if (m_cnNum > m_cnCount) Notify ; return...
m_rto = m_cnTimeout^(m_cnNum) 

This way all you need to do on connect is make sure m_cnNum is reset to 0.

http://codereview.appspot.com/1702042/diff/168002/src/internet-stack/tcp-sock...
src/internet-stack/tcp-socket-base.cc:1167: m_retxEvent = Simulator::Schedule
(m_rto, &TcpSocketBase::SendEmptyPacket, this, flags);
Not sure this works the way you think it should. You decrement m_cnCount, but
then schedule SendEmptyPacket on timeout, but there is no check here to see if
m_cnCount has reached zero, so SYN packets might be repeatedly sent at longer
and longer timeouts.

m_rto should also be bounded, I believe RFC specifies MAX_RTO 240s. This would
avoid m_cnTimeout overflowing as it increases exponentially.

[Josh Pelkey, 2010-12-15 18:07:39]

Adrian, this was pointed out to me by a student a Georgia Tech (see comment
below).

http://codereview.appspot.com/1702042/diff/168002/src/internet-stack/tcp-sock...
File src/internet-stack/tcp-socket-base.cc (right):

http://codereview.appspot.com/1702042/diff/168002/src/internet-stack/tcp-sock...
src/internet-stack/tcp-socket-base.cc:437: address = InetSocketAddress
(m_endPoint->GetLocalAddress (), m_endPoint->GetLocalPort ());
RecvFrom calls this method which is supposed to return the remote address;
however, here you are returning the local address.