On Tue, Feb 05, 2019 at 05:14:52PM -0700, Jason Gunthorpe wrote:
> On Mon, Feb 04, 2019 at 10:15:20PM -0700, Jason Gunthorpe wrote:
> > > > > I'm not familiar enough with your code to figure out where you'd want
> > > > > to rcu_read_unlock() :-)
> > > >
> > > > Hum, it barely matters, but just after the if would be fine and
> > > > slightly fewer pauses. This thing only has about 5 entries in it. I
> > > > often wonder why it was made dynamic at all.
> > >
> > > Cool. Just remember to call xas_reset() if you drop the rcu read lock
> > > and continue the loop.
> >
> > xas_reset? Did you mean xas_pause?
> >
> > I guess the full version looks like this:
> >
> > down_read(&dev->client_data_rwsem);
> > rcu_read_lock();
> > xas_for_each_marked(&xas, client_data, ULONG_MAX,
> > CLIENT_DATA_REGISTERED) {
> > struct ib_client *client;
> >
> > if (!xa_is_zero(entry) && xas_retry(&xas, client_data))
> > continue;
>
> Rather looks like this needs to check for XA_FREE_MARK too...
No. XA_FREE_MARK is only set on NULL entries, which xas_for_each()
won't return. What you will need to do is convert 'entry' to NULL
if it's a zero entry. So perhaps this:
xas_for_each_marked(&xas, client_data, ULONG_MAX,
CLIENT_DATA_REGISTERED) {
struct ib_client *client;
if (xa_is_zero(client_data))
client_data = NULL;
else if (xas_retry(&xas, client_data))
continue;
But if I understood you correctly, the array can't change while you're holding
the read semaphore. So you don't need to check for xas_retry().
> > client = xa_load(&clients, xas.xa_index);
> > if (!client || !client->get_net_dev_by_params)
> > continue;
> >
> > xas_pause();
> > rcu_read_unlock();
> >
> > net_dev = client->get_net_dev_by_params(dev, port, pkey, gid,
> > addr, client_data);
> > rcu_read_lock();
> > if (net_dev)
> > break;
> >
> > }
> > rcu_read_unlock();
> > up_read(&dev->client_data_rwsem);
