Re: [PATCH net] tcp_cubic: switch ca->last_time to usec resolution

________________________________________
From: Eric Dumazet <edumazet@google.com>
Sent: Monday, August 26, 2024 12:27 PM
To: Neal Cardwell
Cc: David S . Miller; Jakub Kicinski; Paolo Abeni; netdev@vger.kernel.org; eric.dumazet@gmail.com; Mingrui Zhang; Lisong Xu; Yuchung Cheng
Subject: Re: [PATCH net] tcp_cubic: switch ca->last_time to usec resolution

Caution: Non-NU Email


On Mon, Aug 26, 2024 at 3:26 PM Neal Cardwell [off-list ref] wrote:

quoted

On Mon, Aug 26, 2024 at 5:27 AM Eric Dumazet [off-list ref] wrote:

quoted

bictcp_update() uses ca->last_time as a timestamp
to decide of several heuristics.

Historically this timestamp has been fed with jiffies,
which has too coarse resolution, some distros are
still using CONFIG_HZ_250=y

It is time to switch to usec resolution, now TCP stack
already caches in tp->tcp_mstamp the high resolution time.

Also remove the 'inline' qualifier, this helper is used
once and compilers are smarts.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Link: https://urldefense.com/v3/__https://lore.kernel.org/netdev/20240817163400.2616134-1-mrzhang97@gmail.com/T/*mb6a64c9e2309eb98eaeeeb4b085c4a2270b6789d__;Iw!!PvXuogZ4sRB2p-tU!CFiPa78hSiQJz2pJVSL2IbKk_L0o9RGTCO29eeo32aJX9_1V_QyTFP9gxuafoE_Ye7DtfD-LhjMf$
Cc: Mingrui Zhang <redacted>
Cc: Lisong Xu <redacted>
---
 net/ipv4/tcp_cubic.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index 5dbed91c6178257df8d2ccd1c8690a10bdbaf56a..3b1845103ee1866a316926a130c212e6f5e78ef0 100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c

@@ -87,7 +87,7 @@ struct bictcp {
        u32     cnt;            /* increase cwnd by 1 after ACKs */
        u32     last_max_cwnd;  /* last maximum snd_cwnd */
        u32     last_cwnd;      /* the last snd_cwnd */
-       u32     last_time;      /* time when updated last_cwnd */
+       u32     last_time;      /* time when updated last_cwnd (usec) */
        u32     bic_origin_point;/* origin point of bic function */
        u32     bic_K;          /* time to origin point
                                   from the beginning of the current epoch */

@@ -211,26 +211,28 @@ static u32 cubic_root(u64 a)
 /*
  * Compute congestion window to use.
  */
-static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
+static void bictcp_update(struct sock *sk, u32 cwnd, u32 acked)
 {
+       const struct tcp_sock *tp = tcp_sk(sk);
+       struct bictcp *ca = inet_csk_ca(sk);
        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 <= USEC_PER_SEC / 32)
                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;

AFAICT there is a problem here. It is switching this line of code to
use microsecond resolution without also changing the core CUBIC slope
(ca->cnt) calculation to also use microseconds.  AFAICT that means we
would be re-introducing the bug that was fixed in 2015 in
d6b1a8a92a1417f8859a6937d2e6ffe2dfab4e6d (see below). Basically, if
the CUBIC slope (ca->cnt) calculation uses jiffies, then we should
only run that code once per jiffy, to avoid getting the wrong answer
for the slope:

Interesting.... would adding the following part deal with this
problem, or is it something else ?

diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index 3b1845103ee1866a316926a130c212e6f5e78ef0..bff5688ba5109fa5a0bbff7dc529525b2752dc46

100644

--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c

@@ -268,9 +268,10 @@ static void bictcp_update(struct sock *sk, u32

cwnd, u32 acked)

        t = (s32)(tcp_jiffies32 - ca->epoch_start);
        t += usecs_to_jiffies(ca->delay_min);
-       /* change the unit from HZ to bictcp_HZ */
+       t = jiffies_to_msecs(t);
+       /* change the unit from ms to bictcp_HZ */
        t <<= BICTCP_HZ;
-       do_div(t, HZ);
+       do_div(t, MSEC_PER_SEC);

        if (t < ca->bic_K)              /* t - K */
                offs = ca->bic_K - t;