Re: xarray reserve/release?

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Feb 20, 2019 at 12:47:26PM -0800, Matthew Wilcox wrote:
> On Wed, Feb 20, 2019 at 10:43:33AM -0700, Jason Gunthorpe wrote:
> > On Wed, Feb 20, 2019 at 09:14:14AM -0800, Matthew Wilcox wrote:
> > > > void __xa_release(struct xarray *xa, unsigned long index)
> > > > {
> > > > 	XA_STATE(xas, xa, index);
> > > > 	void *curr;
> > > > 
> > > > 	curr = xas_load(&xas);
> > > > 	if (curr == XA_ZERO_ENTRY)
> > > > 		xas_store(&xas, NULL);
> > > > }
> > > > 
> > > > ?
> > > 
> > > I decided to instead remove the magic from xa_cmpxchg().  I used
> > > to prohibit any internal entry being passed to the regular API, but
> > > I recently changed that with 76b4e5299565 ("XArray: Permit storing
> > > 2-byte-aligned pointers").  Now that we can pass XA_ZERO_ENTRY, I
> > > think this all makes much more sense.
> > 
> > Except that for allocating arrays xa_cmpxchg and xa_store now do
> > different things with NULL. Not necessarily bad, but if you have this
> > ABI variation it should be mentioned in the kdoc comment.
> 
> I'm worrying about the whole xa_store(... NULL, gfp) situation.  Before
> I realised I needed to unify the XArray and the IDR (I'd originally
> intended to have the IDR be a client of the XArray the same way that
> it was a client of the radix tree), everything was nice and simple.
> xa_erase() was a synonym for xa_store(... NULL, gfp).  Then the IDR
> users showed up with their understanding of what storing NULL meant,
> and now xa_store(NULL) means something different depending what kind of
> array you have.  This sucks.  I'm tempted to have xa_store(NULL) always
> transform the NULL into XA_ZERO_ENTRY, but I worry I might break some
> users without noticing.

So you will end up with xa_store, xa_insert, xa_alloc doing the
conversion on store, and xa_cmpxchg not doing it.

I'd excuse xa_alloc/xa_insert, as anyone calling them surely means to
reserve the entry. The comment for xa_insert even says it does
this. Great.

xa_store(NULL) == xa_erase(), sometimes, is weird, IMHO. Two APIs
would be clearer: xa_store() which always does the XA_ZERO_ENTRY and
xa_store_erase() which always does xa_erase() if the argument is
NULL.

This makes the itent of the call site super clear without having to
find the xa_init and check the mode.

If this was done then xa_track_free() would only cause the mark to be
updated, or not, and has no other behavior change.

For completeness xa_cmpxchg() should probably get a similar comment as
xa_insert():

  If the predicate matches then a NULL entry will do xa_erase() on the
  index. Otherwise the value is stored.

  When matching the predicate the value NULL only matches unreserved
  entries. [this more or less matches xa_insert anyhow]

> > This is a bit worrysome though:
> > 
> >                 curr = xas_load(&xas);
> > -               if (curr == XA_ZERO_ENTRY)
> > -                       curr = NULL;
> >                 if (curr == old) {
> > 
> > It means any cmpxchg user has to care explicitly about the possibility
> > for true-NULL vs reserved. Seems like a difficult API.
> 
> I think the users know, though.  I went through the current users of
> xa_cmpxchg() and they're not the same users which are using xa_reserve()
> or xa_alloc().

Reflecting on this more, the use case I was looking at was basically

   // First thread here wins ownership of 'id' and does reserve
   ret = xa_cmpxchg(xa, id NULL, XA_ZERO_ENTRY)
   if (ret != NULL)
       return;

   ...

   // Can't fail
   xa_store(xa, id, something);

So that actually works better the way you have it.

Can always have another cmpxchg varient if a use comes up.

> > Also I would think !curr is clearer? I assume the point is to not pay
> > the price of xas_clear_mark if we already know the index stored is
> > marked?
> 
> If you find it clearer, I'll use 'curr'.  They're equal at this point anyway.

Yeah, it brings to mind your table more directly then having to also
work out that old == curr == xa_load ==> optimizing away unneeded clear

BTW, will you send conversion patches for drivers/infiniband sometime
soonish? Feel like I know enough about xarray to review them these
days

Thanks,
Jason