| Message ID | 20250324104012.367366-3-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethtool: Introduce ethnl dump helpers | expand |
On Mon, 24 Mar 2025 11:40:04 +0100 Maxime Chevallier <maxime.chevallier@bootlin.com> wrote: > We have a number of netlink commands in the ethnl family that may have > multiple objects to dump even for a single net_device, including : > > - PLCA, PSE-PD, phy: one message per PHY device > - tsinfo: one message per timestamp source (netdev + phys) > - rss: One per RSS context > > To get this behaviour, these netlink commands need to roll a custom > ->dumpit(). > > To prepare making per-netdev DUMP more generic in ethnl, introduce a > member in the ethnl ops to indicate if a given command may allow > pernetdev DUMPs (also referred to as filtered DUMPs). ... > + > /* generic ->start() handler for GET requests */ > static int ethnl_default_start(struct netlink_callback *cb) > { > @@ -636,10 +659,10 @@ static int ethnl_default_start(struct netlink_callback > *cb) } > > ret = ethnl_default_parse(req_info, &info->info, ops, false); > - if (req_info->dev) { > - /* We ignore device specification in dump requests but as the > - * same parser as for non-dump (doit) requests is used, it > - * would take reference to the device if it finds one > + if (req_info->dev && !ops->allow_pernetdev_dump) { > + /* We ignore device specification in unfiltered dump requests > + * but as the same parser as for non-dump (doit) requests is > + * used, it would take reference to the device if it finds This means the dump will have a different behavior in case of filtered dump (allow_pernetdev_dump) or standard dump. The standard dump will drop the interface device so it will dump all interfaces even if one is specified. The filtered dump will dump only the specified interface. Maybe it would be nice to have the same behavior for the dump for all the ethtool command. Even if this change modify the behavior of the dump for all the ethtool commands it won't be an issue as the filtered dump did not exist before, so I suppose it won't break anything. IMHO it is safer to do it now than later, if existing ethtool command adds support for filtered dump. We should find another way to know the parser is called from dump or doit. Regards,
On Tue, 25 Mar 2025 12:27:06 +0100 Kory Maincent wrote: > > @@ -636,10 +659,10 @@ static int ethnl_default_start(struct netlink_callback > > *cb) } > > > > ret = ethnl_default_parse(req_info, &info->info, ops, false); > > - if (req_info->dev) { > > - /* We ignore device specification in dump requests but as the > > - * same parser as for non-dump (doit) requests is used, it > > - * would take reference to the device if it finds one > > + if (req_info->dev && !ops->allow_pernetdev_dump) { > > + /* We ignore device specification in unfiltered dump requests > > + * but as the same parser as for non-dump (doit) requests is > > + * used, it would take reference to the device if it finds > > This means the dump will have a different behavior in case of filtered dump > (allow_pernetdev_dump) or standard dump. > The standard dump will drop the interface device so it will dump all interfaces > even if one is specified. > The filtered dump will dump only the specified interface. > Maybe it would be nice to have the same behavior for the dump for all the > ethtool command. > Even if this change modify the behavior of the dump for all the ethtool commands > it won't be an issue as the filtered dump did not exist before, so I suppose it > won't break anything. IMHO it is safer to do it now than later, if existing > ethtool command adds support for filtered dump. > We should find another way to know the parser is called from dump or doit. Let's try. We can probably make required_dev attr of ethnl_parse_header_dev_get() a three state one: require, allow, reject? Part of the problem is that ethtool is not converted to split ops, so do and dump share the same parsing policy. But that's too painful to fix now, I think.
On Tue, 25 Mar 2025 14:15:07 -0700 Jakub Kicinski wrote: > > This means the dump will have a different behavior in case of filtered dump > > (allow_pernetdev_dump) or standard dump. > > The standard dump will drop the interface device so it will dump all interfaces > > even if one is specified. > > The filtered dump will dump only the specified interface. > > Maybe it would be nice to have the same behavior for the dump for all the > > ethtool command. > > Even if this change modify the behavior of the dump for all the ethtool commands > > it won't be an issue as the filtered dump did not exist before, so I suppose it > > won't break anything. IMHO it is safer to do it now than later, if existing > > ethtool command adds support for filtered dump. > > We should find another way to know the parser is called from dump or doit. > > Let's try. We can probably make required_dev attr of > ethnl_parse_header_dev_get() a three state one: require, allow, reject? Ah, don't think this is going to work. You're not converting all the dumps, just the PHY ones. It's fine either way, then.
On Tue, 25 Mar 2025 14:22:02 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 25 Mar 2025 14:15:07 -0700 Jakub Kicinski wrote: > > > This means the dump will have a different behavior in case of filtered dump > > > (allow_pernetdev_dump) or standard dump. > > > The standard dump will drop the interface device so it will dump all interfaces > > > even if one is specified. > > > The filtered dump will dump only the specified interface. > > > Maybe it would be nice to have the same behavior for the dump for all the > > > ethtool command. > > > Even if this change modify the behavior of the dump for all the ethtool commands > > > it won't be an issue as the filtered dump did not exist before, so I suppose it > > > won't break anything. IMHO it is safer to do it now than later, if existing > > > ethtool command adds support for filtered dump. > > > We should find another way to know the parser is called from dump or doit. > > > > Let's try. We can probably make required_dev attr of > > ethnl_parse_header_dev_get() a three state one: require, allow, reject? > > Ah, don't think this is going to work. You're not converting all > the dumps, just the PHY ones. It's fine either way, then. Yeah I noticed that when implementing, but I actually forgot to mention in in my cover, which I definitely should have :( What we can also do is properly support multi-phy dump but not filtered dump on all the existing phy commands (plca, pse-pd, etc.) so that be behaviour is unchanged for these. Only PHY_GET and any future per-phy commands would support it. Maxime
On Wed, 26 Mar 2025 08:59:06 +0100 Maxime Chevallier <maxime.chevallier@bootlin.com> wrote: > On Tue, 25 Mar 2025 14:22:02 -0700 > Jakub Kicinski <kuba@kernel.org> wrote: > > > On Tue, 25 Mar 2025 14:15:07 -0700 Jakub Kicinski wrote: > [...] > > > > > > Let's try. We can probably make required_dev attr of > > > ethnl_parse_header_dev_get() a three state one: require, allow, reject? > > > > > > > Ah, don't think this is going to work. You're not converting all > > the dumps, just the PHY ones. It's fine either way, then. > > Yeah I noticed that when implementing, but I actually forgot to mention > in in my cover, which I definitely should have :( > > What we can also do is properly support multi-phy dump but not filtered > dump on all the existing phy commands (plca, pse-pd, etc.) so that be > behaviour is unchanged for these. Only PHY_GET and any future per-phy > commands would support it. Couldn't we remove the existence check of ctx->req_info->dev in ethnl_default_start and add the netdev_put() in the ethnl_default_dumpit()? Would this work? Or we could keep your change and let the userspace adapt to the new support of filtered dump. In fact you are modifying all the ethtool commands that are already related to PHY, if you don't they surely will one day or another so it is good to keep it. Regards,
On Wed, 26 Mar 2025 11:29:36 +0100 Kory Maincent <kory.maincent@bootlin.com> wrote: > On Wed, 26 Mar 2025 08:59:06 +0100 > Maxime Chevallier <maxime.chevallier@bootlin.com> wrote: > > > On Tue, 25 Mar 2025 14:22:02 -0700 > > Jakub Kicinski <kuba@kernel.org> wrote: > > > > > On Tue, 25 Mar 2025 14:15:07 -0700 Jakub Kicinski wrote: > > [...] > > > > > > > > Let's try. We can probably make required_dev attr of > > > > ethnl_parse_header_dev_get() a three state one: require, allow, reject? > > > > > > > > > > Ah, don't think this is going to work. You're not converting all > > > the dumps, just the PHY ones. It's fine either way, then. > > > > Yeah I noticed that when implementing, but I actually forgot to mention > > in in my cover, which I definitely should have :( > > > > What we can also do is properly support multi-phy dump but not filtered > > dump on all the existing phy commands (plca, pse-pd, etc.) so that be > > behaviour is unchanged for these. Only PHY_GET and any future per-phy > > commands would support it. > > Couldn't we remove the existence check of ctx->req_info->dev in > ethnl_default_start and add the netdev_put() in the ethnl_default_dumpit()? > Would this work? It would work, but it seems unnecessary to hold a refcount on a specific netdev while we iterate on all netdevs in the namespace afterwards. But I'd say that's an implementation detail :) > Or we could keep your change and let the userspace adapt to the new support of > filtered dump. In fact you are modifying all the ethtool commands that are > already related to PHY, if you don't they surely will one day or another so it > is good to keep it. So the question is, is it OK to stop ignoring the ifindex/ifname header attribute for ethnl dump requests, and if so, should we do that for all dump commands or only the ones for which is makes sense to do so. Jakub says "t's fine either way, then.", but don't fully get if this is an answer to the above question :) This will only change the behaviour of filtered dumps, that aren't really used with ethnl (except for PHY_GET but we won't change its behaviour either way) Maxime
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 6b1725795435..31ab89ca0bcc 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -577,9 +577,7 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev, return ret; } -/* Default ->dumpit() handler for GET requests. */ -static int ethnl_default_dumpit(struct sk_buff *skb, - struct netlink_callback *cb) +static int ethnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb) { struct ethnl_dump_ctx *ctx = ethnl_dump_context(cb); struct net *net = sock_net(skb->sk); @@ -609,6 +607,31 @@ static int ethnl_default_dumpit(struct sk_buff *skb, return ret; } +/* Default ->dumpit() handler for GET requests. */ +static int ethnl_default_dumpit(struct sk_buff *skb, + struct netlink_callback *cb) +{ + struct ethnl_dump_ctx *ctx = ethnl_dump_context(cb); + int ret; + + if (ctx->req_info->dev) { + /* Filtered DUMP request targeted to a single netdev. We already + * hold a ref to the netdev from ->start() + */ + ret = ethnl_default_dump_one(skb, ctx->req_info->dev, ctx, + genl_info_dump(cb)); + netdev_put(ctx->req_info->dev, &ctx->req_info->dev_tracker); + + if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len)) + ret = skb->len; + + } else { + ret = ethnl_dump_all(skb, cb); + } + + return ret; +} + /* generic ->start() handler for GET requests */ static int ethnl_default_start(struct netlink_callback *cb) { @@ -636,10 +659,10 @@ static int ethnl_default_start(struct netlink_callback *cb) } ret = ethnl_default_parse(req_info, &info->info, ops, false); - if (req_info->dev) { - /* We ignore device specification in dump requests but as the - * same parser as for non-dump (doit) requests is used, it - * would take reference to the device if it finds one + if (req_info->dev && !ops->allow_pernetdev_dump) { + /* We ignore device specification in unfiltered dump requests + * but as the same parser as for non-dump (doit) requests is + * used, it would take reference to the device if it finds one */ netdev_put(req_info->dev, &req_info->dev_tracker); req_info->dev = NULL; diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index ec6ab5443a6f..4aaa73282d6a 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -331,6 +331,7 @@ int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid, * @req_info_size: size of request info * @reply_data_size: size of reply data * @allow_nodev_do: allow non-dump request with no device identification + * @allow_pernetdev_dump: allow filtering dump requests with ifname/ifindex * @set_ntf_cmd: notification to generate on changes (SET) * @parse_request: * Parse request except common header (struct ethnl_req_info). Common @@ -388,6 +389,7 @@ struct ethnl_request_ops { unsigned int req_info_size; unsigned int reply_data_size; bool allow_nodev_do; + bool allow_pernetdev_dump; u8 set_ntf_cmd; int (*parse_request)(struct ethnl_req_info *req_info,
We have a number of netlink commands in the ethnl family that may have multiple objects to dump even for a single net_device, including : - PLCA, PSE-PD, phy: one message per PHY device - tsinfo: one message per timestamp source (netdev + phys) - rss: One per RSS context To get this behaviour, these netlink commands need to roll a custom ->dumpit(). To prepare making per-netdev DUMP more generic in ethnl, introduce a member in the ethnl ops to indicate if a given command may allow pernetdev DUMPs (also referred to as filtered DUMPs). Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- V4: - Don't hold RCU in the filtered path - Move dump_all into a new separate helper net/ethtool/netlink.c | 37 ++++++++++++++++++++++++++++++------- net/ethtool/netlink.h | 2 ++ 2 files changed, 32 insertions(+), 7 deletions(-)