| 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: <CABdStnN1SYyuEb56CFEckWeY7gYg8Dag3bcD8Ez1NKFCyn1U+Q@mail.gmail.com>
Date: Tue, 27 Aug 2024 09:34:12 -0500
From: Lisong Xu <lisongxu@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Neal Cardwell <ncardwell@...gle.com>, "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
On Tue, Aug 27, 2024 at 9:19 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
>
>
> On Tue, Aug 27, 2024 at 4:14 PM Lisong Xu <lisongxu@...il.com> wrote:
>>
>> > ________________________________________
>> > 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);
>
>
> Changing ca->epoch_start would require more changes, say in cubictcp_cwnd_event().
>
> This probably should be done in a separate patch.
>
>
Yes, it requires more changes, as ca->epoch_start is used also in
several other methods.
Powered by blists - more mailing lists