lkml.org 
[lkml]   [2019]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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

\
 
 \ /
  Last update: 2019-02-20 18:45    [W:0.069 / U:42.472 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site