Bugzilla – Bug 2263
Support processing of multiple TCP options of the same type within 1 header
Last modified: 2017-01-25 13:11:26 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
(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.
>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 ?
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.
Created attachment 2247 [details] IsTcpOptionEnabled added Copy/paste from your repo
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.
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.
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.
Created attachment 2646 [details] Added method IsTcpOptionEnabled Patch #3 of the series.
pushed in commit 1256{5,6,7}