Re: [PATCH net-next 05/14] devlink: use an explicit structure for dump context

[Jiri Pirko <jiri@resnulli.us>]

Wed, Jan 04, 2023 at 05:16:27AM CET, kuba@kernel.org wrote:
>Create a dump context structure instead of using cb->args
>as an unsigned long array. This is a pure conversion which
>is intended to be as much of a noop as possible.
>Subsequent changes will use this to simplify the code.
>
>The two non-trivial parts are:
> - devlink_nl_cmd_health_reporter_dump_get_dumpit() checks args[0]
>   to see if devlink_fmsg_dumpit() has already been called (whether
>   this is the first msg), but doesn't use the exact value, so we
>   can drop the local variable there already
> - devlink_nl_cmd_region_read_dumpit() uses args[0] for address
>   but we'll use args[1] now, shouldn't matter

I don't follow this. Where do you use args[1]? you mean
dump->start_offset? If yes, it does not matter at all and I think
mentioning that only confuses reader (as it did for me).


>
>Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> net/devlink/devl_internal.h | 23 +++++++++
> net/devlink/leftover.c      | 98 ++++++++++++++++++++++---------------
> 2 files changed, 81 insertions(+), 40 deletions(-)
>
>diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>index bc7df9b0f775..91059311f18d 100644
>--- a/net/devlink/devl_internal.h
>+++ b/net/devlink/devl_internal.h
>@@ -107,6 +107,21 @@ enum devlink_multicast_groups {
> 	DEVLINK_MCGRP_CONFIG,
> };
> 
>+/* state held across netlink dumps */
>+struct devlink_nl_dump_state {
>+	int idx;
>+	union {
>+		/* DEVLINK_CMD_REGION_READ */
>+		struct {
>+			u64 start_offset;
>+		};
>+		/* DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET */
>+		struct {
>+			u64 dump_ts;
>+		};
>+	};
>+};
>+
> extern const struct genl_small_ops devlink_nl_ops[56];
> 
> struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs);
>@@ -114,6 +129,14 @@ 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);
> 
>+static inline struct devlink_nl_dump_state *
>+devl_dump_state(struct netlink_callback *cb)

What is the convesion you established again regarding "devl_" and
"devlink_" prefixes? I don't find it written down anywhere and honestly
it confuses me a bit.



>+{
>+	NL_ASSET_DUMP_CTX_FITS(struct devlink_nl_dump_state);
>+
>+	return (struct devlink_nl_dump_state *)cb->ctx;
>+}
>+
> /* Ports */
> int devlink_port_netdevice_event(struct notifier_block *nb,
> 				 unsigned long event, void *ptr);
>diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
>index e01ba7999b91..bcc930b7cfcf 100644
>--- a/net/devlink/leftover.c
>+++ b/net/devlink/leftover.c
>@@ -1222,9 +1222,10 @@ static void devlink_rate_notify(struct devlink_rate *devlink_rate,
> static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
> 					  struct netlink_callback *cb)
> {
>+	struct devlink_nl_dump_state *dump = devl_dump_state(cb);

Could this be named "state" or "dump_state"? "dump" is not what it is.



> 	struct devlink_rate *devlink_rate;
> 	struct devlink *devlink;
>-	int start = cb->args[0];
>+	int start = dump->idx;
> 	unsigned long index;
> 	int idx = 0;
> 	int err = 0;

[..]