Message-ID: <865255fd30cd4339966425ea1b1bd8f9@amazon.com>
Date:   Wed, 11 Jan 2023 08:58:46 +0000
From:   "Arinzon, David" <darinzon@...zon.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Machulsky, Zorik" <zorik@...zon.com>,
        "Matushevsky, Alexander" <matua@...zon.com>,
        "Bshara, Saeed" <saeedb@...zon.com>,
        "Bshara, Nafea" <nafea@...zon.com>,
        "Saidi, Ali" <alisaidi@...zon.com>,
        "Kiyanovski, Arthur" <akiyano@...zon.com>,
        "Dagan, Noam" <ndagan@...zon.com>,
        "Agroskin, Shay" <shayagr@...zon.com>,
        "Itzko, Shahar" <itzko@...zon.com>,
        "Abboud, Osama" <osamaabb@...zon.com>
Subject: RE: [PATCH V1 net-next 0/5] Add devlink support to ena

> On Tue, 10 Jan 2023 20:11:23 +0000 Arinzon, David wrote:
> > > On Sun, 8 Jan 2023 10:35:28 +0000 David Arinzon wrote:
> > > > This patchset adds devlink support to the ena driver.
> > >
> > > Wrong place, please take a look at
> > >
> > >         struct kernel_ethtool_ringparam::tx_push
> > >
> > > and ETHTOOL_A_RINGS_TX_PUSH. I think you just want to configure
> the
> > > max size of the TX push, right?
> >
> > We're not configuring the max size of the TX push, but effectively the
> > maximal packet header size to be pushed to the device.
> > This is noted in the documentation on patch 5/5 in this patchset.
> > AFAIK, there's no relevant ethtool parameter for this configuration.
> 
> Perhaps I should have complained about the low quality of that
> documentation to make it clear that I have in fact read it :/
> 

Noted, we will see how to improve documentation going forward.

> I read it again - and I still don't know what you're doing.
> I sounds like inline header length configuration yet you also use LLQ all
> over the place. And LLQ for ENA is documented as basically tx_push:
> 
>   - **Low Latency Queue (LLQ) mode or "push-mode":**
> 
> Please explain this in a way which assumes zero Amazon-specific
> knowledge :(
> 

Low Latency Queues (LLQ) is a mode of operation where the packet headers
(up to a defined length) are being written directly to the device memory.
Therefore, you are right, the description is similar to tx_push. However,
This is not a configurable option while ETHTOOL_A_RINGS_TX_PUSH
configures whether to work in a mode or not.
If I'm understanding the intent behind ETHTOOL_A_RINGS_TX_PUSH
and the implementation in the driver that introduced the feature, it
refers to a push of the packet and not just the headers, which is not what
the ena driver does.

In this patchset, we allow the configuration of an extended size of the
Low Latency Queue, meaning, allow enabled another, larger, pre-defined
size to be used as a max size of the packet header to be pushed directly to
device memory. It is not configurable in value, therefore, it was defined as
large LLQ.

I hope this provides more clarification, if not, I'll be happy to elaborate further.

> > > The reload is also an overkill, reload should re-register all driver
> > > objects but the devlink instance, IIRC. You're not even unregistering
> the netdev.
> > > You should handle this change the same way you handle any ring size
> > > changes.
> >
> > The LLQ configuration is different from other configurations set via
> > ethtool (like queue size and number of queues). LLQ requires
> > re-negotiation with the device and requires a reset, which is not
> > performed in the ethtool configurations case.
> 
> What do you mean when you say that reset is not required in the ethool
> configuration case?
> 
> AFAIK ethtool config should not (although sadly very often it does) cause
> any loss of unrelated configuration. But you can certainly reset HW blocks
> or reneg features with FW or whatever else...
> 

The ena driver currently supports various configurations via ethtool,
for example, changing the number of queues/channels and the
queue/channel size. These options, for example, require changes in the
netdev and the interfaces with the kernel, therefore, we perform a
reconfiguration on this level. But, these configurations do not require
the interface between the driver and device to be reset.
Low Latency Queue mode change from standard to large (what's being
configured in this pathset) is more significant functionality change and
requires this reset.

> > It may be possible to unregister/register the netdev, but it is
> > unnecessary in this case, as most of the changes are reflected in the
> > interface and structures between the driver and the device.
> >
> > > For future reference - if you ever _actually_ need devlink please
> > > use the
> > > devl_* APIs and take the instance locks explicitly. There has not
> > > been a single devlink reload implementation which would get locking
> > > right using the devlink_* APIs 😔️
> >
> > This operation can happen in parallel to a reset of the device from a
> > different context which is unrelated to devlink. Our intention is to
> > avoid such cases, therefore, holding the devlink lock using devl_lock APIs
> will not be sufficient.
> > The driver holds the RTNL_LOCK in key places, either explicitly or
> > implicitly, as in ethtool configuration changes for example.
> 
> Yeah, which is why you should not be using devlink for this.

Though using ethtool would've been more convenient as RTNL_LOCK is being
held while the command takes place, this functionality is specific to Amazon and
adding a private flag, for example, is not desirable. Creating a new ethtool or
updating an existing one would be specific only for Amazon as well.
Per our understanding, ethtool is used for interface operations, while devlink
is used for device operations. Performing the discussed configuration change
is a device configuration, as it affects the way the device configures its own memory.

Powered by blists - more mailing lists