Re: [PATCH V2] tcp: add window scaling and RTTM support

[Josef Bacik <jbacik@fb.com>]

On 02/01/2016 03:43 AM, Andrei Borzenkov wrote:
> On Sat, Jan 30, 2016 at 12:48 AM, Josef Bacik <jbacik@fb.com> wrote:
>> Sometimes we have to provision boxes across regions, such as California to
>> Sweden.  The http server has a 10 minute timeout, so if we can't get our 250mb
>> image transferred fast enough our provisioning fails, which is not ideal.  So
>> add tcp window scaling on open connections and set the window size to 1mb.  With
>> this change we're able to get higher sustained transfers between regions and can
>> transfer our image in well below 10 minutes.  Without this patch we'd time out
>> every time halfway through the transfer.
>>
>> RTTM is needed in order to make window scaling work well under heavy congestion
>> or packet loss.  In most cases grub could recover with just window scaling
>> enabled, but on some machines the congestion would be so high that it would
>> never recover and would timeout.
>>
>> I've made the window size configureable with the grub env variable
>> "tcp_window_size".  By default this is set to 1mb but can be configured to
>> whatever a user wants, and we will calculate the appropriate window size and
>> scale settings.  Thanks,
>>

<snip>

>> @@ -906,6 +1027,8 @@ grub_net_recv_tcp_packet (struct grub_net_buff *nb,
>>                return err;
>>              }
>>
>> +         /* We only update the tsecr when we advance the window. */
>> +         sock->cur_tsecr = tsecr;
>
> As far as I can tell, this does not agree with algorithm proposed in
> RFC 7323. It recommends that TSecr be advanced on immediate segment
> arrival and only if TSval is greater than previous one. I read it that
> TSecr advance should be done before queue processing, probably at the
> spot where we look for TS option.
>
> (2)  If:
>
>              SEG.TSval >= TS.Recent and SEG.SEQ <= Last.ACK.sent
>
>          then SEG.TSval is copied to TS.Recent; otherwise, it is ignored.
>
> and
>
>     It is important to note that the timestamp MUST be checked only when
>     a segment first arrives at the receiver, regardless of whether it is
>     in sequence or it must be queued for later delivery.

So if you look further down it shows an example of what I've 
implemented, where we get packets out of order.  We only increment 
TS.Recent (which in our case is sock->cur_tsecr) when we've gotten the 
next segment that we are expecting, which is what that bit

(2)  If:

              SEG.TSval >= TS.Recent and SEG.SEQ <= Last.ACK.sent

is talking about.  Look at the page 17 for the example, I was super 
confused about this as well until I saw the example they gave.  I 
probably should check for SEG.TSval >= TS.Recent before resetting it, so 
I'll do that.

>
> Note that it also recommends that if TS option was negotiated, packets
> without TS option should be discarded. Not sure if we need to follow
> it.
>

I'll go ahead and do this just to be on the safe side.  Thanks,

Josef