diff mbox series

[net-next,v2,10/11] devlink: introduce dump selector attr and use it for per-instance dumps

Message ID 20230720121829.566974-11-jiri@resnulli.us (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series devlink: introduce dump selector attr and use it for per-instance dumps | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1722 this patch: 1722
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1380 this patch: 1380
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1895 this patch: 1895
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko July 20, 2023, 12:18 p.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

For SFs, one devlink instance per SF is created. There might be
thousands of these on a single host. When a user needs to know port
handle for specific SF, he needs to dump all devlink ports on the host
which does not scale good.

Allow user to pass devlink handle alongside the dump command
and dump only objects which are under selected devlink instance.

Introduce new attr DEVLINK_ATTR_DUMP_SELECTOR to nest the selection
attributes. This way the userspace can use maxattr to tell if dump
selector is supported by kernel or not.

Assemble netlink policy for selector attribute. If user passes attr
unknown to kernel, netlink validation errors out.

Example:
$ devlink port show
auxiliary/mlx5_core.eth.0/65535: type eth netdev eth2 flavour physical port 0 splittable false
auxiliary/mlx5_core.eth.1/131071: type eth netdev eth3 flavour physical port 1 splittable false

$ devlink port show auxiliary/mlx5_core.eth.0
auxiliary/mlx5_core.eth.0/65535: type eth netdev eth2 flavour physical port 0 splittable false

$ devlink port show auxiliary/mlx5_core.eth.1
auxiliary/mlx5_core.eth.1/131071: type eth netdev eth3 flavour physical port 1 splittable false

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- extended to patch that covers all dumpit commands
- used start() and done() callback to parse the selector attr
- changed the selector attr netlink policy to be created on fly
- changed patch description accordingly
---
 include/uapi/linux/devlink.h |  2 +
 net/devlink/devl_internal.h  |  1 +
 net/devlink/netlink.c        | 99 +++++++++++++++++++++++++++++++++++-
 3 files changed, 101 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski July 25, 2023, 6:40 p.m. UTC | #1 
On Thu, 20 Jul 2023 14:18:28 +0200 Jiri Pirko wrote:
> +static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr)
> +{
> +	memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy));
> +}
> +
> +static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd,
> +						 struct nla_policy *policy)
> +{
> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME);
> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME);
> +}
> +
> +static int devlink_nl_start(struct netlink_callback *cb)
> +{
> +	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> +	struct nlattr **attrs = info->attrs;
> +	const struct devlink_cmd *cmd;
> +	struct nla_policy *policy;
> +	struct nlattr **selector;
> +	int err;
> +
> +	if (!attrs[DEVLINK_ATTR_DUMP_SELECTOR])
> +		return 0;
> +
> +	selector = kzalloc(sizeof(*selector) * (DEVLINK_ATTR_MAX + 1),
> +			   GFP_KERNEL);
> +	if (!selector)
> +		return -ENOMEM;
> +	policy = kzalloc(sizeof(*policy) * (DEVLINK_ATTR_MAX + 1), GFP_KERNEL);
> +	if (!policy) {
> +		kfree(selector);
> +		return -ENOMEM;
> +	}
> +
> +	cmd = devl_cmds[info->op.cmd];
> +	devlink_nl_dump_selector_policy_init(cmd, policy);
> +	err = nla_parse_nested(selector, DEVLINK_ATTR_MAX,
> +			       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
> +			       policy, cb->extack);
> +	kfree(policy);
> +	if (err) {
> +		kfree(selector);
> +		return err;
> +	}
> +
> +	state->selector = selector;
> +	return 0;
> +}

Why not declare a fully nested policy with just the two attrs?

