Bug 2263 - Support processing of multiple TCP options of the same type within 1 header
Support processing of multiple TCP options of the same type within 1 header
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: tcp
ns-3-dev
PC Linux
: P5 enhancement
Assigned To: natale.patriciello
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-13 12:20 EST by Matthieu Coudron
Modified: 2017-01-25 13:11 EST (History)
1 user (show)

See Also:


Attachments
Patch adding ReadOptions with a placeholder for a switch-loop (3.65 KB, patch)
2016-01-21 05:10 EST, natale.patriciello
Details | Diff
IsTcpOptionEnabled added (1.48 KB, patch)
2016-01-21 05:11 EST, natale.patriciello
Details | Diff
placeholder for option switching (3.88 KB, patch)
2016-11-02 14:45 EDT, natale.patriciello
Details | Diff
GetOption returns const Option (3.22 KB, patch)
2016-11-02 14:46 EDT, natale.patriciello
Details | Diff
Added method IsTcpOptionEnabled (4.60 KB, patch)
2016-11-02 14:47 EDT, natale.patriciello
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthieu Coudron 2016-01-13 12:20:47 EST
Looking at TCP options, I would like to ask for your opinion before submitting a patch with stub support for MPTCP:

First of all in TcpSocketBase::ReadOptions (), I would like to replace the current structure with a loop+switch as in:
https://github.com/teto/ns-3-dev-git/blob/merge_nat/src/internet/model/tcp-socket-base.cc#L1689
```
  for(TcpHeader::TcpOptionList::const_iterator it(options.begin()); it != options.end(); ++it)
  {
      //!
      Ptr<const TcpOption> option = *it;
      switch(option->GetKind())
      {
        case TcpOption::WINSCALE:
          if ((header.GetFlags () & TcpHeader::SYN) && m_winScalingEnabled && m_state < ESTABLISHED)
            {
              ProcessOptionWScale (option);
              ScaleSsThresh (m_sndScaleFactor);
            }
            break;
```

The current code calls "HasOption()" to know if it should process an option but it is problematic in the case the option is present multiple times (as it is the case with MPTCP). The previous structure solves that problem.

