Date:   Thu, 15 Dec 2022 10:11:03 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
        pabeni@...hat.com, jacob.e.keller@...el.com, leon@...nel.org
Subject: Re: [RFC net-next 14/15] devlink: add by-instance dump infra

Thu, Dec 15, 2022 at 03:01:54AM CET, kuba@...nel.org wrote:
>Most dumpit implementations walk the devlink instances.
>This requires careful lock taking and reference dropping.
>Factor the loop out and provide just a callback to handle
>a single instance dump.
>
>Convert one user as an example, other users converted
>in the next change.
>
>Slightly inspired by ethtool netlink code.
>
>Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>---
> net/devlink/basic.c         | 55 ++++++++++++++++---------------------
> net/devlink/devl_internal.h | 10 +++++++
> net/devlink/netlink.c       | 33 ++++++++++++++++++++++
> 3 files changed, 67 insertions(+), 31 deletions(-)
>
>diff --git a/net/devlink/basic.c b/net/devlink/basic.c
>index c6ad8133fc23..f18d8dcf9751 100644
>--- a/net/devlink/basic.c
>+++ b/net/devlink/basic.c
>@@ -1219,47 +1219,40 @@ static void devlink_rate_notify(struct devlink_rate *devlink_rate,
> 				0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
> }
> 
>-static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
>-					  struct netlink_callback *cb)
>+static int
>+devlink_nl_cmd_rate_get_dumpinst(struct sk_buff *msg, struct devlink *devlink,
>+				 struct netlink_callback *cb)
> {
> 	struct devlink_nl_dump_state *dump = devl_dump_state(cb);
>-	struct devlink *devlink;
>+	struct devlink_rate *devlink_rate;
>+	int idx = 0;
> 	int err = 0;
> 
>-	devlink_dump_for_each_instance_get(msg, dump, devlink) {
>-		struct devlink_rate *devlink_rate;
>-		int idx = 0;
>-
>-		devl_lock(devlink);
>-		list_for_each_entry(devlink_rate, &devlink->rate_list, list) {
>-			enum devlink_command cmd = DEVLINK_CMD_RATE_NEW;
>-			u32 id = NETLINK_CB(cb->skb).portid;
>+	list_for_each_entry(devlink_rate, &devlink->rate_list, list) {
>+		enum devlink_command cmd = DEVLINK_CMD_RATE_NEW;
>+		u32 id = NETLINK_CB(cb->skb).portid;
> 
>-			if (idx < dump->idx) {
>-				idx++;
>-				continue;
>-			}
>-			err = devlink_nl_rate_fill(msg, devlink_rate, cmd, id,
>-						   cb->nlh->nlmsg_seq,
>-						   NLM_F_MULTI, NULL);
>-			if (err) {
>-				devl_unlock(devlink);
>-				devlink_put(devlink);
>-				dump->idx = idx;
>-				goto out;
>-			}
>+		if (idx < dump->idx) {
> 			idx++;
>+			continue;
> 		}
>-		devl_unlock(devlink);
>-		devlink_put(devlink);
>+		err = devlink_nl_rate_fill(msg, devlink_rate, cmd, id,
>+					   cb->nlh->nlmsg_seq,
>+					   NLM_F_MULTI, NULL);
>+		if (err) {
>+			dump->idx = idx;
>+			break;
>+		}
>+		idx++;
> 	}
>-out:
>-	if (err != -EMSGSIZE)
>-		return err;
> 
>-	return msg->len;
>+	return err;
> }
> 
>+const struct devlink_gen_cmd devl_gen_rate_get = {
>+	.dump_one		= devlink_nl_cmd_rate_get_dumpinst,
>+};
>+
> static int devlink_nl_cmd_rate_get_doit(struct sk_buff *skb,
> 					struct genl_info *info)
> {
>@@ -9130,7 +9123,7 @@ const struct genl_small_ops devlink_nl_ops[56] = {
> 	{
> 		.cmd = DEVLINK_CMD_RATE_GET,
> 		.doit = devlink_nl_cmd_rate_get_doit,
>-		.dumpit = devlink_nl_cmd_rate_get_dumpit,
>+		.dumpit = devlink_instance_iter_dump,
> 		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
> 		/* can be retrieved by unprivileged users */
> 	},
>diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>index 5adac38454fd..e49b82dd77cd 100644
>--- a/net/devlink/devl_internal.h
>+++ b/net/devlink/devl_internal.h
>@@ -122,6 +122,11 @@ struct devlink_nl_dump_state {
> 	};
> };
> 
>+struct devlink_gen_cmd {
>+	int (*dump_one)(struct sk_buff *msg, struct devlink *devlink,
>+			struct netlink_callback *cb);
>+};
>+
> /* Iterate over devlink pointers which were possible to get reference to.
>  * devlink_put() needs to be called for each iterated devlink pointer
>  * in loop body in order to release the reference.
>@@ -138,6 +143,9 @@ struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs);
> void devlink_notify_unregister(struct devlink *devlink);
> void devlink_notify_register(struct devlink *devlink);
> 
>+int devlink_instance_iter_dump(struct sk_buff *msg,
>+			       struct netlink_callback *cb);
>+
> static inline struct devlink_nl_dump_state *
> devl_dump_state(struct netlink_callback *cb)
> {
>@@ -173,6 +181,8 @@ devlink_linecard_get_from_info(struct devlink *devlink, struct genl_info *info);
> void devlink_linecard_put(struct devlink_linecard *linecard);
> 
> /* Rates */
>+extern const struct devlink_gen_cmd devl_gen_rate_get;
>+
> struct devlink_rate *
> devlink_rate_get_from_info(struct devlink *devlink, struct genl_info *info);
> struct devlink_rate *
>diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
>index ce1a7d674d14..fcf10c288480 100644
>--- a/net/devlink/netlink.c
>+++ b/net/devlink/netlink.c
>@@ -5,6 +5,7 @@
>  */
> 
> #include <net/genetlink.h>
>+#include <net/sock.h>
> 
> #include "devl_internal.h"
> 
>@@ -177,6 +178,38 @@ static void devlink_nl_post_doit(const struct genl_split_ops *ops,
> 	devlink_put(devlink);
> }
> 
>+static const struct devlink_gen_cmd *devl_gen_cmds[] = {
>+	[DEVLINK_CMD_RATE_GET]		= &devl_gen_rate_get,
>+};

Instead of having this extra list of ops struct, woudn't it make sence
to rather implement this dumpit_one infra directly as a part of generic
netlink code? Something like:

 	{
 		.cmd = DEVLINK_CMD_RATE_GET,
 		.doit = devlink_nl_cmd_rate_get_doit,
		.dumpit_one = devlink_nl_cmd_rate_get_dumpit_one,
		.dumpit_one_walk = devlink_nl_dumpit_one_walk,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
 		/* can be retrieved by unprivileged users */
 	},

Where devlink_nl_dumpit_one_walk would be basically your
devlink_instance_iter_dump(), it would get an extra arg dumpit_one
function pointer from generic netlink code to call per item:

int devlink_nl_dumpit_one_walk(struct sk_buff *msg, struct netlink_callback *cb,
			       int (*dumpit_one)(struct sk_buff *msg,
						 struct netlink_callback *cb,
						 void *priv));
{
	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
	struct devlink_nl_dump_state *dump = devl_dump_state(cb);
	struct devlink *devlink;
	int err = 0;

	devlink_dump_for_each_instance_get(msg, dump, devlink) {
		devl_lock(devlink);
		err = dumpit_one(msg, cb, devlink);
		devl_unlock(devlink);
		devlink_put(devlink);

		if (err)
			break;

		/* restart sub-object walk for the next instance */
		dump->idx = 0;
	}

	if (err != -EMSGSIZE)
		return err;
	return msg->len;
}



Or we can avoid .dumpit_one_walk() and just have classic .dumpit() which
would get the dumpit_one() pointer cb->dumpit_one (obtainable by
a helper doing a proper check-warn_on on null).


>+
>+int devlink_instance_iter_dump(struct sk_buff *msg, struct netlink_callback *cb)
>+{
>+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>+	struct devlink_nl_dump_state *dump = devl_dump_state(cb);
>+	const struct devlink_gen_cmd *cmd;
>+	struct devlink *devlink;
>+	int err = 0;
>+
>+	cmd = devl_gen_cmds[info->op.cmd];
>+
>+	devlink_dump_for_each_instance_get(msg, dump, devlink) {
>+		devl_lock(devlink);
>+		err = cmd->dump_one(msg, devlink, cb);
>+		devl_unlock(devlink);
>+		devlink_put(devlink);
>+
>+		if (err)
>+			break;
>+
>+		/* restart sub-object walk for the next instance */
>+		dump->idx = 0;
>+	}
>+
>+	if (err != -EMSGSIZE)
>+		return err;
>+	return msg->len;
>+}
>+
> struct genl_family devlink_nl_family __ro_after_init = {
> 	.name		= DEVLINK_GENL_NAME,
> 	.version	= DEVLINK_GENL_VERSION,
>-- 
>2.38.1
>

Powered by blists - more mailing lists