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

Issue 24900043: TCP Window Scale and Timestamps Implementation

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 2 months ago by trucanh524
Modified:
7 years, 9 months ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

TCP Window Scale and Timestamps Implementation The implementation in this submission uses the TCP option interface implemented by Adrian Tam in Issue 5452045, which can be found at https://codereview.appspot.com/5452045. TCP window scale option allows a 32-bit window to be carried in the 16-bit advertised window field of the TCP header (RFC 1323) enabling TCP simulations in high speed networks. Once window scale option is enabled through the attribute WindScaleEnabled in TcpSocketBase, the two endpoints exchange their scale factors (m_rcvWindScale) in the SYN and SYN-ACK segments at the beginning of a connection. Then all the advertised receive windows will be left-shifted by the scale factor (m_sndWindScale) before updating m_rWnd. TCP timestamps implementation enhances the Rtt estimation in ns-3 by eliminating the use of RttHistory to keep track of timestamps. However, since the Timestamps option (TSopt) (RFC 1323) occupies 10 out of the maximum 40 bytes allowed for all TCP options, the implementation does not make TSopt mandatory. Users can enable or disable TSopt using TcpSocketBase's attribute TSEnabled. Once enabled, a TSopt will be sent in the SYN segment to initiate TSopt usage for the connection. Then each data segment and ACK segment will carry a TSopt. As stated in RFC 1323, the Timestamp Echo Reply field (TSecr) is only vaid when the ACK bit is set. When it is valid, it has the value of m_recentTS whose value is obtained by following the algorithm described in the RFC. That is, a segment's timestamp (m_ts) can only be echoed (copied into m_recentTS) if m_lastAckSent falls within its range of sequence numbers. This condition is checked in ReceivedData () before an ACK is sent. RttEstimator is also modified with the addition of EstimateRtt () to replace AckSeq () when Tsopt is used. TcpSocketBase is also modified with the addition of 2 new member methods: ProcessSentOptions () and ProcessReceivedOptions () that in turns call the corresponding option's Set () and Get () to process all options appended to an outgoing and incoming segment, respectively.

Patch Set 1 #

Total comments: 17

Patch Set 2 : #

Total comments: 48
Unified diffs Side-by-side diffs Delta from patch set Stats (+1881 lines, -208 lines) Patch
M src/internet/model/rtt-estimator.h View 1 3 chunks +16 lines, -0 lines 0 comments Download
M src/internet/model/rtt-estimator.cc View 3 chunks +18 lines, -2 lines 0 comments Download
M src/internet/model/tcp-header.h View 1 4 chunks +92 lines, -25 lines 1 comment Download
M src/internet/model/tcp-header.cc View 1 10 chunks +194 lines, -55 lines 6 comments Download
M src/internet/model/tcp-l4-protocol.cc View 2 chunks +2 lines, -2 lines 1 comment Download
A src/internet/model/tcp-option.h View 1 1 chunk +89 lines, -0 lines 3 comments Download
A src/internet/model/tcp-option.cc View 1 1 chunk +75 lines, -0 lines 1 comment Download
A src/internet/model/tcp-option-end.h View 1 1 chunk +80 lines, -0 lines 3 comments Download
A src/internet/model/tcp-option-end.cc View 1 1 chunk +86 lines, -0 lines 3 comments Download
A src/internet/model/tcp-option-ts.h View 1 1 chunk +102 lines, -0 lines 2 comments Download
A src/internet/model/tcp-option-ts.cc View 1 1 chunk +127 lines, -0 lines 6 comments Download
A src/internet/model/tcp-option-winscale.h View 1 1 chunk +91 lines, -0 lines 1 comment Download
A src/internet/model/tcp-option-winscale.cc View 1 1 chunk +106 lines, -0 lines 6 comments Download
M src/internet/model/tcp-socket-base.h View 1 7 chunks +508 lines, -86 lines 3 comments Download
M src/internet/model/tcp-socket-base.cc View 1 27 chunks +287 lines, -38 lines 12 comments Download
M src/internet/wscript View 2 chunks +8 lines, -0 lines 0 comments Download

Messages

Total messages: 4
bpswenson
Hello Thanks for the contribution! I made a few comments inline all minor. Mostly formatting ...
8 years, 1 month ago (2013-11-26 20:41:46 UTC) #1
trucanh524
Hi Brian, Thank you for reviewing the code and helping me with the test script. ...
8 years, 1 month ago (2013-12-09 23:38:16 UTC) #2
Peter Barnes
I'd *much* prefer to see this use defined enum constants, rather than hard-coded "kind" values ...
8 years ago (2013-12-26 21:25:44 UTC) #3
Tom Henderson
7 years, 9 months ago (2014-04-18 17:11:05 UTC) #4
These need some additional work, since (if I recall correctly the last time I
tested this patch) they can crash when run against an implementation TCP that
generates additional options like SackPermitted).  Peter also raises some good
suggestions.

I've suggested to Anh to try to address Peter's comments, and then I'd like to
proceed with these steps:

1) get Adrian's base TCP option code in with support for option kinds 0, 1, 2,
and > 2.  The code should not crash when run against a TCP that generates
options (i.e. a real TCP that generates Sack permitted, or possibly others that
come in the SYN).  A TCP test that tests the deserialization logic for specially
constructed test TCP options would be useful.  For instance, create two test
options that, when included, require also the use of No-op option and padding.

Once we are OK with this design, then add Window Scale and Timestamp as separate
patches on top of this.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b