| 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
| ||
|
Message-ID: <CABdStnNL4ipEfT4xiJXH=4bmyOdRdD4n2AzF31TreDKMyOnhHA@mail.gmail.com>
Date: Tue, 27 Aug 2024 09:14:41 -0500
From: Lisong Xu <lisongxu@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, Neal Cardwell <ncardwell@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"eric.dumazet@...il.com" <eric.dumazet@...il.com>, Mingrui Zhang <mrzhang97@...il.com>,
Yuchung Cheng <ycheng@...gle.com>, Lisong Xu <xu@....edu>
Subject: Re: [PATCH net] tcp_cubic: switch ca->last_time to usec resolution
> ________________________________________
> From: Eric Dumazet <edumazet@...gle.com>
> Sent: Monday, August 26, 2024 12:27 PM
> To: Neal Cardwell
> Cc: David S . Miller; Jakub Kicinski; Paolo Abeni; netdev@...r.kernel.org; eric.dumazet@...il.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 <ncardwell@...gle.com> wrote:
> >
> > On Mon, Aug 26, 2024 at 5:27 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > 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@...gle.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 <mrzhang97@...il.com>
> > > Cc: Lisong Xu <xu@....edu>
> > > ---
> > > 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;
How about something like the following? basically, we change
ca->epoch_start also to usec.
ca->epoch_start = tp->tcp_mstamp;
...
t = (s32)(tp->tcp_mstamp - ca->epoch_start);
t += ca->delay_min;
/* first change the unit from usec to msec to avoid overflow*/
do_div(t, USEC_PER_MSEC);
/* then change the unit from msec to bictcp_HZ */
t <<= BICTCP_HZ;
do_div(t, MSEC_PER_SEC);
Powered by blists - more mailing lists