Created attachment 2233 [details]
Proposed patch

Attached the proposed patch; it fixes the naming convention, and initialize cwnd and ssth (in the way I proposed) when a packet with the SYN flag is received.

(In reply to natale.patriciello from comment #0)
> In Bug 2067, we introduced the API ScaleSsThresh to be compliant with
> https://tools.ietf.org/html/rfc5681 , page 4, where it says:
> 
> The initial value of ssthresh SHOULD be set arbitrarily high (e.g., to the
> size of the largest possible advertised window)
> 
> the function is:
> 
> void
> TcpSocketBase::ScaleSsThresh (uint8_t scaleFactor)
> {
>   m_tcb->m_ssThresh <<= scaleFactor;
> }
> 
> and the use this function with wrong shift (ours instead of the other
> counterpart), this is the first part of the bug.
> 
> However, I would like to get rid of this function, because now users can't
> use ssthresh exact value as Attribute.
> 
> Let's think in terms of two agents: we have Sender (S) and Receiver (R), and
> their buffer are returned by imaginary functions Tx and Rx. Tx (S) returns
> the Tx Buffer of the sender, and Rx (S) the Rx buffer of the sender. As RFC
> 7323 demands, we have Snd.Wind.Shift and Rcv.Wind.Shift, to be applied to
> the incoming and outgoing window fields, respectively.
> 
> SsThresh (S) = 0xffff (attribute default)
> 
> later in the code, we have
> 
> SsThresh (S) <<= RCV.WND.SHIFT;
> 
> This is wrong because we should have used SND.WND.SHIFT (see first part),
> but on the other hand the value set by the user is always shifted. What if I
> want to make a simulation with a limited slow start value (e.g. 10 segments?)
> 
> I propose to:
> 
> - Default InitialSlowStartThreshold set to UINT32_MAX (we cannot exceed the
> 32 bit boundary in TCP)
> - If the previous value is UINT32_MAX, instead of ScaleSsThresh, set
> InitialSlowStartThreshold to the maximum adv window
> - If the previous value is different, do nothing.
> 
> Comments ?

I agree with most of this patch, but suggest to delete this:

+      if (m_tcb->m_initialSsThresh == UINT32_MAX)
+        {
+          m_tcb->m_initialSsThresh = m_rWnd << m_sndWindShift;
+        }


Reasons to delete:

1) where in the RFCs does it say to do this?  Why not leave it at UINT32_MAX?

2) just above this code, and in RFC 5681, it says that rWnd in SYN segment is not scaled, but here you are scaling it.  Just above this code, you are not scaling it when rWnd is assigned.  Elsewhere in the code, rWnd is stored as the scaled value, so I don't see why you store it differently initially.

(In reply to Tom Henderson from comment #2)
> I agree with most of this patch, but suggest to delete this:
> 
> +      if (m_tcb->m_initialSsThresh == UINT32_MAX)
> +        {
> +          m_tcb->m_initialSsThresh = m_rWnd << m_sndWindShift;
> +        }
> 
> 
> Reasons to delete:
> 
> 1) where in the RFCs does it say to do this?  Why not leave it at UINT32_MAX?

RFC 5681, page 4:

"  The initial value of ssthresh SHOULD be set arbitrarily high (e.g.,
   to the size of the largest possible advertised window), "

We could take the approach of Linux, where 0 is "arbitrarily high". This would also not mess up graphs (where the first value of ssthresh would mess up scales of the graph). What do you think?


> 2) just above this code, and in RFC 5681, it says that rWnd in SYN segment
> is not scaled, but here you are scaling it.  Just above this code, you are
> not scaling it when rWnd is assigned.  Elsewhere in the code, rWnd is stored
> as the scaled value, so I don't see why you store it differently initially.

This is a portion of the patch:

       /* The window field in a segment where the SYN bit is set (i.e., a <SYN>
        * or <SYN,ACK>) MUST NOT be scaled (from RFC 7323 page 9). But should be
        * saved anyway..
        */
       m_rWnd = tcpHeader.GetWindowSize ();
+
+      if (m_tcb->m_initialSsThresh == UINT32_MAX)
+        {
+          m_tcb->m_initialSsThresh = m_rWnd << m_sndWindShift;
+        }