Also - do you know of any userspace which would pass garbage attrs 
to the dumps? Do we really need to accept all attributes, or can
we trim the dump policies to what's actually supported?
Jiri Pirko July 31, 2023, 12:47 p.m. UTC | #2 
Tue, Jul 25, 2023 at 08:40:44PM CEST, kuba@kernel.org wrote:
>On Thu, 20 Jul 2023 14:18:28 +0200 Jiri Pirko wrote:
>> +static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr)
>> +{
>> +	memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy));
>> +}
>> +
>> +static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd,
>> +						 struct nla_policy *policy)
>> +{
>> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME);
>> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME);
>> +}
>> +
>> +static int devlink_nl_start(struct netlink_callback *cb)
>> +{
>> +	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>> +	struct nlattr **attrs = info->attrs;
>> +	const struct devlink_cmd *cmd;
>> +	struct nla_policy *policy;
>> +	struct nlattr **selector;
>> +	int err;
>> +
>> +	if (!attrs[DEVLINK_ATTR_DUMP_SELECTOR])
>> +		return 0;
>> +
>> +	selector = kzalloc(sizeof(*selector) * (DEVLINK_ATTR_MAX + 1),
>> +			   GFP_KERNEL);
>> +	if (!selector)
>> +		return -ENOMEM;
>> +	policy = kzalloc(sizeof(*policy) * (DEVLINK_ATTR_MAX + 1), GFP_KERNEL);
>> +	if (!policy) {
>> +		kfree(selector);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	cmd = devl_cmds[info->op.cmd];
>> +	devlink_nl_dump_selector_policy_init(cmd, policy);
>> +	err = nla_parse_nested(selector, DEVLINK_ATTR_MAX,
>> +			       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
>> +			       policy, cb->extack);
>> +	kfree(policy);
>> +	if (err) {
>> +		kfree(selector);
>> +		return err;
>> +	}
>> +
>> +	state->selector = selector;
>> +	return 0;
>> +}
>
>Why not declare a fully nested policy with just the two attrs?

Not sure I follow. But the nest under DEVLINK_ATTR_DUMP_SELECTOR has
its own policy, generated by devlink_nl_dump_selector_policy_init(). I
did it this way instead of separate policy array for 2 reasons:
1) We don't have duplicate and possibly conflicting policies for devlink
   root and selector
2) It is easy for specific object type to pass attrs that are included
   in the policy initialization (see the health reporter extension later
   in this patchset). There are couple of object to benefit from this,
   for example "sb".
3) It is I think a bit nicer for specific object type to pass array of
   attrs, instead of a policy array that would be exported from netlink.c

If you insist on separate policy arrays, I can do it though. I had it
like that initially, I just decided to go this way for the 3 reasons
listed above.


>
>Also - do you know of any userspace which would pass garbage attrs 
>to the dumps? Do we really need to accept all attributes, or can
>we trim the dump policies to what's actually supported?

That's what this patch is doing. It only accepts what the kernel
understands. It gives the object types (as for example health reporter)
option to extend the attr set to accept them into selectors as well, if
they know how to handle them.


>-- 
>pw-bot: cr
Jakub Kicinski July 31, 2023, 5:03 p.m. UTC | #3 
On Mon, 31 Jul 2023 14:47:08 +0200 Jiri Pirko wrote:
> >Why not declare a fully nested policy with just the two attrs?  
> 
> Not sure I follow. But the nest under DEVLINK_ATTR_DUMP_SELECTOR has
> its own policy, generated by devlink_nl_dump_selector_policy_init(). I
> did it this way instead of separate policy array for 2 reasons:
> 1) We don't have duplicate and possibly conflicting policies for devlink
>    root and selector
> 2) It is easy for specific object type to pass attrs that are included
>    in the policy initialization (see the health reporter extension later
>    in this patchset). There are couple of object to benefit from this,
>    for example "sb".
> 3) It is I think a bit nicer for specific object type to pass array of
>    attrs, instead of a policy array that would be exported from netlink.c
> 
> If you insist on separate policy arrays, I can do it though.

IMO the contents of the series do not justify the complexity.

> I had it like that initially, I just decided to go this way for the 3 reasons
> listed above.
> 
> >Also - do you know of any userspace which would pass garbage attrs 
> >to the dumps? Do we really need to accept all attributes, or can
> >we trim the dump policies to what's actually supported?  
> 
> That's what this patch is doing. It only accepts what the kernel
> understands. It gives the object types (as for example health reporter)
> option to extend the attr set to accept them into selectors as well, if
> they know how to handle them.

