On Tue, Feb 05, 2019 at 06:21:31PM -0800, Matthew Wilcox wrote:
> 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.
Okay, I see that XA_ZERO_ENTRY and XA_FREE_MARK are exclusive
> What you will need to do is convert 'entry' to NULL if it's a zero
> entry. So perhaps this:
Yes, I forgot about that.
> 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().
This particular array can have a parallel xa_erase, which looks like
it creates the retrys.
Thanks,
Jason
