Re: [PATCH v4 net-next 1/5] ethtool: Add support for configuring tx_push_buf_len

From: Gal Pressman <gal@nvidia.com>
To: Shay Agroskin <shayagr@amazon.com>, Jakub Kicinski <kuba@kernel.org>
Cc: David Miller <davem@davemloft.net>,
	netdev@vger.kernel.org, "Woodhouse, David" <dwmw@amazon.com>,
	"Machulsky, Zorik" <zorik@amazon.com>,
	"Matushevsky, Alexander" <matua@amazon.com>,
	Saeed Bshara <saeedb@amazon.com>, "Wilson, Matt" <msw@amazon.com>,
	"Liguori, Anthony" <aliguori@amazon.com>,
	"Bshara, Nafea" <nafea@amazon.com>,
	"Belgazal, Netanel" <netanel@amazon.com>,
	"Saidi, Ali" <alisaidi@amazon.com>,
	"Herrenschmidt, Benjamin" <benh@amazon.com>,
	"Kiyanovski, Arthur" <akiyano@amazon.com>,
	"Dagan, Noam" <ndagan@amazon.com>,
	"Arinzon, David" <darinzon@amazon.com>,
	"Itzko, Shahar" <itzko@amazon.com>,
	"Abboud, Osama" <osamaabb@amazon.com>
Subject: Re: [PATCH v4 net-next 1/5] ethtool: Add support for configuring tx_push_buf_len
Date: Thu, 16 Mar 2023 13:57:26 +0200	[thread overview]
Message-ID: <30cb3990-80ce-ca07-6d73-cdc00d0ad7b8@nvidia.com> (raw)
In-Reply-To: <pj41zlbkkv2v6z.fsf@u570694869fb251.ant.amazon.com>

On 14/03/2023 17:38, Shay Agroskin wrote:
> 
> Jakub Kicinski <kuba@kernel.org> writes:
> 
>> CAUTION: This email originated from outside of the organization. Do
>> not click links or open attachments unless you can confirm the sender
>> and know the content is safe.
>>
>>
>>
>> On Sun, 12 Mar 2023 14:41:39 +0200 Gal Pressman wrote:
>>> On 10/03/2023 8:53, Jakub Kicinski wrote:
>>> > On Thu, 9 Mar 2023 19:15:43 +0200 Gal Pressman wrote:
>>> >> I know Jakub prefers the new parameter, but the description >> of
>>> this
>>> >> still sounds extremely similar to TX copybreak to me..
>>> >> TX copybreak was traditionally used to copy packets to >>
>>> preallocated DMA
>>> >> buffers, but this could be implemented as copying the packet >> to
>>> the
>>> >> (preallocated) WQE's inline part. That usually means DMA >>
>>> memory, but
>>> >> could also be device memory in this ENA LLQ case.
>>> >>
>>> >> Are we drawing a line that TX copybreak is the threshold for >>
>>> DMA memory
>>> >> and tx_push_buf_len is the threshold for device memory?
>>> >
>>> > Pretty much, yes. Not an amazing distinction but since TX >
>>> copybreak can
>>> > already mean two different things (inline or DMA buf) I'd err > on
>>> > the side of not overloading it with another one.
>>>
>>> Can we document that please?
>>
>> Shay, could you add a paragraph in the docs regarding copybreak in v5?
> 
> Document that tx_copybreak defines the threshold below which the packet
> is copied into a preallocated DMA'ed buffer and that tx_push_buf defines
> the same but for device memory?
> Are we sure we want to make this distinction ? While the meaning of both
> params can overlap in their current definition, the motivation to use
> them is pretty different.
> A driver can implement both for different purposes (and still copy both
> into the device).

I don't understand what it means to implement both.

It's confusing because both parameters result in skipping the DMA map
operation, but their usage motivation is different?
What are we instructing our customers? Use copybreak when your iommu is
slow, but when should they use this new parameter?

IMO, a reasonable way to use both would be to only account for the
tx_push_buf_len when tx_push is enabled, otherwise use copybreak.