netdev - Re: [PATCH net v2] tcp_cubic: fix incorrect HyStart round start detection

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] [day] [month] [year] [list]
Message-ID: <CADVnQymHGF=X_b32Vfp+ZtpNf+t535Lw0vqnxO4M20Z-0bWqEw@mail.gmail.com>
Date: Thu, 16 Jan 2025 12:03:22 -0500
From: Neal Cardwell <ncardwell@...gle.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jason Xing <kerneljasonxing@...il.com>, Mahdi Arghavani <ma.arghavani@...oo.com>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"haibo.zhang@...go.ac.nz" <haibo.zhang@...go.ac.nz>, 
	"david.eyers@...go.ac.nz" <david.eyers@...go.ac.nz>, "abbas.arghavani@....se" <abbas.arghavani@....se>
Subject: Re: [PATCH net v2] tcp_cubic: fix incorrect HyStart round start detection

On Thu, Jan 16, 2025 at 10:46 AM Neal Cardwell <ncardwell@...gle.com> wrote:
>
> On Thu, Jan 16, 2025 at 10:30 AM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Thu, Jan 16, 2025 at 3:42 PM Neal Cardwell <ncardwell@...gle.com> wrote:
> > >
> > > On Thu, Jan 16, 2025 at 6:40 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > > >
> > > > On Thu, Jan 16, 2025 at 5:49 PM Mahdi Arghavani <ma.arghavani@...oo.com> wrote:
> > > > >
> > > > > Hi Jason,
> > > > >
> > > > > I will explain this using a test conducted on my local testbed. Imagine a client and a server connected through two Linux software routers. In this setup, the minimum RTT is 150 ms, the bottleneck bandwidth is 50 Mbps, and the bottleneck buffer size is 1 BDP, calculated as (50M / 1514 / 8) * 0.150 = 619 packets.
> > > > >
> > > > > I conducted the test twice, transferring data from the server to the client for 1.5 seconds:
> > > > >
> > > > > TEST 1) With the patch applied: HyStart stopped the exponential growth of cwnd when cwnd = 632 and the bottleneck link was saturated (632 > 619).
> > > > >
> > > > >
> > > > > TEST 2) Without the patch applied: HyStart stopped the exponential growth of cwnd when cwnd = 516 and the bottleneck link was not yet saturated (516 < 619). This resulted in 300 KB less delivered data compared to the first test.
> > > >
> > > > Thanks for sharing these numbers. I would suggest in the v3 adding the
> > > > above description in the commit message. No need to send v3 until the
> > > > maintainers of TCP (Eric and Neal) give further suggestions :)
> > > >
> > > > Feel free to add my reviewed-by tag in the next version:
> > > > Reviewed-by: Jason Xing <kerneljasonxing@...il.com>
> > > >
> > > > Thanks,
> > > > Jason
> > >
> > > Mahdi, a few quick questions about your test logs, beforePatch.log and
> > > afterPatch.log:
> > >
> > > + What is moRTT? Is that ca->curr_rtt? It would be great to share the
> > > debug patch you used, so we know for certain how to interpret each
> > > column in the debug output.
> >
> > +1
> >
> > Debug patches can alone add delays...
> >
> >
> >
> > >
> > > + Are both HYSTART-DELAY and HYSTART-ACK-TRAIN enabled for both of those tests?
>
> I like Jason's suggestion to include your test results in the v3
> commit message. Please also include a phrase about which Hystart
> mechanism triggered in each case.
>
> Perhaps something like the following, just above the footers (with an
> empty line separating this text and the following footers):
>
> ---
> I tested with a client and a server connected through two Linux
> software routers. In this setup, the minimum RTT is 150 ms, the
> bottleneck bandwidth is 50 Mbps, and the bottleneck buffer size is 1
> BDP, calculated as (50M / 1514 / 8) * 0.150 = 619 packets.
>
> I conducted the test twice, transferring data from the server to the
> client for 1.5 seconds:
>
> Before: Without the patch applied: HYSTART-DELAY stopped the
> exponential growth of cwnd when cwnd = 516 and the bottleneck link was
> not yet saturated (516 < 619). This resulted in 300 KB less delivered
> data compared to the first test.
>
> After: With the patch applied: HYSTART-ACK-TRAIN stopped the
> exponential growth of cwnd when cwnd = 632 and the bottleneck link was
> saturated (632 > 619).
> ---
>
> (with all lines wrapped in a way that makes scripts/checkpatch.pl happy...)
>
> Eric mentioned:
> > Debug patches can alone add delays...
>
> Yes, this is a good point. To avoid introducing delays, you probably
> want to run your emulation test without the debug patch, and simply
> use "nstat -n" before the test and "nstat" after the test to see (a)
> the type of Hystart mechanism that triggered (b) the cwnd at which it
> triggered.
>
> (I'm still interested in your debug patch so we know for certain how
> to interpret your previously shared log files.)
>
> Mahdi, your tcp_cubic.c patch looks OK to me, so I'm running your
> patch and tests through our kernel test pipeline to see what we get.

Your tcp_cubic.c patch passes our internal packetdrill test suite,
with your updated/added packetdrill scripts, and one additional
reasonable tweak for an internal test that is not upstream yet (in
which the flow now exits slowstart with a cwnd of 28 instead of 24).

So I would suggest sending the v3 version of the patch with the tweaks
we have suggested above for the commit message. And hopefully we can
get a consensus to merge the v3 version of the patch, if we get a
consensus that we like the v3 commit message.

thanks,
neal