Re: [PATCH net-next 13/14] devlink: add by-instance dump infra

Thu, Jan 05, 2023 at 04:46:04AM CET, kuba@kernel.org wrote:

On Wed, 4 Jan 2023 17:50:33 +0100 Jiri Pirko wrote:

quoted

Wed, Jan 04, 2023 at 05:16:35AM CET, kuba@kernel.org wrote:

quoted

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.

quoted

quoted

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 {  

What is "gen"? Generic netlink?

Generic devlink command. In other words the implementation 
is straightforward enough to factor out the common parts.

Could it be "genl" then?

quoted

Not sure why perhaps "nl" would be fine to be consistent with the
rest of the code? Why "cmd"? That looks a bit odd to me.

quoted

+	int (*dump_one)(struct sk_buff *msg, struct devlink
*devlink,
+			struct netlink_callback *cb);  

Do you plan to have more callbacks here? If no, wouldn't it be better
to just have typedef and assign the pointer to the dump_one in
devl_gen_cmds array?

If I find the time - yes, more refactoring is possible.

Could you elaborate a bit more about that?

quoted

quoted

+};
+
/* 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;  

The struct name is *_cmd, not sure why the variable name is *_get
Shouldn't it be rather devl_gen_cmd_rate?

It is the implementation of get.. there's also a set command.. 
which would be under a different index...

I see.

quoted

quoted

+			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,  

dump_one/dumpinst inconsistency in names

Sure...

quoted

quoted

+};
+
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,  

again, inconsistency:
devlink_instance_iter_dumpit

You mean it doesn't have nl, cmd, dump_one in the name?
Could you *please* at least say what you want the names to be if you're
sending all those subjective nit picks? :/

Well, I provided a suggested name, not sure why that was not clear.
The point was s/dump/dumpit/ to match the op name.

I'm sorry you find this a subjective nitpick. I believe it is important
to maintain some naming consistency in order to make code readable.
Nothing subjective about it.

I'll call it devlink_nl_instance_iter_dump

devlink_nl_instance_iter_dumpit please.

quoted

quoted

		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
		/* can be retrieved by unprivileged users */  

Unrelated to this patch, I wonder, why you didn't move devlink_nl_ops
along with the rest of the netlink code to netlink.c?

It's explained in the commit message for patch 3 :/

I missed that, sorry.

quoted

quoted

	},

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,

static const devlink_nl_dump_one_t *devlink_nl_dump_one[] = {
	[DEVLINK_CMD_RATE_GET]	= &devl_nl_rate_dump_one,
}
Maybe? (not sure how the devlink/devl should be used here though)

Nope.

Awesome.