diff mbox series

[net-next] devlink: introduce dump selector attr and implement it for port dumps

Message ID 20230713151528.2546909-1-jiri@resnulli.us (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] devlink: introduce dump selector attr and implement it for port dumps | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
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: 1721 this patch: 1721
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1379 this patch: 1379
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: 1894 this patch: 1894
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 87 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 13, 2023, 3:15 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 for
ports and dump only ports 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.

Each object (port in this case), has to pass nla_policy array to expose
what are the supported selection attributes. If user passes attr unknown
to kernel, netlink validation errors out.

Note this infrastructure could be later on easily extended by:
1) Other commands to select dumps by devlink instance.
2) Include other attrs into selection for specific object type. For that
   the dump_one() op would be extended by selector attrs arg.

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>
---
 include/uapi/linux/devlink.h |  2 ++
 net/devlink/devl_internal.h  |  3 +++
 net/devlink/leftover.c       |  5 +++--
 net/devlink/netlink.c        | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski July 14, 2023, 3:51 a.m. UTC | #1 
On Thu, 13 Jul 2023 17:15:28 +0200 Jiri Pirko wrote:
> +	/* If the user provided selector attribute with devlink handle, dump only
> +	 * objects that belong under this instance.
> +	 */
> +	if (cmd->dump_selector_nla_policy &&
> +	    attrs[DEVLINK_ATTR_DUMP_SELECTOR]) {
> +		struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
> +
> +		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX,
> +				       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
> +				       cmd->dump_selector_nla_policy,
> +				       cb->extack);
> +		if (err)
> +			return err;
> +		if (tb[DEVLINK_ATTR_BUS_NAME] && tb[DEVLINK_ATTR_DEV_NAME]) {
> +			devlink = devlink_get_from_attrs_lock(sock_net(msg->sk), tb);
> +			if (IS_ERR(devlink))
> +				return PTR_ERR(devlink);
> +			err = cmd->dump_one(msg, devlink, cb);
> +			devl_unlock(devlink);
> +			devlink_put(devlink);
> +			goto out;
> +		}

This implicitly depends on the fact that cmd->dump_one() will set and
pay attention to state->idx. If it doesn't kernel will infinitely dump
the same instance. I think we should explicitly check state->idx and
set it to 1 after calling ->dump_one.

Could you also move the filtered dump to a separate function which
either does this or calls devlink_nl_instance_iter_dumpit()?
I like the concise beauty that devlink_nl_instance_iter_dumpit()
currently is, it'd be a shame to side load it with other logic :]

> +	}
> +
>  	while ((devlink = devlinks_xa_find_get(sock_net(msg->sk),
>  					       &state->instance))) {
>  		devl_lock(devlink);
> @@ -228,6 +259,7 @@ int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
>  		state->idx = 0;
>  	}
>  
> +out:
>  	if (err != -EMSGSIZE)
>  		return err;
>  	return msg->len;
Jiri Pirko July 14, 2023, 8 a.m. UTC | #2 
Fri, Jul 14, 2023 at 05:51:41AM CEST, kuba@kernel.org wrote:
>On Thu, 13 Jul 2023 17:15:28 +0200 Jiri Pirko wrote:
>> +	/* If the user provided selector attribute with devlink handle, dump only
>> +	 * objects that belong under this instance.
>> +	 */
>> +	if (cmd->dump_selector_nla_policy &&
>> +	    attrs[DEVLINK_ATTR_DUMP_SELECTOR]) {
>> +		struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
>> +
>> +		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX,
>> +				       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
>> +				       cmd->dump_selector_nla_policy,
>> +				       cb->extack);
>> +		if (err)
>> +			return err;
>> +		if (tb[DEVLINK_ATTR_BUS_NAME] && tb[DEVLINK_ATTR_DEV_NAME]) {
>> +			devlink = devlink_get_from_attrs_lock(sock_net(msg->sk), tb);
>> +			if (IS_ERR(devlink))
>> +				return PTR_ERR(devlink);
>> +			err = cmd->dump_one(msg, devlink, cb);
>> +			devl_unlock(devlink);
>> +			devlink_put(devlink);
>> +			goto out;
>> +		}
>
>This implicitly depends on the fact that cmd->dump_one() will set and
>pay attention to state->idx. If it doesn't kernel will infinitely dump
>the same instance. I think we should explicitly check state->idx and
>set it to 1 after calling ->dump_one.

Nothing changes, only instead of iterating over multiple devlinks, we
just work with one.

So, the state->idx is in-devlink-instance index. That means, after
iterating to next devlink instance it is reset to 0 below (state->idx = 0;).
Here however, as we stay only within a single devlink instance,
the reset is not needed.

Am I missing something?


>
>Could you also move the filtered dump to a separate function which
>either does this or calls devlink_nl_instance_iter_dumpit()?
>I like the concise beauty that devlink_nl_instance_iter_dumpit()
>currently is, it'd be a shame to side load it with other logic :]

No problem. I put the code here as if in future the selector attr nest
would be passed down to dump_one(), there is one DEVLINK_ATTR_DUMP_SELECTOR
processing here. But could be resolved later on.

Will do.

