| Message ID | 20250117213751.2404-1-ma.arghavani@yahoo.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] tcp_cubic: fix incorrect HyStart round start detection | expand |
On Fri, Jan 17, 2025 at 5:00 PM Eric Dumazet <edumazet@google.com> wrote: > > On Fri, Jan 17, 2025 at 10:38 PM Mahdi Arghavani <ma.arghavani@yahoo.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@yahoo.com> > > Reviewed-by: Jason Xing <kerneljasonxing@gmail.com> > > Cc: Neal Cardwell <ncardwell@google.com> > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Haibo Zhang <haibo.zhang@otago.ac.nz> > > Cc: David Eyers <david.eyers@otago.ac.nz> > > Cc: Abbas Arghavani <abbas.arghavani@mdu.se> > > --- > > SGTM thanks. > > Reviewed-by: Eric Dumazet <edumazet@google.com> LGTM as well. Thanks for the fix! Reviewed-by: Neal Cardwell <ncardwell@google.com> Tested-by: Neal Cardwell <ncardwell@google.com> 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); }
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@yahoo.com> Reviewed-by: Jason Xing <kerneljasonxing@gmail.com> Cc: Neal Cardwell <ncardwell@google.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Haibo Zhang <haibo.zhang@otago.ac.nz> Cc: David Eyers <david.eyers@otago.ac.nz> Cc: Abbas Arghavani <abbas.arghavani@mdu.se> --- net/ipv4/tcp_cubic.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)