Re: [PATCH net-next 09/14] devlink: restart dump based on devlink instance ids (simple)

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 4 Jan 2023 15:18:54 +0100 Jiri Pirko wrote:
> >@@ -117,6 +122,15 @@ struct devlink_nl_dump_state {
> > 	};
> > };
> > 
> >+/* 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.
> >+ */
> >+#define devlink_dump_for_each_instance_get(msg, dump, devlink)		\
> >+	for (; (devlink = devlinks_xa_find_get(sock_net(msg->sk),	\  
> 
> I undestand that the "dump" is zeroed at the beginning of dumpit call,
> however, if you call this helper multiple times, the second iteration
> would't not work.
> 
> Perhaps better to initialize instance=0 at the beginning of the loop to
> make this helper calls behaviour independent on context.

Mmm. dump is zeroed when the operation starts. If there are multiple
->dumpit calls / skbs - the dump will not get re-started.

Restarting the instance walk would be tricky in practice (the
subsequent ->dumpit calls would need to know if they are pre- or post-
restarted) - so I don't think we should anticipate having to do this.

Let me rewrite to comment to make the dumpit-only-ness explicit.

> >+					       &dump->instance, xa_find)); \
> >+	     dump->instance++)
> >+
> > extern const struct genl_small_ops devlink_nl_ops[56];
> > 
> > struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs);
> >diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
> >index e3cfb64990b4..0f24b321b0bb 100644
> >--- a/net/devlink/leftover.c
> >+++ b/net/devlink/leftover.c
> >@@ -1319,17 +1319,9 @@ static int devlink_nl_cmd_get_dumpit(struct sk_buff *msg,
> > {
> > 	struct devlink_nl_dump_state *dump = devl_dump_state(cb);
> > 	struct devlink *devlink;
> >-	unsigned long index;
> >-	int idx = 0;
> > 	int err;
> > 
> >-	devlinks_xa_for_each_registered_get(sock_net(msg->sk), index, devlink) {
> >-		if (idx < dump->idx) {
> >-			idx++;
> >-			devlink_put(devlink);
> >-			continue;
> >-		}
> >-
> >+	devlink_dump_for_each_instance_get(msg, dump, devlink) {  
> 
> The name suggests on the first sight that you are iterating some dump,
> which is slightly confusing. Perhaps better to have
> "devlinks_xa_for_each_" in the prefix somehow?
> 
> 	devlinks_xa_for_each_registered_get_dumping()
> 
> I know it is long :)

It's only usable in netlink dumps and with clear semantic implications.
More generic name is not appropriate IMO.
Maybe after you remove the line card locks we could remove the macro
completely? It only has 3 users after my patches - on in the generic
dump handling and one for the subobjects which have their own locks.