On Mon, Feb 04, 2019 at 08:40:50PM -0800, Matthew Wilcox wrote:
> On Mon, Feb 04, 2019 at 09:24:49PM -0700, Jason Gunthorpe wrote:
> > > > +#define xan_for_each(xa, index, entry, filter) \
> > > > + for (index = 0, entry = xan_find(xa, &(index), ULONG_MAX, filter); \
> > > > + !IS_ERR(entry); \
> > > > + (index)++, entry = xan_find(xa, &(index), ULONG_MAX, filter))
> > >
> > > We can't do this in the generic XArray code because ERR_PTR(-ENOENT) might
> > > overlap with an XArray value entry.
> >
> > Oh, I guess it could overlap with a user value here in some bizzaro
> > case.. Maybe xa_err would be better?
>
> Yes, we could use an xa_err value, but all the users we had actively
> didn't want to see the NULL values ... so that was the interface
> provided ;-)
I think the ideal would be to split reserved and NULL, so that an
allocating xarray with a reserved entry is hidden, but a NULL entry is
shown.
Then people who want the NULL hidden can use reserve, which seems
overall clearer to intent.
And normal xarray can just always hide NULL & reserved, that already
seems inherent to its definition.
> > > Now, what you can do is iterate using xas_for_each(). That will return the
> > > zeroed entries.
> > >
> > > XA_STATE(xas, &dev->client_data, 0);
> > > ...
> > > rcu_read_lock();
> > > xas_for_each_marked(&xas, client_data, ULONG_MAX,
> > > CLIENT_DATA_REGISTERED) {
> > > struct ib_client *client = xa_load(&clients, xas.xa_index);
> > > ...
> >
> > Is this all that is needed? Do I have to worry about XA_FREE_MARK or
> > xas_retry()? It wasn't totally clear to me from the docs what was
> > required to use xas_for_each.
>
> Fair. The "Advanced" (I should have called it Plumbing) API is less
> well-documented. If you were going to use the client_data then you'd
> theoretically need to care about retry values, but seeing a retry value
> means "There was something at this entry, but it has now moved".
I never did figure out what causes retries..
> You don't care -- all you care about is there was something stored
> at this index.
I do need the content of the xarray, it is passed in to the callback..
> > > 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;
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);
Doesn't do much for readability :)
BTW, if I'm excluding writers via my own external client_data_rwsem, I
still need RCU & xas_pause, right? This is because of the potential
defragmentation you explained earlier?
Thanks,
Jason