I'm talking about the "outer" policy, the level at which
DEVLINK_ATTR_DUMP_SELECTOR is defined.
Jiri Pirko Aug. 1, 2023, 6:42 a.m. UTC | #4 
Mon, Jul 31, 2023 at 07:03:41PM CEST, kuba@kernel.org wrote:
>On Mon, 31 Jul 2023 14:47:08 +0200 Jiri Pirko wrote:
>> >Why not declare a fully nested policy with just the two attrs?  
>> 
>> Not sure I follow. But the nest under DEVLINK_ATTR_DUMP_SELECTOR has
>> its own policy, generated by devlink_nl_dump_selector_policy_init(). I
>> did it this way instead of separate policy array for 2 reasons:
>> 1) We don't have duplicate and possibly conflicting policies for devlink
>>    root and selector
>> 2) It is easy for specific object type to pass attrs that are included
>>    in the policy initialization (see the health reporter extension later
>>    in this patchset). There are couple of object to benefit from this,
>>    for example "sb".
>> 3) It is I think a bit nicer for specific object type to pass array of
>>    attrs, instead of a policy array that would be exported from netlink.c
>> 
>> If you insist on separate policy arrays, I can do it though.
>
>IMO the contents of the series do not justify the complexity.
>
>> I had it like that initially, I just decided to go this way for the 3 reasons
>> listed above.
>> 
>> >Also - do you know of any userspace which would pass garbage attrs 
>> >to the dumps? Do we really need to accept all attributes, or can
>> >we trim the dump policies to what's actually supported?  
>> 
>> That's what this patch is doing. It only accepts what the kernel
>> understands. It gives the object types (as for example health reporter)
>> option to extend the attr set to accept them into selectors as well, if
>> they know how to handle them.
>
>I'm talking about the "outer" policy, the level at which
>DEVLINK_ATTR_DUMP_SELECTOR is defined.

I don't follow :/ Could you please describe what exactly do you mean and
want to see? Thanks!
Jakub Kicinski Aug. 1, 2023, 3:53 p.m. UTC | #5 
On Tue, 1 Aug 2023 08:42:09 +0200 Jiri Pirko wrote:
> >> >Also - do you know of any userspace which would pass garbage attrs 
> >> >to the dumps? Do we really need to accept all attributes, or can
> >> >we trim the dump policies to what's actually supported?    
> >> 
> >> That's what this patch is doing. It only accepts what the kernel
> >> understands. It gives the object types (as for example health reporter)
> >> option to extend the attr set to accept them into selectors as well, if
> >> they know how to handle them.  
> >
> >I'm talking about the "outer" policy, the level at which
> >DEVLINK_ATTR_DUMP_SELECTOR is defined.  
> 
> I don't follow :/ Could you please describe what exactly do you mean and
> want to see? Thanks!

It's a bit obscured by the macros, but AFAICT you pass
devlink_nl_policy for the dumps, while the _only_ attribute
the kernel will interpret is DEVLINK_ATTR_DUMP_SELECTOR
and its insides.
Jiri Pirko Aug. 2, 2023, 7:02 a.m. UTC | #6 
Tue, Aug 01, 2023 at 05:53:01PM CEST, kuba@kernel.org wrote:
>On Tue, 1 Aug 2023 08:42:09 +0200 Jiri Pirko wrote:
>> >> >Also - do you know of any userspace which would pass garbage attrs 
>> >> >to the dumps? Do we really need to accept all attributes, or can
>> >> >we trim the dump policies to what's actually supported?    
>> >> 
>> >> That's what this patch is doing. It only accepts what the kernel
>> >> understands. It gives the object types (as for example health reporter)
>> >> option to extend the attr set to accept them into selectors as well, if
>> >> they know how to handle them.  
>> >
>> >I'm talking about the "outer" policy, the level at which
>> >DEVLINK_ATTR_DUMP_SELECTOR is defined.  
>> 
>> I don't follow :/ Could you please describe what exactly do you mean and
>> want to see? Thanks!
>
>It's a bit obscured by the macros, but AFAICT you pass
>devlink_nl_policy for the dumps, while the _only_ attribute
>the kernel will interpret is DEVLINK_ATTR_DUMP_SELECTOR
>and its insides.

True, you are correct.
Anyway with the split ops generation, this is going to be narrowed down,
so possiblem garbage is ignored.
Thanks!
diff mbox series

Patch

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 3782d4219ac9..8b74686512ae 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -612,6 +612,8 @@  enum devlink_attr {
 
 	DEVLINK_ATTR_REGION_DIRECT,		/* flag */
 
+	DEVLINK_ATTR_DUMP_SELECTOR,		/* nested */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 79614b45e8ac..168d36dbc6f7 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -109,6 +109,7 @@  struct devlink_nl_dump_state {
 			u64 dump_ts;
 		};
 	};
