mbox series

[net-next,0/7] net: ethtool: Introduce ethnl dump helpers

Message ID 20250305141938.319282-1-maxime.chevallier@bootlin.com (mailing list archive)
Headers show
Series net: ethtool: Introduce ethnl dump helpers | expand

Message

Maxime Chevallier March 5, 2025, 2:19 p.m. UTC
Hi everyone,

This series adds some scaffolding into ethnl to ease the support of
DUMP operations.

As of today when using ethnl's default ops, the DUMP requests will
simply perform a GET for each netdev.

That hits limitations for commands that may return multiple messages for
a single netdev, such as :

 - RSS (listing contexts)
 - All PHY-specific commands (PLCA, PSE-PD, phy)
 - tsinfo (one item for the netdev +  one per phy)

 Commands that need a non-default DUMP support have to re-implement
 ->dumpit() themselves, which prevents using most of ethnl's internal
 circuitry.

This series therefore introduces a better support for dump operations in
ethnl.

The patches 1 and 2 introduce the support for filtered DUMPs, where an
ifindex/ifname can be passed in the request header for the DUMP
operation. This is for when we want to dump everything a netdev
supports, but without doing so for every single netdev. ethtool's
"--show-phys ethX" option for example performs a filtered dump.

Patch 3 introduces 3 new ethnl ops : 
 ->dump_start() to initialize a dump context
 ->dump_one_dev(), that can be implemented per-command to dump
 everything on a given netdev
 ->dump_done() to release the context

The default behaviour for dumps remains the same, calling the whole
->doit() path for each netdev.

Patch 4 introduces a set of ->dump_start(), ->dump_one_dev() and
->dump_done() callback implementations that can simply be plugged into
the existing commands that list objects per-phy, making the 
phy-targeting command behaviour more coherent.

