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

Message

Maxime Chevallier March 24, 2025, 10:40 a.m. UTC

Hi everyone,

This is V4 for the ethnl dump support, allowing better handling of
per-phy dump but also any other dump operation that needs to dump more
than one message per netdev.

Changes in V4:
 - Don't grab rcu_read_lock when we already have a refcounter netdev on
   the filtered dump path (Paolo)
 - Move the dump_all stuff in a dedicated helper (Paolo)
 - Added patch 1 to set the dev in ctx->req_info

Changes in V3:
 - Fixed some typos and xmas tree issues
 - Added a missing check for EOPNOTSUPP in patch 1
 - Added missing kdoc
 - Added missing comments in phy_reply_size

Changes in V2:
 - Rebased on the netdev_lock work by Stanislav and the fixes from Eric
 - Fixed a bissectability issue
 - Fixed kdoc for the new ethnl ops and fields

V1: https://lore.kernel.org/netdev/[e-mail]/
V2: https://lore.kernel.org/netdev/[e-mail]/
V3: https://lore.kernel.org/netdev/[e-mail]/

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.

Thanks,

Maxime



Maxime Chevallier (8):
  net: ethtool: Set the req_info->dev on DUMP requests for each dev
  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 | 185 ++++++++++++++++++-----
 net/ethtool/netlink.h |  47 +++++-
 net/ethtool/phy.c     | 344 ++++++++++++------------------------------
 net/ethtool/plca.c    |  12 ++
 net/ethtool/pse-pd.c  |   6 +
 5 files changed, 310 insertions(+), 284 deletions(-)

Comments

Jakub Kicinski March 25, 2025, 9:31 p.m. UTC | #1

On Mon, 24 Mar 2025 11:40:02 +0100 Maxime Chevallier wrote:
> 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  

Did you consider ignoring the RSS and focusing purely on PHYs?
The 3 callbacks are a bit generic, but we end up primarily
using them for PHY stuff. And the implementations still need 
to call ethnl_req_get_phydev() AFAICT

Maxime Chevallier March 26, 2025, 7:52 a.m. UTC | #2

Hi Jakub,

On Tue, 25 Mar 2025 14:31:11 -0700
Jakub Kicinski <[e-mail]> wrote:

> On Mon, 24 Mar 2025 11:40:02 +0100 Maxime Chevallier wrote:
> > 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    
> 
> Did you consider ignoring the RSS and focusing purely on PHYs?
> The 3 callbacks are a bit generic, but we end up primarily
> using them for PHY stuff. And the implementations still need 
> to call ethnl_req_get_phydev() AFAICT

True, one can even argue that the start() and done() aren't really
useful (allocate/free a ctx, we only really need to know the size of
the context), we'd end-up with just one dedicated helper for PHY dump.

I'll rework this and spin a new version when net-next re-opens, and
I'll clarify the DUMP behaviour for filtering, based on the discussin
with Köry.

Thanks a lot for taking a look then,

Maxime