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