Patch 5 uses that new set of helpers to rewrite the phy.c support, which
now uses the regulat ethnl_ops instead of fully custom genl ops. This
one is the hardest to review, sorry about that, I couldn't really manage
to incrementally rework that file :(

Patches 6 and 7 are where the new dump infra shines, adding per-netdev
per-phy dump support for PLCA and PSE-PD.

We could also consider converting tsinfo/tsconfig, rss and tunnels to
these new ->dump_***() operations as well, but that's out of this
series' scope.

I've tested that series with some netdevsim PHY patches that I plan to
submit (they can be found here [1]), with the refcount tracker
for net/netns enabled to make sure the lock usage is somewhat coherent.

Thanks,

Maxime

[1]: https://github.com/minimaxwell/linux/tree/mc/netdevsim-phy



Maxime Chevallier (7):
  net: ethtool: netlink: Allow per-netdevice DUMP operations
  net: ethtool: netlink: Rename ethnl_default_dump_one
  net: ethtool: netlink: Introduce command-specific dump_one_dev
  net: ethtool: netlink: Introduce per-phy DUMP helpers
  net: ethtool: phy: Convert the PHY_GET command to generic phy dump
  net: ethtool: plca: Use per-PHY DUMP operations
  net: ethtool: pse-pd: Use per-PHY DUMP operations

 net/ethtool/netlink.c | 161 ++++++++++++++------
 net/ethtool/netlink.h |  46 +++++-
 net/ethtool/phy.c     | 335 ++++++++++++------------------------------
 net/ethtool/plca.c    |  12 ++
 net/ethtool/pse-pd.c  |   6 +
 5 files changed, 277 insertions(+), 283 deletions(-)

Comments

Maxime Chevallier March 5, 2025, 5:02 p.m. UTC | #1
On Wed,  5 Mar 2025 15:19:30 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hi everyone,
> 
> This series adds some scaffolding into ethnl to ease the support of
> DUMP operations.
> 
> As of today when using ethnl's default ops, the DUMP requests will
> simply perform a GET for each netdev.
> 
> That hits limitations for commands that may return multiple messages for
> a single netdev, such as :
> 
>  - RSS (listing contexts)
>  - All PHY-specific commands (PLCA, PSE-PD, phy)
>  - tsinfo (one item for the netdev +  one per phy)
> 
>  Commands that need a non-default DUMP support have to re-implement
>  ->dumpit() themselves, which prevents using most of ethnl's internal  
>  circuitry.
> 
> This series therefore introduces a better support for dump operations in
> ethnl.
> 
> The patches 1 and 2 introduce the support for filtered DUMPs, where an
> ifindex/ifname can be passed in the request header for the DUMP
> operation. This is for when we want to dump everything a netdev
> supports, but without doing so for every single netdev. ethtool's
> "--show-phys ethX" option for example performs a filtered dump.
> 
> Patch 3 introduces 3 new ethnl ops : 
>  ->dump_start() to initialize a dump context
>  ->dump_one_dev(), that can be implemented per-command to dump  
>  everything on a given netdev
>  ->dump_done() to release the context  
> 
> The default behaviour for dumps remains the same, calling the whole
> ->doit() path for each netdev.  
> 
> Patch 4 introduces a set of ->dump_start(), ->dump_one_dev() and
> ->dump_done() callback implementations that can simply be plugged into  
> the existing commands that list objects per-phy, making the 
> phy-targeting command behaviour more coherent.
> 
> Patch 5 uses that new set of helpers to rewrite the phy.c support, which
> now uses the regulat ethnl_ops instead of fully custom genl ops. This
> one is the hardest to review, sorry about that, I couldn't really manage
> to incrementally rework that file :(
> 
> Patches 6 and 7 are where the new dump infra shines, adding per-netdev
> per-phy dump support for PLCA and PSE-PD.
> 
> We could also consider converting tsinfo/tsconfig, rss and tunnels to
> these new ->dump_***() operations as well, but that's out of this
> series' scope.
> 
> I've tested that series with some netdevsim PHY patches that I plan to
> submit (they can be found here [1]), with the refcount tracker
> for net/netns enabled to make sure the lock usage is somewhat coherent.
> 
> Thanks,
> 
> Maxime
> 
> [1]: https://github.com/minimaxwell/linux/tree/mc/netdevsim-phy
> 

This series will very likely conflict with Stanislav's netdev lock
work [2], I'll of course be happy to rebase should it get merged :)

Thanks,

Maxime

[2]: https://lore.kernel.org/netdev/20250305163732.2766420-1-sdf@fomichev.me/T/#t

> 
> Maxime Chevallier (7):
>   net: ethtool: netlink: Allow per-netdevice DUMP operations
>   net: ethtool: netlink: Rename ethnl_default_dump_one
>   net: ethtool: netlink: Introduce command-specific dump_one_dev
>   net: ethtool: netlink: Introduce per-phy DUMP helpers
>   net: ethtool: phy: Convert the PHY_GET command to generic phy dump
>   net: ethtool: plca: Use per-PHY DUMP operations
>   net: ethtool: pse-pd: Use per-PHY DUMP operations
> 
>  net/ethtool/netlink.c | 161 ++++++++++++++------
>  net/ethtool/netlink.h |  46 +++++-
>  net/ethtool/phy.c     | 335 ++++++++++++------------------------------
>  net/ethtool/plca.c    |  12 ++
>  net/ethtool/pse-pd.c  |   6 +
>  5 files changed, 277 insertions(+), 283 deletions(-)
>
Jakub Kicinski March 6, 2025, 2:47 a.m. UTC | #2
On Wed, 5 Mar 2025 18:02:52 +0100 Maxime Chevallier wrote:
> This series will very likely conflict with Stanislav's netdev lock
> work [2], I'll of course be happy to rebase should it get merged :)

Also this doesn't build. Please hold off on reposting for a couple of
days - because of the large conflict radius the previous few revisions
of the locking series haven't even gotten into the selftest CI queue :(
Maxime Chevallier March 6, 2025, 7:42 a.m. UTC | #3
On Wed, 5 Mar 2025 18:47:49 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 5 Mar 2025 18:02:52 +0100 Maxime Chevallier wrote:
> > This series will very likely conflict with Stanislav's netdev lock
> > work [2], I'll of course be happy to rebase should it get merged :)  
> 
> Also this doesn't build. Please hold off on reposting for a couple of
> days - because of the large conflict radius the previous few revisions
> of the locking series haven't even gotten into the selftest CI queue :(

Sure no problem. I'll try t figure out what's up with the build in the
meantime, looks like I inverted a few changes that break
bissecatability.

I'll keep an eye on Stanislav's work in the meantime as well.

Thanks,

Maxime