Messages in this thread | | | Date | Wed, 20 Feb 2019 10:43:33 -0700 | From | Jason Gunthorpe <> | Subject | Re: xarray reserve/release? |
| |
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
| |