netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave Taht <dave.taht@gmail.com>
To: Daniele Palmas <dnlplm@gmail.com>
Cc: "Gal Pressman" <gal@nvidia.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"David Miller" <davem@davemloft.net>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Eric Dumazet" <edumazet@google.com>,
	"Subash Abhinov Kasiviswanathan" <quic_subashab@quicinc.com>,
	"Sean Tranchetti" <quic_stranche@quicinc.com>,
	"Jonathan Corbet" <corbet@lwn.net>, "Bjørn Mork" <bjorn@mork.no>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	netdev@vger.kernel.org
Subject: Re: [PATCH net-next 1/3] ethtool: add tx aggregation parameters
Date: Tue, 15 Nov 2022 07:27:05 -0800	[thread overview]
Message-ID: <CAA93jw5oQTGEiBanFn_hZZCPkXTP4XP6mj=v88wkQYykhjDUFA@mail.gmail.com> (raw)
In-Reply-To: <CAGRyCJFG0kybDzwYrdj2-Y868KbePCVBxFXsOo5TTJ_4PwrQDQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 5626 bytes --]

On Tue, Nov 15, 2022 at 3:57 AM Daniele Palmas <dnlplm@gmail.com> wrote:
>
> Hello Dave,
>
> Il giorno lun 14 nov 2022 alle ore 11:46 Dave Taht
> <dave.taht@gmail.com> ha scritto:
> > > Tx packets aggregation allows to overcome this issue, so that a single
> > > URB holds N qmap packets, reducing URBs frequency.
> >
> > While I understand the use case... it's generally been my hope we got
> > to a BQL-like mechanism for
> > 4G and 5G that keeps the latency under control. Right now, that can be
> > really, really, really miserable -
> > measured in *seconds* - and adding in packet aggregation naively is
> > what messed up Wifi for the
> > past decade. Please lose 8 minutes of your life to this (hilarious)
> > explanation of why aggregation can be bad.
> >
> > https://www.youtube.com/watch?v=Rb-UnHDw02o&t=1560s
> >
>
> Nice video and really instructive :-)

Thank you. I wish more folk working on the wifi7 firmware had exposed
per station queues so they could have been gang scheduled by linux for
the RU facility.

I targeted a more recent video (with the help of the MIT juggling
club!) at a wider audience.

https://www.youtube.com/watch?v=TWViGcBlnm0&t=120s

> > So given a choice between being able to drive the modem at the maximum
> > rate in a testbed...
> > or having it behave well at all possible (and highly variable) egress
> > rates, I would so love for more to focus on the latter problem than
> > the former, at whatever levels and layers in the stack it takes.
> >
>
> I get your point, but here it's not just a testbed issue, since I
> think that the huge tx drop due to a concurrent rx can happen also in
> real life scenarios.
>
> Additionally, it seems that Qualcomm modems are meant to be used in
> this way: as far as I know all QC downstream kernel versions have this
> kind of feature in the rmnet code.

"feature" is not exactly the word I'd use...

>
> I think that this can be seen as adding one more choice for the user:
> by default tx aggregation in rmnet would be disabled, so no one should
> notice this change and suffer from latencies different than the ones
> the current rmnet driver already has.
>
> But for those that are affected by the same bug I'm facing or are
> interested in a different use-case in which tx aggregation makes
> sense, this feature can help.
>
> Hope that this makes sense.
>
> > As a test, what happens on the flent "rrul" test, before and after
> > this patch? Under good wireless conditions, and bad?
> >
> > flent -H server -t my-test-conditions -x --socket-stats rrul
> > flent -H server -t my-test-conditions -x --socket-stats
> > --test-parameter=upload_streams=4 tcp_nup
> >
> > I have servers for that all over the world
> > {de,london,fremont,dallas,singapore,toronto,}.starlink.taht.net
> >
>
> I've uploaded some results at
> https://drive.google.com/drive/folders/1-HjhyJaN4oWRNv8P8C__KD9-V-IoBwbL?usp=sharing

Thank you very much for performing those tests. Under bad conditions,
without tx aggregation, network delays grew
on the rrul test to over 15 seconds, and with tx aggregation, to 25
seconds. Since most of our protocols tend to break
at more than 1 second of induced latency, I do hope more can lean in
to find ways of resolving these problems.

Under good conditions, your result was almost reasonable - only 150ms
of induced delay on the rrul test, and aggregation worked a lot better
than non-aggregation. That said, over here are the results we
currently get out of some forms of fq_codel for wifi, at any rate,
only 8ms, and 40ms with others:
https://forum.openwrt.org/t/reducing-multiplexing-latencies-still-further-in-wifi/133605/42

Btw, you can plot these tests using the flent-gui various ways. I will
attach a couple in the hope they get through
this listserv.