+	struct nlattr **selector;
 };
 
 struct devlink_cmd {
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 90497d0e1a7b..c2083398bd73 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -80,6 +80,7 @@  static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_RATE_TX_PRIORITY] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_RATE_TX_WEIGHT] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_REGION_DIRECT] = { .type = NLA_FLAG },
+	[DEVLINK_ATTR_DUMP_SELECTOR] = { .type = NLA_NESTED },
 };
 
 struct devlink *
@@ -195,6 +196,30 @@  static const struct devlink_cmd *devl_cmds[] = {
 	[DEVLINK_CMD_SELFTESTS_GET]	= &devl_cmd_selftests_get,
 };
 
+static int devlink_nl_instance_single_dumpit(struct sk_buff *msg,
+					     struct netlink_callback *cb)
+{
+	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct nlattr **selector = state->selector;
+	const struct devlink_cmd *cmd;
+	struct devlink *devlink;
+	int err;
+
+	cmd = devl_cmds[info->op.cmd];
+
+	devlink = devlink_get_from_attrs_lock(sock_net(msg->sk), selector);
+	if (IS_ERR(devlink))
+		return PTR_ERR(devlink);
+	err = cmd->dump_one(msg, devlink, cb);
+	devl_unlock(devlink);
+	devlink_put(devlink);
+
+	if (err != -EMSGSIZE)
+		return err;
+	return msg->len;
+}
+
 static int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 					   struct netlink_callback *cb)
 {
@@ -232,6 +257,76 @@  static int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 	return msg->len;
 }
 
+static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr)
+{
+	memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy));
+}
+
+static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd,
+						 struct nla_policy *policy)
+{
+	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME);
+	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME);
+}
+
+static int devlink_nl_start(struct netlink_callback *cb)
+{
+	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct nlattr **attrs = info->attrs;
+	const struct devlink_cmd *cmd;
+	struct nla_policy *policy;
+	struct nlattr **selector;
+	int err;
+
+	if (!attrs[DEVLINK_ATTR_DUMP_SELECTOR])
+		return 0;
+
+	selector = kzalloc(sizeof(*selector) * (DEVLINK_ATTR_MAX + 1),
+			   GFP_KERNEL);
+	if (!selector)
+		return -ENOMEM;
+	policy = kzalloc(sizeof(*policy) * (DEVLINK_ATTR_MAX + 1), GFP_KERNEL);
+	if (!policy) {
+		kfree(selector);
+		return -ENOMEM;
+	}
+
+	cmd = devl_cmds[info->op.cmd];
+	devlink_nl_dump_selector_policy_init(cmd, policy);
+	err = nla_parse_nested(selector, DEVLINK_ATTR_MAX,
+			       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
+			       policy, cb->extack);
+	kfree(policy);
+	if (err) {
+		kfree(selector);
+		return err;
+	}
+
+	state->selector = selector;
+	return 0;
+}
+
+static int devlink_nl_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
+{
+	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+	struct nlattr **selector = state->selector;
+
+	if (selector && selector[DEVLINK_ATTR_BUS_NAME] &&
+	    selector[DEVLINK_ATTR_DEV_NAME])
+		return devlink_nl_instance_single_dumpit(msg, cb);
+	else
+		return devlink_nl_instance_iter_dumpit(msg, cb);
+}
+
+static int devlink_nl_done(struct netlink_callback *cb)
+{
+	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+
+	kfree(state->selector);
+	return 0;
+}
+
 #define __DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix, _validate,	\
 			_maxattr, _policy)					\
 	{									\
@@ -248,7 +343,9 @@  static int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 #define __DEVL_NL_OP_DUMP(cmd_subname, _validate, _maxattr, _policy)		\
 	{									\
 		.cmd = DEVLINK_CMD_##cmd_subname,				\
-		.dumpit = devlink_nl_instance_iter_dumpit,			\
+		.start = devlink_nl_start,					\
+		.dumpit = devlink_nl_dumpit,					\
+		.done = devlink_nl_done,					\
 		.flags = GENL_CMD_CAP_DUMP,					\
 		.validate = _validate,						\
 		.maxattr = _maxattr,						\