Re: [RFC net-next 01/10] devlink: bump the instance index directly when iterating

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote:
> >-struct devlink *
> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
> >-					  unsigned long, xa_mark_t))
> >+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
> > {
> >-	struct devlink *devlink;
> >+	struct devlink *devlink = NULL;
> > 
> > 	rcu_read_lock();
> > retry:
> >-	devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
> >+	devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
> > 	if (!devlink)
> > 		goto unlock;
> > 
> >@@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> > 	 * This prevents live-lock of devlink_unregister() wait for completion.
> > 	 */
> > 	if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
> >-		goto retry;
> >+		goto next;
> > 
> >-	/* For a possible retry, the xa_find_after() should be always used */
> >-	xa_find_fn = xa_find_after;  
> 
> Hmm. Any idea why xa_find_after()? implementation is different to
> xa_find()?

I'm guessing it's because for _after the code needs to take special
care of skipping multi-index entries. If an entry spans 0-3 xa_find(0)
can return the same entry as xa_find(1). But xa_find_after(1) must
return an entry under index 4 or higher.

Since our use of the Xarray is very trivial with no range indexes,
it should not matter.

Let me CC Matthew, just in case. The question boils down to whether:

	for (index = 0; (entry = xa_find(net, &index));	index++)  

is a legal way of iterating over an Xarray without range indexes.

> > 	if (!devlink_try_get(devlink))
> >-		goto retry;
> >+		goto next;
> > 	if (!net_eq(devlink_net(devlink), net)) {
> > 		devlink_put(devlink);
> >-		goto retry;
> >+		goto next;
> > 	}
> > unlock:
> > 	rcu_read_unlock();
> > 	return devlink;
> >-}
> >-
> >-struct devlink *
> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp)
> >-{
> >-	return devlinks_xa_find_get(net, indexp, xa_find);
> >-}
> > 
> >-struct devlink *
> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp)
> >-{
> >-	return devlinks_xa_find_get(net, indexp, xa_find_after);
> >+next:
> >+	(*indexp)++;
> >+	goto retry;
> > }
> > 
> > /**
> >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> >index 1d7ab11f2f7e..ef0369449592 100644
> >--- a/net/devlink/devl_internal.h
> >+++ b/net/devlink/devl_internal.h
> >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family;
> >  * in loop body in order to release the reference.
> >  */
> > #define devlinks_xa_for_each_registered_get(net, index, devlink)	\
> >-	for (index = 0,							\
> >-	     devlink = devlinks_xa_find_get_first(net, &index);	\
> >-	     devlink; devlink = devlinks_xa_find_get_next(net, &index))
> >-
> >-struct devlink *
> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
> >-					  unsigned long, xa_mark_t));
> >-struct devlink *
> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp);
> >-struct devlink *
> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp);
> >+	for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)