netdev - Re: [PATCH net v4 1/3] tcp_cubic: fix to run bictcp

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: <CANn89iKwN8vCH4Dx0mYvLJexWEmz5TWkfvCFnxmqKGgTTzeraQ@mail.gmail.com>
Date: Mon, 19 Aug 2024 11:00:42 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Mingrui Zhang <mrzhang97@...il.com>
Cc: davem@...emloft.net, ncardwell@...gle.com, netdev@...r.kernel.org, 
	Lisong Xu <xu@....edu>
Subject: Re: [PATCH net v4 1/3] tcp_cubic: fix to run bictcp_update() at least
 once per RTT

On Sat, Aug 17, 2024 at 6:35 PM Mingrui Zhang <mrzhang97@...il.com> wrote:
>
> The original code bypasses bictcp_update() under certain conditions
> to reduce the CPU overhead. Intuitively, when last_cwnd==cwnd,
> bictcp_update() is executed 32 times per second. As a result,
> it is possible that bictcp_update() is not executed for several
> RTTs when RTT is short (specifically < 1/32 second = 31 ms and
> last_cwnd==cwnd which may happen in small-BDP networks),
> thus leading to low throughput in these RTTs.
>
> The patched code executes bictcp_update() 32 times per second
> if RTT > 31 ms or every RTT if RTT < 31 ms, when last_cwnd==cwnd.
>
> Fixes: df3271f3361b ("[TCP] BIC: CUBIC window growth (2.0)")
> Fixes: ac35f562203a ("tcp: bic, cubic: use tcp_jiffies32 instead of tcp_time_stamp")

I do not understand this Fixes: tag ?

Commit  ac35f562203a was essentially a nop at that time...


> Signed-off-by: Mingrui Zhang <mrzhang97@...il.com>
> Signed-off-by: Lisong Xu <xu@....edu>
> ---
> v3->v4: Replace min() with min_t()
> v2->v3: Correct the "Fixes:" footer content
> v1->v2: Separate patches
>
>  net/ipv4/tcp_cubic.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
> index 5dbed91c6178..00da7d592032 100644
> --- a/net/ipv4/tcp_cubic.c
> +++ b/net/ipv4/tcp_cubic.c
> @@ -218,8 +218,12 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
>
>         ca->ack_cnt += acked;   /* count the number of ACKed packets */
>
> +       /* Update 32 times per second if RTT > 1/32 second,
> +        * or every RTT if RTT < 1/32 second even when last_cwnd == cwnd
> +        */
>         if (ca->last_cwnd == cwnd &&
> -           (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32)
> +           (s32)(tcp_jiffies32 - ca->last_time) <=
> +           min_t(s32, HZ / 32, usecs_to_jiffies(ca->delay_min)))

This looks convoluted to me and still limited if HZ=250 (some distros
still use 250 jiffies per second :/ )

I would suggest switching to usec right away.


diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index 5dbed91c6178257df8d2ccd1c8690a10bdbaf56a..fae000a57bf7d3803c5dd854af64b6933c4e26dd
100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -211,26 +211,27 @@ static u32 cubic_root(u64 a)
 /*
  * Compute congestion window to use.
  */
-static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
+static inline void bictcp_update(struct tcp_sock *tp, struct bictcp *ca,
+                                u32 cwnd, u32 acked)
 {
        u32 delta, bic_target, max_cnt;
        u64 offs, t;

        ca->ack_cnt += acked;   /* count the number of ACKed packets */

-       if (ca->last_cwnd == cwnd &&
-           (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32)
+       delta = tp->tcp_mstamp - ca->last_time;
+       if (ca->last_cwnd == cwnd && delta <= ca->delay_min)
                return;

-       /* The CUBIC function can update ca->cnt at most once per jiffy.
+       /* The CUBIC function can update ca->cnt at most once per ms.
         * On all cwnd reduction events, ca->epoch_start is set to 0,
         * which will force a recalculation of ca->cnt.
         */
-       if (ca->epoch_start && tcp_jiffies32 == ca->last_time)
+       if (ca->epoch_start && delta < USEC_PER_MSEC)
                goto tcp_friendliness;

        ca->last_cwnd = cwnd;
-       ca->last_time = tcp_jiffies32;
+       ca->last_time = tp->tcp_mstamp;

        if (ca->epoch_start == 0) {
                ca->epoch_start = tcp_jiffies32;        /* record beginning */
@@ -334,7 +335,7 @@ __bpf_kfunc static void cubictcp_cong_avoid(struct
sock *sk, u32 ack, u32 acked)
                if (!acked)
                        return;
        }
-       bictcp_update(ca, tcp_snd_cwnd(tp), acked);
+       bictcp_update(tp, ca, tcp_snd_cwnd(tp), acked);
        tcp_cong_avoid_ai(tp, ca->cnt, acked);
 }