| Message ID | 20250114043131.2035-1-ma.arghavani@yahoo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tcp_cubic: fix incorrect HyStart round start detection | expand |
On Mon, Jan 13, 2025 at 11:31 PM Mahdi Arghavani <ma.arghavani@yahoo.com> wrote: > > 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") > 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@yahoo.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> > --- Thanks for the patch! To comply with Linux commit description policy, please remove the empty line between the Fixes: footer and Signed-off-by footer, so tools and readers can be sure to correctly parse all the footers. Note that this page: https://patchwork.kernel.org/project/netdevbpf/patch/20250114043131.2035-1-ma.arghavani@yahoo.com/ ...highlights that issue in the "netdev/verify_fixesfailProblems with Fixes tag: 1" row: https://netdev.bots.linux.dev/static/nipa/925140/13938399/verify_fixes/summary Commit: 08181a887689 ("tcp_cubic: fix incorrect HyStart round start detection") Fixes tag: Fixes: 4e1fddc98d25 ("tcp_cubic: fix spurious Hystart ACK train detections for not-cwnd-limited flows") Has these problem(s): - empty lines surround the Fixes tag To help maintainers understand the relationship between your next post of the patch, and the current one, can you please use the following to apply a "v2" in your git format-patch command line: --subject-prefix='PATCH net v2' Also, can you please either (a) respond in the other email thread on this topic, attaching your updated packetdrill tests, or (b) post a pull request on the packetdrill github repo at https://github.com/google/packetdrill with your updated tests? thanks, neal > 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 >
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, 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") 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@yahoo.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(-)