netdev - Re: [PATCH net] Fixes: 4e1fddc98d25 ("tcp_cubic: fix spurious Hystart ACK train detections for not-cwnd-limited flows")

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] [day] [month] [year] [list]

Message-ID: <CADVnQy=AhO3gUMQ2NcVWzK-9bW13DfsXWf7-s9TktSR4wryYHQ@mail.gmail.com>
Date: Mon, 13 Jan 2025 09:22:12 -0500
From: Neal Cardwell <ncardwell@...gle.com>
To: Mahdi Arghavani <ma.arghavani@...oo.com>
Cc: netdev@...r.kernel.org, edumazet@...gle.com, haibo.zhang@...go.ac.nz, 
	david.eyers@...go.ac.nz, abbas.arghavani@....se
Subject: Re: [PATCH net] Fixes: 4e1fddc98d25 ("tcp_cubic: fix spurious Hystart
 ACK train detections for not-cwnd-limited flows")

On Sun, Jan 12, 2025 at 11:11 PM Mahdi Arghavani <ma.arghavani@...oo.com> wrote:

Thanks for sending this patch.

Regarding the proposed commit title:

   Fixes: 4e1fddc98d25 ("tcp_cubic: fix spurious Hystart ACK train
detections for not-cwnd-limited flows")

Please move this Fixes: part to be the first footer, just before your
Signed-off-by: footer:

Fixes: 4e1fddc98d25 ("tcp_cubic: fix spurious Hystart ACK train
detections for not-cwnd-limited flows")

For a patch title, please consider something like:

   tcp_cubic: fix end_seq to update for all cwnd values

Please use "git log" to take a look at example commits in the Linux
history to get a sense of how footers are used.

> I noticed that HyStart incorrectly marks the start of rounds,
> resulting in inaccurate measurements of ACK train lengths.
> Since HyStart relies on ACK train lengths as one of two thresholds
> to terminate exponential cwnd growth during Slow-Start, this
> inaccuracy renders that threshold ineffective, potentially degrading
> TCP performance.
>
> The issue arises because the changes introduced in commit
> 4e1fddc98d25 ("tcp_cubic: fix spurious Hystart ACK train detections for not-cwnd-limited flows")
> move the caller of the `bictcp_hystart_reset` function inside the `hystart_update` function.
>
> This modification adds 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`.

Please combine the previous two paragraphs so that it is more clear
that "this modification" refers to 4e1fddc98d25 rather than your
commit. And please reword this as "This modification added", to make
it clear that the modification was in the past.

> The proposed 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.

Please change "The proposed fix" to "This fix", since if/when this is
merged, the commit will live on forever in git history and, for most
folks who read this, it will be an actual or accepted fix rather than
just a "proposed" fix.

> 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>
> ---
>  Makefile             | 2 +-
>  net/ipv4/tcp_cubic.c | 8 +++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 7904d5d88088..e20a62ad397f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2,7 +2,7 @@
>  VERSION = 6
>  PATCHLEVEL = 13
>  SUBLEVEL = 0
> -EXTRAVERSION = -rc6
> +EXTRAVERSION = -rc7
>  NAME = Baby Opossum Posse
>
>  # *DOCUMENTATION*

Please remove this accidental unrelated change from your patch.

thanks,
neal


> 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
>