TCP SACK + SACK emulation

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

[n.p, 2016-05-23 08:56:23]

Request for comments on TCP SACK and SACK emulation implementation

[mattator, 2016-06-01 15:55:54]

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...
src/internet/model/tcp-header.h:50: typedef std::list< Ptr<TcpOption> >
TcpOptionList; //!< List of TcpOption
I would highly suggest adding a const :
typedef std::list< Ptr<const TcpOption> > TcpOptionList;
otherwise as the header length is stateful, if you add an option and modify it
afterwards, the size of options mismatch and create bugs.

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-option...
File src/internet/model/tcp-option-sack.cc (right):

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-option...
src/internet/model/tcp-option-sack.cc:63: {
Shouldn't it print like the number of blocks at least (printing the blocks may
end up verbose).

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-option...
File src/internet/model/tcp-option-sack.h (right):

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)

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-rx-buf...
File src/internet/model/tcp-rx-buffer.cc (right):

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-rx-buf...
src/internet/model/tcp-rx-buffer.cc:267: m_sackList.insert (m_sackList.begin (),
current);
or "push_front"

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.cc (right):

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;
}

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:2917: if (win < unack)
Looks like a regression in terms of readibility

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:3154: //m_tcb->m_highTxMark =
m_txBuffer->HeadSequence ();
remove comment

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:3583: header.AppendOption (option);
I am a bit worried that nowhere AppendOption's return value is checked. With
MPTCP, the handling of TCP space option might become even more critic so what do
we do if we can't append the packet ? Should we remember it and send a second
ack with the information ? MPTCP advises to do so  as long as it does not
trigger a duplicate ack.

This option is small and during the SYN so it's ok so the comment is more valid
for AddOptionSACK I guess.

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:3598: {
add an NS_LOG_DEBUG  ?

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.h (right):

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.h:793: * The difference between SND.UNA and
HighTx
I applaud ! Much clearer

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 !" ?

Also if you follow the Qt API  guidelines
(https://wiki.qt.io/API_Design_Principles), you would use a pointer instead of a
reference

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-tx-buf...
File src/internet/model/tcp-tx-buffer.h (right):

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 ?

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

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-tx-buf...
src/internet/model/tcp-tx-buffer.h:235: 
is Ptr<const TcpOptionSack> possible ?

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-tx-buf...
src/internet/model/tcp-tx-buffer.h:254: 
https://wiki.qt.io/API_Design_Principles  => pointer rather than ref

https://codereview.appspot.com/299130043/diff/1/src/internet/model/tcp-tx-buf...
src/internet/model/tcp-tx-buffer.h:421: 
t2 should be const ? wouldn't it be cleaner to return a new object instead or
move this funciton as a member of TxItem ?

https://codereview.appspot.com/299130043/diff/1/src/internet/test/tcp-tx-buff...
File src/internet/test/tcp-tx-buffer-test.cc (right):

https://codereview.appspot.com/299130043/diff/1/src/internet/test/tcp-tx-buff...
src/internet/test/tcp-tx-buffer-test.cc:355: //                       "TxBuf
miscalculates size of in flight segments");
comments

[n.p, 2016-06-10 13:44:56]

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?