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

From: Jakub Kicinski <kuba@kernel.org>
To: Daniele Palmas <dnlplm@gmail.com>
Cc: "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: Fri, 11 Nov 2022 09:07:20 -0800	[thread overview]
Message-ID: <20221111090720.278326d1@kernel.org> (raw)
In-Reply-To: <20221109180249.4721-2-dnlplm@gmail.com>

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.

> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index d578b8bcd8a4..a6f115867648 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -1001,6 +1001,9 @@ Kernel response contents:
>    ``ETHTOOL_A_COALESCE_RATE_SAMPLE_INTERVAL``  u32     rate sampling interval
>    ``ETHTOOL_A_COALESCE_USE_CQE_TX``            bool    timer reset mode, Tx
>    ``ETHTOOL_A_COALESCE_USE_CQE_RX``            bool    timer reset mode, Rx
> +  ``ETHTOOL_A_COALESCE_TX_MAX_AGGR_SIZE``      u32     max aggr packets size, Tx
> +  ``ETHTOOL_A_COALESCE_TX_MAX_AGGR_FRAMES``    u32     max aggr packets, Tx
> +  ``ETHTOOL_A_COALESCE_TX_USECS_AGGR_TIME``    u32     time (us), aggr pkts, Tx

nit: perhaps move _aggr before the specifics? e.g.

 ETHTOOL_A_COALESCE_TX_AGGR_MAX_SIZE
 ETHTOOL_A_COALESCE_TX_AGGR_USECS_TIME

FWIW I find that the easiest way to do whole-sale renames in a series
is to generate the patches as .patch files, run sed on those, and apply
them back on a fresh branch. Rebasing is a PITA with renames.

Other then these nit picks - looks very reasonable :)