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

Issue 299130043: TCP SACK + SACK emulation

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 10 months ago by n.p
Modified:
7 years, 3 months ago
Reviewers:
mattator
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

Implementation of TCP SACK and SACK emulation inside tcp-socket-base.cc . The patch is quite big, it is divided in commit in my personal github repo. This is the optimized version. https://github.com/kronat/ns-3-dev-git/tree/tcp-sack

Patch Set 1 #

Total comments: 17

Patch Set 2 : Matt comments addressed, ASSERT inserted, management of the RTO improved #

Patch Set 3 : SACK rebased on top of ns-3-dev #

Patch Set 4 : Fixed NextSeg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3879 lines, -646 lines) Patch
M examples/tcp/tcp-variants-comparison.cc View 1 2 3 3 chunks +3 lines, -0 lines 0 comments Download
M src/internet/doc/tcp.rst View 1 2 1 chunk +38 lines, -1 line 0 comments Download
M src/internet/model/tcp-option.h View 1 2 1 chunk +8 lines, -6 lines 0 comments Download
M src/internet/model/tcp-option.cc View 5 chunks +14 lines, -8 lines 0 comments Download
A src/internet/model/tcp-option-sack.h View 1 chunk +101 lines, -0 lines 0 comments Download
A src/internet/model/tcp-option-sack.cc View 1 1 chunk +159 lines, -0 lines 0 comments Download
A + src/internet/model/tcp-option-sack-permitted.h View 3 chunks +25 lines, -41 lines 0 comments Download
A src/internet/model/tcp-option-sack-permitted.cc View 1 1 chunk +107 lines, -0 lines 0 comments Download
M src/internet/model/tcp-rx-buffer.h View 3 chunks +80 lines, -2 lines 0 comments Download
M src/internet/model/tcp-rx-buffer.cc View 1 3 chunks +129 lines, -0 lines 0 comments Download
M src/internet/model/tcp-socket-base.h View 1 2 3 13 chunks +98 lines, -26 lines 0 comments Download
M src/internet/model/tcp-socket-base.cc View 1 2 3 37 chunks +573 lines, -333 lines 0 comments Download
M src/internet/model/tcp-tx-buffer.h View 1 2 3 6 chunks +372 lines, -25 lines 0 comments Download
M src/internet/model/tcp-tx-buffer.cc View 1 2 3 7 chunks +990 lines, -82 lines 0 comments Download
M src/internet/test/tcp-bytes-in-flight-test.cc View 1 2 3 8 chunks +39 lines, -11 lines 0 comments Download
M src/internet/test/tcp-fast-retr-test.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/internet/test/tcp-fast-retr-test.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/internet/test/tcp-general-test.h View 1 2 7 chunks +29 lines, -8 lines 0 comments Download
M src/internet/test/tcp-general-test.cc View 1 2 5 chunks +39 lines, -12 lines 0 comments Download
M src/internet/test/tcp-header-test.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/internet/test/tcp-option-test.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M src/internet/test/tcp-rto-test.h View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M src/internet/test/tcp-rto-test.cc View 1 2 5 chunks +8 lines, -8 lines 0 comments Download
A src/internet/test/tcp-rx-buffer-test.cc View 1 chunk +346 lines, -0 lines 0 comments Download
A src/internet/test/tcp-sack-permitted-test.cc View 1 2 3 1 chunk +198 lines, -0 lines 0 comments Download
M src/internet/test/tcp-timestamp-test.cc View 1 chunk +1 line, -1 line 0 comments Download
A src/internet/test/tcp-tx-buffer-test.cc View 1 2 3 1 chunk +402 lines, -0 lines 0 comments Download
M src/internet/wscript View 1 2 3 5 chunks +10 lines, -3 lines 0 comments Download
M src/test/ns3tcp/ns3tcp-loss-test-suite.cc View 1 2 3 13 chunks +47 lines, -36 lines 0 comments Download
M src/test/ns3tcp/ns3tcp-state-test-suite.cc View 1 2 3 17 chunks +55 lines, -35 lines 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno1-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno2-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno3-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno4-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-Westwood2-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-Westwood3-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-Westwood4-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-WestwoodPlus2-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-WestwoodPlus3-response-vectors.pcap View 1 2 Binary file 0 comments Download
M src/test/ns3tcp/response-vectors/ns3tcp-loss-WestwoodPlus4-response-vectors.pcap View 1 2 Binary file 0 comments Download

