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: <CANn89i+yoe=GJXUO57V84WM3FHqQBOKsvEN3+9cdp_UKKbT4Mw@mail.gmail.com>
Date: Tue, 20 Aug 2024 14:53:13 +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 Mon, Aug 19, 2024 at 10:36 PM Mingrui Zhang <mrzhang97@...il.com> wrote:
>
> On 8/19/24 04:00, Eric Dumazet wrote:
> > 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...
> >
> I may misunderstood the use of Fixes tag and choose the latest commit of that line.
>
> Shall it supposed to be the very first commit with that behavior?
> That is, the very first commit (df3271f3361b ("[TCP] BIC: CUBIC window growth (2.0)")) when the code was first introduced?

I was referring to this line : Fixes: ac35f562203a ("tcp: bic, cubic:
use tcp_jiffies32 instead of tcp_time_stamp")

Commit ac35f562203a did not change the behavior at all.

I see no particular reason to mention it, this is confusing.


> >> 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.
> Thank you for the suggestion, however, I may need more time to discuss with another author for this revision. :)
> Thank you

No problem, there is no hurry.