netdev - RE: [PATCH V1 net-next 0/5] Add devlink support to ena

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <574f532839dd4e93834dbfc776059245@amazon.com>
Date:   Tue, 10 Jan 2023 20:11:23 +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 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?
> 

Hi Jakub,
Thank you for the feedbacks.

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.

> 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. 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.