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

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

[Jiri Pirko]

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(-)

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;
-- 
2.39.2

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

[Jakub Kicinski]

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;
-- 
pw-bot: cr

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

[Jiri Pirko]

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

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

[Jakub Kicinski]

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 :)