Re: [PATCH net v2 1/2] tcp: process the 3rd ACK with sk_socket for TFO/MPTCP

[Matthieu Baerts <matttbe@kernel.org>]

Hi Neal,

On 24/07/2024 00:01, Neal Cardwell wrote:
> On Tue, Jul 23, 2024 at 3:09 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>>
>> Hi Eric,
>>
>> On 23/07/2024 18:42, Eric Dumazet wrote:
>>> On Tue, Jul 23, 2024 at 6:08 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>>>>
>>>> Hi Eric,
>>>>
>>>> On 23/07/2024 17:38, Eric Dumazet wrote:
>>>>> On Tue, Jul 23, 2024 at 4:58 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>>>>>>
>>>>>> Hi Eric,
>>>>>>
>>>>>> +cc Neal
>>>>>> -cc Jerry (NoSuchUser)
>>>>>>
>>>>>> On 23/07/2024 16:37, Eric Dumazet wrote:
>>>>>>> On Thu, Jul 18, 2024 at 12:34 PM Matthieu Baerts (NGI0)
>>>>>>> <matttbe@kernel.org> wrote:
>>>>>>>>
> ...
>>>>> +.045 < S. 1234:1234(0) ack 1001 win 14600 <mss
>>>>> 940,nop,nop,sackOK,nop,wscale 6,FO 12345678,nop,nop>
>>>>>    +0 > . 1001:1001(0) ack 1 <nop,nop,sack 0:1>  // See here
>>>>
>>>> I'm sorry, but is it normal to have 'ack 1' with 'sack 0:1' here?
>>>
>>> It is normal, because the SYN was already received/processed.
>>>
>>> sack 0:1 represents this SYN sequence.
>>
>> Thank you for your reply!
>>
>> Maybe it is just me, but does it not look strange to have the SACK
>> covering a segment (0:1) that is before the ACK (1)?
>>
>> 'ack 1' and 'sack 0:1' seem to cover the same block, no?
>> Before Kuniyuki's patch, this 'sack 0:1' was not present.
> 
> A SACK that covers a sequence range that was already cumulatively or
> selectively acknowledged is legal and useful, and is called a
> Duplicate Selective Acknowledgement (DSACK or D-SACK).
> 
> A DSACK indicates that a receiver received duplicate data. That can be
> very useful in allowing a data sender to determine that a
> retransmission was not needed (spurious). If a data sender receives
> DSACKs for all retransmitted data in a loss detection episode then it
> knows all retransmissions were spurious, and thus it can "undo" its
> congestion control reaction to that estimated loss, since the
> congestion control algorithm was responding to an incorrect loss
> signal. This can be very helpful for performance in the presence of
> varying delays or reordering, which can cause spurious loss detection
> episodes..
> 
> See:
> 
> https://datatracker.ietf.org/doc/html/rfc2883
> An Extension to the Selective Acknowledgement (SACK) Option for TCP
> 
> https://www.rfc-editor.org/rfc/rfc3708.html
> "Using TCP Duplicate Selective Acknowledgement (DSACKs) and Stream
> Control Transmission Protocol (SCTP) Duplicate  Transmission Sequence
> Numbers (TSNs) to Detect Spurious Retransmissions"

Thank you for the great explanations!

I was not expecting this in a 3rd ACK :)
I don't know if it will help the congestion control algorithm in this
case, but I guess we don't need to worry about this marginal case.

I will then send the v3, and the Packetdrill modification.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.