Date:   Mon, 14 Nov 2022 02:45:53 -0800
From:   Dave Taht <dave.taht@...il.com>
To:     Daniele Palmas <dnlplm@...il.com>
Cc:     Gal Pressman <gal@...dia.com>, Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>,
        Sean Tranchetti <quic_stranche@...cinc.com>,
        Jonathan Corbet <corbet@....net>,
        Bjørn Mork <bjorn@...k.no>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] ethtool: add tx aggregation parameters

On Mon, Nov 14, 2022 at 2:31 AM Daniele Palmas <dnlplm@...il.com> wrote:
>
> Hello Gal,
>
> Il giorno dom 13 nov 2022 alle ore 10:48 Gal Pressman <gal@...dia.com>
> ha scritto:
> >
> > On 11/11/2022 19:07, Jakub Kicinski wrote:
> > > On Wed,  9 Nov 2022 19:02:47 +0100 Daniele Palmas wrote:
> > >> Add the following ethtool tx aggregation parameters:
> > >>
> > >> ETHTOOL_A_COALESCE_TX_MAX_AGGR_SIZE
> > >> Maximum size of an aggregated block of frames in tx.
> > > perhaps s/size/bytes/ ? Or just mention bytes in the doc? I think it's
> > > the first argument in coalescing expressed in bytes, so to avoid
> > > confusion we should state that clearly.
> > >
> > >> ETHTOOL_A_COALESCE_TX_MAX_AGGR_FRAMES
> > >> Maximum number of frames that can be aggregated into a block.
> > >>
> > >> ETHTOOL_A_COALESCE_TX_USECS_AGGR_TIME
> > >> Time in usecs after the first packet arrival in an aggregated
> > >> block for the block to be sent.
> > > Can we add this info to the ethtool-netlink.rst doc?
> > >
> > > Can we also add a couple of sentences describing what aggregation is?
> > > Something about copying the packets into a contiguous buffer to submit
> > > as one large IO operation, usually found on USB adapters?
> > >
> > > People with very different device needs will read this and may pattern
> > > match the parameters to something completely different like just
> > > delaying ringing the doorbell. So even if things seem obvious they are
> > > worth documenting.
> >
> > Seems like I am these people, I was under the impression this is kind of
> > a threshold for xmit more or something?
> > What is this contiguous buffer?
>
> I would like to explain the issue I'm trying to solve.
>
> I'm using an USB modem that is driven by qmi_wwan which creates a
> netdevice: on top of this the rmnet module creates another netdevice,
> needed since packets sent to the modem needs to follow the qmap
> protocol both for multiplexing and performance.
>
> Without tx packets aggregation each tx packet sent to the rmnet
> netdevice makes an URB to be sent through qmi_wwan/usbnet, so that
> there is a 1:1 relation between a qmap packet and an URB.
>
> So far, this has not been an issue, but I've run into a family of
> modems for which this behavior does not work well, preventing the
> modem from reaching the maximum throughput both in rx and tx (an
> example of the issue is described at
> https://lore.kernel.org/netdev/CAGRyCJEkTHpLVsD9zTzSQp8d98SBM24nyqq-HA0jbvHUre+C4g@mail.gmail.com/
> )
>
> 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

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.

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

> 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

Powered by blists - more mailing lists