On Thu, Feb 21, 2019 at 05:43:37PM -0800, Matthew Wilcox wrote:
> On Thu, Feb 21, 2019 at 12:12:58PM -0700, Jason Gunthorpe wrote:
> > On Thu, Feb 21, 2019 at 11:06:58AM -0800, Matthew Wilcox wrote:
> > > On Thu, Feb 21, 2019 at 11:37:02AM -0700, Jason Gunthorpe wrote:
> > > > What does the GFP flags == 0 do? I've seen you use this as a marker
> > > > for stores that cannot fail?
> > >
> > > It's for stores which we know we're just overwriting, so there won't
> > > be any need to allocate memory. But I've started to move away from
> > > this idea -- it's a quirk of the current implementation that it won't
> > > need to allocate memory, and not even guaranteed true by the current
> > > implementation (if you try to store a 2-byte aligned pointer in index 0,
> > > it will have to allocate memory).
> > >
> > > So I think I'm actually going to replace this 0 with a GFP_KERNEL (I just
> > > checked the calling context, and it can absolutely sleep to allocate
> > > memory).
> >
> > In that case the error code has to be returned here..
> >
> > But, IMHO, a major reason to use xa_reserve is to create a situation
> > where store can't fail, so it is disappointing to hear that isn't
> > possible anymore.
>
> You're right. This is just a bug, and I've fixed it (see the head of the
> xarray tree).
>
> But I still want to pass in GFP_KERNEL here. For the replacement data
> structure I'm working on to replace the radix tree, there will be times
> when being able to reallocate nodes will be advantageous. It'll never
> be necessary, so it can't return an error, but it may lead to a more
> balanced tree.
I still fell like it would be clearer & safer to have a xa_replace()
type API for this case. Easier to understand what the contract is, and
easier for things like syzkaller to trigger the correctness check.
/*
* xa_replace() can only be called on an entry that already has a value or is
* reserved. It replaces the value with entry and cannot fail. If gfp flags
* are specified the function may optionally reallocate memory to optimize the
* storage (future)
*/
void __xa_replace(struct xarray *xa, unsigned long index, void *entry, gfp_t gfp)
{
XA_STATE(xas, xa, index);
void *curr;
if (WARN_ON_ONCE(xa_is_advanced(entry)))
return -EINVAL;
if (!entry)
entry = XA_ZERO_ENTRY;
do {
curr = xas_load(&xas);
WARN_ON(curr == NULL);
xas_store(&xas, entry);
if (xa_track_free(xa))
xas_clear_mark(&xas, XA_FREE_MARK);
} while (WARN_ON(__xas_nomem(&xas, gfp)));
}
Jason