We've had a team for two years now, trying to make 5G (and starlink)
behave better, leveraging active measurements
and cake. There are three very usable scripts here:
https://forum.openwrt.org/t/cake-w-adaptive-bandwidth/135379/2 that
while targetted at openwrt, also work on generic linux.

I'd love to know what happens for you on those. Something more elegant
would be nice.

> The good network condition has been simulated through a callbox
> connected to LAN (there are also a few pictures of the throughput on
> the callbox side while performing the tests with tx aggregation
> enabled/disabled).

It doesn't look like your software measures the same things flent does.

>
> Thanks,
> Daniele
>
> > > The maximum number of allowed packets in a single URB and the maximum
> > > size of the URB are dictated by the modem through the qmi control
> > > protocol: the values returned by the modem are then configured in the
> > > driver with the new ethtool parameters.
> > >
> > > > Isn't this the same as TX copybreak? TX
> > > > copybreak for multiple packets?
> > >
> > > I tried looking at how tx copybreak works to understand your comment,
> > > but I could not find any useful document. Probably my fault, but can
> > > you please point me to something I can read?
> > >
> > > Thanks,
> > > Daniele
> >
> >
> >
> > --
> > This song goes out to all the folk that thought Stadia would work:
> > https://www.linkedin.com/posts/dtaht_the-mushroom-song-activity-6981366665607352320-FXtz
> > Dave Täht CEO, TekLibre, LLC



-- 
This song goes out to all the folk that thought Stadia would work:
https://www.linkedin.com/posts/dtaht_the-mushroom-song-activity-6981366665607352320-FXtz
Dave Täht CEO, TekLibre, LLC

[-- Attachment #2: rrul_-_good_tx_aggr.png --]
[-- Type: image/png, Size: 281857 bytes --]

[-- Attachment #3: rrul_-_bad_tx_aggr.png --]
[-- Type: image/png, Size: 232329 bytes --]

  reply	other threads:[~2022-11-15 15:27 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-09 18:02 [PATCH net-next 0/3] add tx packets aggregation to ethtool and rmnet Daniele Palmas
2022-11-09 18:02 ` [PATCH net-next 1/3] ethtool: add tx aggregation parameters Daniele Palmas
2022-11-11 17:07   ` Jakub Kicinski
2022-11-11 21:51     ` Daniele Palmas
2022-11-13  9:48     ` Gal Pressman
2022-11-14 10:06       ` Daniele Palmas
2022-11-14 10:45         ` Dave Taht
2022-11-15 11:51           ` Daniele Palmas
2022-11-15 15:27             ` Dave Taht [this message]
2022-11-15  0:42         ` Jakub Kicinski
2022-11-15 10:59           ` Gal Pressman
2022-11-15 16:21             ` Jakub Kicinski
2022-11-09 18:02 ` [PATCH net-next 2/3] net: qualcomm: rmnet: add tx packets aggregation Daniele Palmas
2022-11-10 17:32   ` Alexander Lobakin
2022-11-11  1:17     ` Subash Abhinov Kasiviswanathan (KS)
2022-11-11 17:11       ` Jakub Kicinski
2022-11-11 22:00       ` Daniele Palmas
2022-11-14  8:48         ` Daniele Palmas
2022-11-11 17:23     ` Daniele Palmas
2022-11-16 15:19     ` Daniele Palmas
2022-11-16 16:20       ` Alexander Lobakin
2022-11-20  9:25         ` Daniele Palmas
2022-11-20  9:39           ` Bjørn Mork
2022-11-20  9:52             ` Daniele Palmas
2022-11-20 17:48               ` Subash Abhinov Kasiviswanathan (KS)
2022-11-21  7:00                 ` Daniele Palmas
2022-11-24  3:32                   ` Subash Abhinov Kasiviswanathan (KS)
2022-11-10 19:09   ` kernel test robot
2022-11-11 17:14   ` Jakub Kicinski
2022-11-14  9:13     ` Daniele Palmas
2022-11-14 10:25       ` Bjørn Mork
2022-11-15  0:43         ` Jakub Kicinski
2022-11-09 18:02 ` [PATCH net-next 3/3] net: qualcomm: rmnet: add ethtool support for configuring tx aggregation Daniele Palmas
2022-11-11  6:46   ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAA93jw5oQTGEiBanFn_hZZCPkXTP4XP6mj=v88wkQYykhjDUFA@mail.gmail.com' \
    --to=dave.taht@gmail.com \
    --cc=bjorn@mork.no \
    --cc=corbet@lwn.net \
    --cc=davem@davemloft.net \
    --cc=dnlplm@gmail.com \
    --cc=edumazet@google.com \
    --cc=gal@nvidia.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=quic_stranche@quicinc.com \
    --cc=quic_subashab@quicinc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).