Message-ID: <CANn89iLYd90nPph6PqxiC5KJt0LYgTtHyU0FmTCPUK_9_iWT4A@mail.gmail.com>
Date: Mon, 30 Sep 2024 18:24:02 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Neal Cardwell <ncardwell@...gle.com>
Cc: Mingrui Zhang <mrzhang97@...il.com>, davem@...emloft.net, 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 Wed, Aug 28, 2024 at 10:32 PM Neal Cardwell <ncardwell@...gle.com> wrote:
>
> On Mon, Aug 26, 2024 at 5:26 AM Eric Dumazet <edumazet@...gle.com> wrote:
> ...
> > I prefer you rebase your patch after mine is merged.
> >
> > There is a common misconception with jiffies.
> > It can change in less than 20 nsec.
> > Assuming that delta(jiffies) == 1 means that 1ms has elapsed is plain wrong.
> > In the old days, linux TCP only could rely on jiffies and we had to
> > accept its limits.
> > We now can switch to high resolution clocks, without extra costs,
> > because we already cache in tcp->tcp_mstamp
> > the usec timestamp for the current time.
> >
> > Some distros are using CONFIG_HZ_250=y or CONFIG_HZ_100=y, this means
> > current logic in cubic is more fuzzy for them.
> >
> > Without ca->last_time conversion to jiffies, your patch would still be
> > limited to jiffies resolution:
> > usecs_to_jiffies(ca->delay_min) would round up to much bigger values
> > for DC communications.
>
> Even given Eric's excellent point that is raised above, that an
> increase of jiffies by one can happen even though only O(us) or less
> may have elapsed, AFAICT the patch should be fine in practice.
>
> The patch says:
>
> +       /* 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)))
>                 return;
>
> So, basically, we only run fall through and try to run the core of
> bictcp_update() if cwnd has increased since ca-> last_cwnd, or
> tcp_jiffies32 has increased by more than
> min_t(s32, HZ / 32, usecs_to_jiffies(ca->delay_min)) since ca->last_time.
>
> AFAICT  this works out OK because the logic is looking for "more than"
> and usecs_to_jiffies() rounds up. That means that in the
> interesting/tricky/common case where ca->delay_min is less than a
> jiffy, usecs_to_jiffies(ca->delay_min) will return 1 jiffy. That means
> that in this case we will only fall through and try to run the core of
> bictcp_update() if cwnd has increased since ca-> last_cwnd, or
> tcp_jiffies32 has increased by more than 1 jiffy (i.e., 2 or more
> jiffies).
>
> AFAICT the fact that this check is triggering only if tcp_jiffies32
> has increased by 2 or more means that  at least one full jiffy has
> elapsed between when we set ca->last_time and the time when this check
> triggers running the core of bictcp_update().
>
> So AFAICT this logic is not tricked by the fact that a single
> increment of tcp_jiffies32 can happen over O(us) or less.
>
> At first glance it may sound like if the RTT is much less than a
> jiffy, many RTTs could elapse before we run the core of
> bictcp_update(). However,  AFAIK if the RTT is much less than a jiffy
> then CUBIC is very likely in Reno mode, and so is very likely to
> increase cwnd by roughly 1 packet per round trip (the behavior of
> Reno), so the (ca->last_cwnd == cwnd) condition should fail roughly
> once per round trip and allow recomputation of the ca->cnt slope.
>
> So AFAICT this patch should be OK in practice.
>
> Given those considerations, Eric, do you think it would be OK to
> accept the patch as-is?
>

Ok, what about updating net/ipv4/tcp_bic.c at the same time ?

Powered by blists - more mailing lists