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

Jiri Pirko
2023-01-04T10:04:44Z

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;

[..]