Re: [PATCH net-next 05/14] devlink: use an explicit structure for dump context
From: Jiri Pirko <jiri@resnulli.us>
Date: 2023-01-04 10:04:44
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).
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+{ + 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;
[..]