netdev - Re: [PATCH net-next] tcp: apply a floor of 1 for RTT samples from TCP timestamps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADVnQym04+QQU3WZ+qgSycZ_2TWJGwChN_jN1ZY_t65fxuWL-A@mail.gmail.com>
Date:   Thu, 30 Jul 2020 22:36:55 -0400
From:   Neal Cardwell <ncardwell@...gle.com>
To:     Jianfeng Wang <jfwang@...gle.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Kevin Yang <yyd@...gle.com>, Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH net-next] tcp: apply a floor of 1 for RTT samples from TCP timestamps

On Thu, Jul 30, 2020 at 7:53 PM Jianfeng Wang <jfwang@...gle.com> wrote:
>
> For retransmitted packets, TCP needs to resort to using TCP timestamps
> for computing RTT samples. In the common case where the data and ACK
> fall in the same 1-millisecond interval, TCP senders with millisecond-
> granularity TCP timestamps compute a ca_rtt_us of 0. This ca_rtt_us
> of 0 propagates to rs->rtt_us.
>
> This value of 0 can cause performance problems for congestion control
> modules. For example, in BBR, the zero min_rtt sample can bring the
> min_rtt and BDP estimate down to 0, reduce snd_cwnd and result in a
> low throughput. It would be hard to mitigate this with filtering in
> the congestion control module, because the proper floor to apply would
> depend on the method of RTT sampling (using timestamp options or
> internally-saved transmission timestamps).
>
> This fix applies a floor of 1 for the RTT sample delta from TCP
> timestamps, so that seq_rtt_us, ca_rtt_us, and rs->rtt_us will be at
> least 1 * (USEC_PER_SEC / TCP_TS_HZ).
>
> Note that the receiver RTT computation in tcp_rcv_rtt_measure() and
> min_rtt computation in tcp_update_rtt_min() both already apply a floor
> of 1 timestamp tick, so this commit makes the code more consistent in
> avoiding this edge case of a value of 0.
>
> Signed-off-by: Jianfeng Wang <jfwang@...gle.com>
> Signed-off-by: Neal Cardwell <ncardwell@...gle.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Acked-by: Kevin Yang <yyd@...gle.com>
> Acked-by: Yuchung Cheng <ycheng@...gle.com>
> ---

One extra note on this patch: IMHO this is a bug fix that is worth
backporting to stable releases. Normally we would submit a patch like
this to the net branch, but we submitted this to the net-next branch
since Eric advised that this was the best approach, given how late it
is in the v5.8 development cycle.

Apologies that a note to this effect is not in the commit message itself.

best,
neal