netdev - Re: [PATCH net v2] tcp_cubic: fix incorrect HyStart round start detection

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: <CAL+tcoAtzuGZ8US5NcJwJdBh29afYGZHpeMJ4jXgiSnWBr75Ww@mail.gmail.com>
Date: Thu, 16 Jan 2025 15:19:37 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Mahdi Arghavani <ma.arghavani@...oo.com>
Cc: netdev@...r.kernel.org, ncardwell@...gle.com, edumazet@...gle.com, 
	haibo.zhang@...go.ac.nz, david.eyers@...go.ac.nz, abbas.arghavani@....se
Subject: Re: [PATCH net v2] tcp_cubic: fix incorrect HyStart round start detection

...

> potentially degrading TCP performance.

Interesting point, I try a few times but don't see the degradation of
performance actually based on the limited experiments I conducted.

>
> The issue arises because the changes introduced in commit 4e1fddc98d25
> ("tcp_cubic: fix spurious Hystart ACK train detections for not-cwnd-limited flows")
> moved the caller of the `bictcp_hystart_reset` function inside the `hystart_update` function.
> This modification added an additional condition for triggering the caller,
> requiring that (tcp_snd_cwnd(tp) >= hystart_low_window) must also
> be satisfied before invoking `bictcp_hystart_reset`.
>
> This fix ensures that `bictcp_hystart_reset` is correctly called
> at the start of a new round, regardless of the congestion window size.
> This is achieved by moving the condition
> (tcp_snd_cwnd(tp) >= hystart_low_window)
> from before calling `bictcp_hystart_reset` to after it.
>
> Fixes: 4e1fddc98d25 ("tcp_cubic: fix spurious Hystart ACK train detections for not-cwnd-limited flows")
> Signed-off-by: Mahdi Arghavani <ma.arghavani@...oo.com>
> Cc: Neal Cardwell <ncardwell@...gle.com>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Haibo Zhang <haibo.zhang@...go.ac.nz>
> Cc: David Eyers <david.eyers@...go.ac.nz>
> Cc: Abbas Arghavani <abbas.arghavani@....se>

As to the patch itself and corresponding theory, it looks good to me
according to my limited understanding of the hystart algorithm :)

After this, we can accurately reset at the beginning of each round
instead of waiting cwnd to reach 16.

Note that tests about big tcp like in the commit 4e1fddc98d25 are not
running on my machine.

> ---
>  net/ipv4/tcp_cubic.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
> index 5dbed91c6178..76c23675ae50 100644
> --- a/net/ipv4/tcp_cubic.c
> +++ b/net/ipv4/tcp_cubic.c
> @@ -392,6 +392,10 @@ static void hystart_update(struct sock *sk, u32 delay)
>         if (after(tp->snd_una, ca->end_seq))
>                 bictcp_hystart_reset(sk);
>
> +       /* hystart triggers when cwnd is larger than some threshold */
> +       if (tcp_snd_cwnd(tp) < hystart_low_window)
> +               return;
> +
>         if (hystart_detect & HYSTART_ACK_TRAIN) {
>                 u32 now = bictcp_clock_us(sk);
>
> @@ -467,9 +471,7 @@ __bpf_kfunc static void cubictcp_acked(struct sock *sk, const struct ack_sample
>         if (ca->delay_min == 0 || ca->delay_min > delay)
>                 ca->delay_min = delay;
>
> -       /* hystart triggers when cwnd is larger than some threshold */
> -       if (!ca->found && tcp_in_slow_start(tp) && hystart &&
> -           tcp_snd_cwnd(tp) >= hystart_low_window)
> +       if (!ca->found && tcp_in_slow_start(tp) && hystart)
>                 hystart_update(sk, delay);
>  }
>
> --
> 2.45.2
>
>