The other proposition I have is an optional one to increase readability (not a problem if refused). Right now there is one variable per TCP option. Within the code, adding a member:
virtual bool TcpSocketBase::IsTcpOptionAllowed (uint8_t  kind) const;
would make it more programmatic
https://github.com/teto/ns-3-dev-git/blob/merge_nat/src/internet/model/tcp-socket-base.cc#L3458
Comment 1 natale.patriciello 2016-01-13 16:09:47 EST
(In reply to Matthieu Coudron from comment #0)
> First of all in TcpSocketBase::ReadOptions (), I would like to replace the
> current structure with a loop+switch as in:
> https://github.com/teto/ns-3-dev-git/blob/merge_nat/src/internet/model/tcp-
> socket-base.cc#L1689
> ```
>   for(TcpHeader::TcpOptionList::const_iterator it(options.begin()); it !=
> options.end(); ++it)
>   {
>       //!
>       Ptr<const TcpOption> option = *it;
>       switch(option->GetKind())
>       {
>         case TcpOption::WINSCALE:
>           if ((header.GetFlags () & TcpHeader::SYN) && m_winScalingEnabled
> && m_state < ESTABLISHED)
>             {
>               ProcessOptionWScale (option);
>               ScaleSsThresh (m_sndScaleFactor);
>             }
>             break;
> ```
> 
> The current code calls "HasOption()" to know if it should process an option
> but it is problematic in the case the option is present multiple times (as
> it is the case with MPTCP). The previous structure solves that problem.

The problem is that, to make some options compatible with RFC, I need to rewrite both ReadOptions and AddOptions. The for-switch idea is really interesting! 

Basically, now Options are managed as they are added / processed every time. As I learned with timestamps, this is not true; for example, ProcessTSOption (which updates m_timestampToEcho) should be called only when an ACK that advances SND.UNA is received (and so, in the method NewAck). Also window scale should be controlled only for SYN packets. So, to recap, with TS and WS option the for-switch is not necessary.

But for SACK and MPTCP, the solution you proposed is cool. I will prepare a snippet where this for-switch could be expanded inserting SACK or MPTCP options.

Only one question : can MPTCP option be read before Ack processing, e.g. it is ok to done it at the beginning of DoForwardUp ?

> The other proposition I have is an optional one to increase readability (not
> a problem if refused). Right now there is one variable per TCP option.
> Within the code, adding a member:
> virtual bool TcpSocketBase::IsTcpOptionAllowed (uint8_t  kind) const;
> would make it more programmatic
> https://github.com/teto/ns-3-dev-git/blob/merge_nat/src/internet/model/tcp-
> socket-base.cc#L3458

Usually I'm not against simplyfing the developer's life, but in the actual code there isn't the need for such a function.. if used heavily for MPTCP, it could be an easy addition. Anyway, to worse the situation, window scale is not a boolean anymore, but can contains 3 value: ON, OFF, and ON but disabled. See Bug 2068 and bug 2262.
Comment 2 Matthieu Coudron 2016-01-13 20:03:43 EST
>Only one question : can MPTCP option be read before Ack processing, e.g. it is >ok to done it at the beginning of DoForwardUp ?
I suppose it should be ok. I suppose I should look at how you do things for SACK and then submit a TCP patch. Do you have a timeframe time ? I hate to ask but I would like to be able to submit an MPTCP proposal right after ns3.25 so that it can be polished over a few months for ns3.26.


>window scale is not a boolean anymore, but can contains 3 value: ON, OFF, and ON >but disabled
Well even before, it was kinda a tristate value aka a *polysemic* boolean. It was first set to true to allow it (IIRC) and then changed to false in case negotiation failed, thus meaning both "allowed" and "enabled". 
I just would like to have a clear distinction between these 2 concepts, which didn't seem to be the case but might be now ?
Comment 3 natale.patriciello 2016-01-21 05:10:30 EST
Created attachment 2246 [details]
Patch adding ReadOptions with a placeholder for a switch-loop

This is my try. I inserted it before the Ack processing, I'm not sure if it's ok.
Comment 4 natale.patriciello 2016-01-21 05:11:07 EST
Created attachment 2247 [details]
IsTcpOptionEnabled added

Copy/paste from your repo
Comment 5 Matthieu Coudron 2016-01-21 08:13:54 EST
Some bug I noticed during my mptcp work is that TcpHeader computes TCP option length in a stateful manner. 
With the current code, you can call AppendOption (TcpHeader updates its option length), modify the option afterwards: there will be a mismatch between the serialized length and the length computed by TcpHeader. That's why in my branch I added "const" and replaced:
typedef std::list< Ptr<TcpOption> > TcpOptionList; //!< List of TcpOption
by
typedef std::list< Ptr<const TcpOption> > TcpOptionList;
bool AppendOption (Ptr<const TcpOption> option); 

I think you can remove the ReadOptions call in TcpSocketBase::ReceivedAck since it does nothing.

I would also vote for deleting TcpHeader::HasOption that does less than GetOption, and add this one:
https://github.com/teto/ns-3-dev-git/blob/merge_nat/src/internet/model/tcp-header.h#L354
Problem with GetOption is that you need to downcast anyway so let's do it from the start.

As for IsTcpOptionEnabled, I am ok.
Comment 6 natale.patriciello 2016-11-02 14:45:56 EDT
Created attachment 2644 [details]
placeholder for option switching

Patch #1 for this bug, add a placeholder switch. I will use it in SACK and (possibly) with MpTCP.
Comment 7 natale.patriciello 2016-11-02 14:46:48 EDT
Created attachment 2645 [details]
GetOption returns const Option

As Matt pointed out, we should not allow the edit of Option once they are in the TcpOptionList. This patch does what he says. Patch #2 of the series.
Comment 8 natale.patriciello 2016-11-02 14:47:27 EDT
Created attachment 2646 [details]
Added method IsTcpOptionEnabled

Patch #3 of the series.
Comment 9 natale.patriciello 2017-01-25 13:11:26 EST
pushed in commit 1256{5,6,7}