Re: xarray reserve/release?

[Jason Gunthorpe]

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.

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.

What about writing it like this:

   if ((curr == XA_ZERO_ENTRY && old == NULL) || curr == old)

? I can't think of a use case to cmpxchg against real-null only.

And here:
                        xas_store(&xas, entry);
-                       if (xa_track_free(xa))
+                       if (xa_track_free(xa) && !old)
                                xas_clear_mark(&xas, XA_FREE_MARK);

Should this be

    if (xa_track_free(xa) && entry && !old)

? Ie we don't want to clear the XA_FREE_MARK if we just wrote NULL

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?

> > Also, I wonder if xa_reserve() is better written as as
> > 
> >        xa_cmpxchg(xa, index, NULL, XA_ZERO_ENTRY)
> > 
> > Bit clearer what is going on..
> 
> Yes, I agree.  I've pushed a couple of new commits to
> http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray

That looks really readable now that reserve and release are tidy
paired operations.

Thanks,
Jason