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

Jakub Kicinski
2023-01-02T22:57:04Z

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++)