netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniele Palmas <dnlplm@gmail.com>
To: Dave Taht <dave.taht@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 12:51:13 +0100	[thread overview]
Message-ID: <CAGRyCJFG0kybDzwYrdj2-Y868KbePCVBxFXsOo5TTJ_4PwrQDQ@mail.gmail.com> (raw)
In-Reply-To: <CAA93jw4OAWRAg+BxftuMgFaHex+BAeV3bS5JUYU7_+pM8ZOaEA@mail.gmail.com>

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 :-)

> 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.

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

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).

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

  reply	other threads:[~2022-11-15 11:57 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 [this message]
2022-11-15 15:27             ` Dave Taht
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=CAGRyCJFG0kybDzwYrdj2-Y868KbePCVBxFXsOo5TTJ_4PwrQDQ@mail.gmail.com \
    --to=dnlplm@gmail.com \
    --cc=bjorn@mork.no \
    --cc=corbet@lwn.net \
    --cc=dave.taht@gmail.com \
    --cc=davem@davemloft.net \
    --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).