The value that should be stored is the receiver window; it should not be scaled for SYN segments.
As you can see, m_rWnd is stored but not scaled; m_rWnd << m_sndWindShift does not scale m_rWnd, but store the scaled value inside m_initialSsThresh.

(In reply to natale.patriciello from comment #3)
> (In reply to Tom Henderson from comment #2)
> > I agree with most of this patch, but suggest to delete this:
> > 
> > +      if (m_tcb->m_initialSsThresh == UINT32_MAX)
> > +        {
> > +          m_tcb->m_initialSsThresh = m_rWnd << m_sndWindShift;
> > +        }
> > 
> > 
> > Reasons to delete:
> > 
> > 1) where in the RFCs does it say to do this?  Why not leave it at UINT32_MAX?
> 
> RFC 5681, page 4:
> 
> "  The initial value of ssthresh SHOULD be set arbitrarily high (e.g.,
>    to the size of the largest possible advertised window), "
> 
> We could take the approach of Linux, where 0 is "arbitrarily high". This
> would also not mess up graphs (where the first value of ssthresh would mess
> up scales of the graph). What do you think?

I was asking about where it says to reduce it to the receive window, vs. just leaving it very high.  I thought that it was only to be reduced upon detection of congestion.  Your comment suggests that this is due to graphing concerns.  

Setting it to zero may lead to confusion that it is disabled; e.g. see
https://monolight.cc/2010/12/increasing-tcp-initial-congestion-window/

There may still be a scale issue with a graph since a UINT32_MAX value exists before this, right?  I guess I'd be fine with reducing it for graphing purposes if it were shown to eliminate the problem, and if a comment in the code was inserted as to why this is being assigned to the value of m_rWnd.

> 
> 
> > 2) just above this code, and in RFC 5681, it says that rWnd in SYN segment
> > is not scaled, but here you are scaling it.  Just above this code, you are
> > not scaling it when rWnd is assigned.  Elsewhere in the code, rWnd is stored
> > as the scaled value, so I don't see why you store it differently initially.
> 
> This is a portion of the patch:
> 
>        /* The window field in a segment where the SYN bit is set (i.e., a
> <SYN>
>         * or <SYN,ACK>) MUST NOT be scaled (from RFC 7323 page 9). But
> should be
>         * saved anyway..
>         */
>        m_rWnd = tcpHeader.GetWindowSize ();
> +
> +      if (m_tcb->m_initialSsThresh == UINT32_MAX)
> +        {
> +          m_tcb->m_initialSsThresh = m_rWnd << m_sndWindShift;
> +        }
> 
> The value that should be stored is the receiver window; it should not be
> scaled for SYN segments.
> As you can see, m_rWnd is stored but not scaled; m_rWnd << m_sndWindShift
> does not scale m_rWnd, but store the scaled value inside m_initialSsThresh.

Yes, but I am questioning why you are scaling a value here that was not intended to be scaled.  Why not set to m_rWnd directly?  Is it because m_rWnd may not be the true maximum receive window if window scaling is not yet being used?

> > Elsewhere in the code, rWnd is stored
> > as the scaled value, so I don't see why you store it differently initially.
> 

I think the above part of my comment was wrong, however.

(In reply to Tom Henderson from comment #4)
> I was asking about where it says to reduce it to the receive window, vs.
> just leaving it very high.  I thought that it was only to be reduced upon
> detection of congestion.  Your comment suggests that this is due to graphing
> concerns.  

It does not say "reduce it to the receiver window" but it seems to me that the RFC suggest, as initial value, the real rWnd, rather than UINT32_MAX. To me, UINT32_MAX or rWnd are perfectly the same.

> 
> Setting it to zero may lead to confusion that it is disabled; e.g. see
> https://monolight.cc/2010/12/increasing-tcp-initial-congestion-window/

This article contains a big error, I don't think that it sets the ssth to 0 anywhere in the code. Anyway, in tcp.h there is a define, which sets infinite ssth at 0x7ffffff (http://lxr.free-electrons.com/source/include/net/tcp.h#L1002) and it is initialized here (http://lxr.free-electrons.com/source/net/ipv4/tcp_metrics.c#L515).

Do you think the best is to use that value ? It can be removed manually from graphs.

Created attachment 2248 [details]
Removed shift to m_rWnd as ssThresh

changeset 11829:4c4c70d7aa0c