>
>> +	}
>> +
>>  	while ((devlink = devlinks_xa_find_get(sock_net(msg->sk),
>>  					       &state->instance))) {
>>  		devl_lock(devlink);
>> @@ -228,6 +259,7 @@ int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
>>  		state->idx = 0;
>>  	}
>>  
>> +out:
>>  	if (err != -EMSGSIZE)
>>  		return err;
>>  	return msg->len;
>-- 
>pw-bot: cr
Jakub Kicinski July 14, 2023, 3:40 p.m. UTC | #3 
On Fri, 14 Jul 2023 10:00:49 +0200 Jiri Pirko wrote:
> Fri, Jul 14, 2023 at 05:51:41AM CEST, kuba@kernel.org wrote:
> >On Thu, 13 Jul 2023 17:15:28 +0200 Jiri Pirko wrote:  
> >> +	/* If the user provided selector attribute with devlink handle, dump only
> >> +	 * objects that belong under this instance.
> >> +	 */
> >> +	if (cmd->dump_selector_nla_policy &&
> >> +	    attrs[DEVLINK_ATTR_DUMP_SELECTOR]) {
> >> +		struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
> >> +
> >> +		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX,
> >> +				       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
> >> +				       cmd->dump_selector_nla_policy,
> >> +				       cb->extack);
> >> +		if (err)
> >> +			return err;
> >> +		if (tb[DEVLINK_ATTR_BUS_NAME] && tb[DEVLINK_ATTR_DEV_NAME]) {
> >> +			devlink = devlink_get_from_attrs_lock(sock_net(msg->sk), tb);
> >> +			if (IS_ERR(devlink))
> >> +				return PTR_ERR(devlink);
> >> +			err = cmd->dump_one(msg, devlink, cb);
> >> +			devl_unlock(devlink);
> >> +			devlink_put(devlink);
> >> +			goto out;
> >> +		}  
> >
> >This implicitly depends on the fact that cmd->dump_one() will set and
> >pay attention to state->idx. If it doesn't kernel will infinitely dump
> >the same instance. I think we should explicitly check state->idx and
> >set it to 1 after calling ->dump_one.  
> 
> Nothing changes, only instead of iterating over multiple devlinks, we
> just work with one.
> 
> So, the state->idx is in-devlink-instance index. That means, after
> iterating to next devlink instance it is reset to 0 below (state->idx = 0;).
> Here however, as we stay only within a single devlink instance,
> the reset is not needed.
> 
> Am I missing something?

The case I was thinking of is if we support filtering of dumps which
do not have sub-objects. In that case state->idx does not get touched
by the dump_one.

Looking closer, tho, there's no case of this sort today, so my concern
is premature. Also what I suggested won't really work. So ignore this
comment, sorry :)
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 62921b2eb0d3..4f5cf18af6a1 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -117,6 +117,7 @@  struct devlink_nl_dump_state {
 struct devlink_cmd {
 	int (*dump_one)(struct sk_buff *msg, struct devlink *devlink,
 			struct netlink_callback *cb);
+	const struct nla_policy *dump_selector_nla_policy;
 };
 
 extern const struct genl_small_ops devlink_nl_ops[56];
@@ -127,6 +128,8 @@  devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs);
 void devlink_notify_unregister(struct devlink *devlink);
 void devlink_notify_register(struct devlink *devlink);
 
+extern const struct nla_policy devlink_nl_handle_policy[DEVLINK_ATTR_MAX + 1];
+
 int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 				    struct netlink_callback *cb);
 
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 5128b9c7eea8..aeb61b8e9e62 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -1119,7 +1119,8 @@  devlink_nl_cmd_port_get_dump_one(struct sk_buff *msg, struct devlink *devlink,
 }
 
 const struct devlink_cmd devl_cmd_port_get = {
-	.dump_one		= devlink_nl_cmd_port_get_dump_one,
+	.dump_one			= devlink_nl_cmd_port_get_dump_one,
+	.dump_selector_nla_policy	= devlink_nl_handle_policy,
 };
 
 static int devlink_port_type_set(struct devlink_port *devlink_port,
@@ -6288,7 +6289,7 @@  const struct genl_small_ops devlink_nl_ops[56] = {
 	},
 	{
 		.cmd = DEVLINK_CMD_PORT_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP_STRICT,
 		.doit = devlink_nl_cmd_port_get_doit,
 		.dumpit = devlink_nl_instance_iter_dumpit,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 7a332eb70f70..f6cd06bd1f09 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 *
@@ -196,17 +197,47 @@  static const struct devlink_cmd *devl_cmds[] = {
 	[DEVLINK_CMD_SELFTESTS_GET]	= &devl_cmd_selftests_get,
 };
 
+const struct nla_policy devlink_nl_handle_policy[DEVLINK_ATTR_MAX + 1] = {
+	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING },
+};
+
 int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 				    struct netlink_callback *cb)
 {
 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
 	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+	struct nlattr **attrs = info->attrs;
 	const struct devlink_cmd *cmd;
 	struct devlink *devlink;
 	int err = 0;
 
 	cmd = devl_cmds[info->op.cmd];
 
+	/* If the user provided selector attribute with devlink handle, dump only
+	 * objects that belong under this instance.
+	 */
+	if (cmd->dump_selector_nla_policy &&
+	    attrs[DEVLINK_ATTR_DUMP_SELECTOR]) {
+		struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
+
+		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX,
+				       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
+				       cmd->dump_selector_nla_policy,
+				       cb->extack);
+		if (err)
+			return err;
+		if (tb[DEVLINK_ATTR_BUS_NAME] && tb[DEVLINK_ATTR_DEV_NAME]) {
+			devlink = devlink_get_from_attrs_lock(sock_net(msg->sk), tb);
+			if (IS_ERR(devlink))
+				return PTR_ERR(devlink);
+			err = cmd->dump_one(msg, devlink, cb);
+			devl_unlock(devlink);
+			devlink_put(devlink);
+			goto out;
+		}
+	}
+
 	while ((devlink = devlinks_xa_find_get(sock_net(msg->sk),
 					       &state->instance))) {
 		devl_lock(devlink);
@@ -228,6 +259,7 @@  int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 		state->idx = 0;
 	}
 
+out:
 	if (err != -EMSGSIZE)
 		return err;
 	return msg->len;