Date:   Wed, 20 Feb 2019 12:47:26 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: xarray reserve/release?

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.

> 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().

> 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

There are four cases to consider: old is NULL/present, entry is NULL/present.
xas_store() will set XA_FREE_MARK (by calling xas_init_marks()) if entry
is NULL.  Otherwise it leaves the marks alone.

old	entry	FREE before	FREE after xas_store	should be FREE
NULL	NULL	true		true			true
NULL	present	true		true			false
present	NULL	false		true			true
present	present	false		false			false

So the only case we need to clear the bit is if old is NULL and entry is
present.  I didn't consider the NULL->NULL transition, so you're right.

> 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.

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

Great!

Powered by blists - more mailing lists