Re: [PATCH net-next 1/3] ethtool: add tx aggregation parameters

From: Daniele Palmas <dnlplm@gmail.com>
To: Gal Pressman <gal@nvidia.com>
Cc: "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: Mon, 14 Nov 2022 11:06:19 +0100	[thread overview]
Message-ID: <CAGRyCJF49NMTt9aqPhF_Yp5T3cof_GtL7+v8PeowsBQWG0bkJQ@mail.gmail.com> (raw)
In-Reply-To: <8b0aba42-627a-f5f5-a9ec-237b69b3b03f@nvidia.com>

Hello Gal,

Il giorno dom 13 nov 2022 alle ore 10:48 Gal Pressman <gal@nvidia.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.

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