netdev - Re: [PATCH net v3] 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: <CADVnQyn-T_3r2_M9M03qPvvyeRgX2GP543x89SeDrDmLWPJAcQ@mail.gmail.com>
Date: Fri, 17 Jan 2025 18:11:15 -0500
From: Neal Cardwell <ncardwell@...gle.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Mahdi Arghavani <ma.arghavani@...oo.com>, netdev@...r.kernel.org, haibo.zhang@...go.ac.nz, 
	david.eyers@...go.ac.nz, abbas.arghavani@....se, 
	Jason Xing <kerneljasonxing@...il.com>
Subject: Re: [PATCH net v3] tcp_cubic: fix incorrect HyStart round start detection

On Fri, Jan 17, 2025 at 5:00 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Fri, Jan 17, 2025 at 10:38 PM Mahdi Arghavani <ma.arghavani@...oo.com> wrote:
> >
> > I noticed that HyStart incorrectly marks the start of rounds,
> > leading to inaccurate measurements of ACK train lengths and
> > resetting the `ca->sample_cnt` variable. This inaccuracy can impact
> > HyStart's functionality in terminating exponential cwnd growth during
> > Slow-Start, 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")
> > 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.
> >
> > I tested with a client and a server connected through two Linux software routers.
> > In this setup, the minimum RTT was 150 ms, the bottleneck bandwidth was 50 Mbps,
> > and the bottleneck buffer size was 1 BDP, calculated as (50M / 1514 / 8) * 0.150 = 619 packets.
> > I conducted the test twice, transferring data from the server to the client for 1.5 seconds.
> > Before the patch was applied, HYSTART-DELAY stopped the exponential growth of cwnd when
> > cwnd = 516, and the bottleneck link was not yet saturated (516 < 619).
> > After the patch was applied, HYSTART-ACK-TRAIN stopped the exponential growth of cwnd when
> > cwnd = 632, and the bottleneck link was saturated (632 > 619).
> > In this test, applying the patch resulted in 300 KB more data delivered.
> >
> > Fixes: 4e1fddc98d25 ("tcp_cubic: fix spurious Hystart ACK train detections for not-cwnd-limited flows")
> > Signed-off-by: Mahdi Arghavani <ma.arghavani@...oo.com>
> > Reviewed-by: Jason Xing <kerneljasonxing@...il.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>
> > ---
>
> SGTM thanks.
>
> Reviewed-by: Eric Dumazet <edumazet@...gle.com>

LGTM as well. Thanks for the fix!

Reviewed-by: Neal Cardwell <ncardwell@...gle.com>
Tested-by: Neal Cardwell <ncardwell@...gle.com>

neal