Messages

Total messages: 3
n.p
Request for comments on TCP SACK and SACK emulation implementation
7 years, 10 months ago (2016-05-23 08:56:23 UTC) #1
mattator
Mostly details, I trust the logic :) https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-header.h File src/internet/model/tcp-header.h (right): https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-header.h#newcode50 src/internet/model/tcp-header.h:50: typedef std::list< ...
7 years, 10 months ago (2016-06-01 15:55:54 UTC) #2
n.p
7 years, 9 months ago (2016-06-10 13:44:56 UTC) #3
On 2016/06/01 15:55:54, mattator wrote:
> Mostly details, I trust the logic :)

Aahah you're wrong, the logic should be checked above all :) Anyway, the
comments you don't see here are the ones fixed in the v2. I reply for the
others.


>
https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-option...
> src/internet/model/tcp-option-sack.h:76: void AddSackBlock (SackBlock s);
> Is it necessary to clutter the public interface with a "SackBlock" ? I prefer
to
> hide the implementation detail when possible and not :
> AddSackBlock (SequenceNumber32, SequenceNumber32)

Yes, it is used in a lot of places. In fact, if I don't expose SackBlock, then I
should redefine a pair of SequenceNumber32 everywhere.. Probably at this point
we can move it to the core, with another name (such as SequenceNumber32Pair).


>
https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
> src/internet/model/tcp-socket-base.cc:1262: }
> the HasOption does the same operation as GetOption expect it returns less
> information. Can't we use sthg more like
> 
> /**
>  * \brief Helper function to find an MPTCP option
>  *
>  * \param ret save found option in ret, otherwise
>  * \return true if matching option type found. If false, ret should be
> considered invalid
> */
> template<class T>
> bool
> GetTcpOption (const TcpHeader& header, Ptr<const T>& ret)
> {
>   TcpHeader::TcpOptionList l;
>   header.GetOptions (l);
>   for (TcpHeader::TcpOptionList::const_iterator it = l.begin (); it != l.end
();
> ++it)
>     {
>       if ( (*it)->GetInstanceTypeId () == T::GetTypeId())
>         {
>               ret = DynamicCast<const T> ( *it );
>               return (ret != 0);
>         }
>     }
>   return false;
> }

This makes sense, but I don't want to incorporate it in this patchset. Anyway, I
modified the Option to be const.

>
https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
> src/internet/model/tcp-socket-base.h:956: 
> It is strange to handle a specific case here. In fact I wonder if ReadOptions
> make sense at all. It is useful in linux to dissect and populate
datastructures,
> the analysis of each TCP option is done when appropriate. 
> In ns3, deserialisation is automatic so we just need to process the option
where
> appropriate and it depends from the TCP state, etc... there are many cases so
> it's hard to say "we process the options here !" ?

As you wish; the placeholder is mainly for MPTCP, but if you will not use it...
we can remove it :)


>
https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-tx-buf...
> src/internet/model/tcp-tx-buffer.h:39: class TcpTxItem
> "Item" does not say much, what about TcpTxPacket ?

TcpTxSegment maybe, but it's not a segment in the natural way ... it's more a
struct with flags for us. For that reason I kept TcpTxItem .. is an item that
will be transmitted.

>
https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-tx-buf...
> src/internet/model/tcp-tx-buffer.h:224: 
> rename to "UpdateScoreBoard" ?

The RFC says "Update". Anyway we can rename it ... what is the best?
Sign in to reply to this message.

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