xa_cmpxchg and XA_ZERO_ENTRY?

xa_cmpxchg and XA_ZERO_ENTRY?

[Jason Gunthorpe]

Hi Matthew,

Nicolin pointed this out and I was wondering what is the right thing.

For instance this:

	xa_init(&xa);
	ret = xa_reserve(&xa, 1, GFP_KERNEL);
	printk("xa_reserve() = %d\n", ret);
	old = xa_cmpxchg(&xa, 1, NULL, &xa, GFP_KERNEL);
	printk("xa_cmpxchg(NULL) = %llx\n", (u64)old);
	old = xa_load(&xa, 1);
	printk("xa_load() = %llx\n", (u64)old);

Prints out:

xa_reserve() = 0
xa_cmpxchg(NULL) = 0
xa_load() = 0

Meaning the cmpxchg failed because NULL is not stored in the entry due
to the xa_reserve(). So far so good.. So lets correct the code to:

	old = xa_cmpxchg(&xa, 1, XA_ZERO_ENTRY, &xa, GFP_KERNEL);

Then:

xa_reserve() = 0
xa_cmpxchg(XA_ZERO_ENTRY) = 0
xa_load() = ffffa969400d3e68

Ok now it succeed but returned NULL.. (noting NULL != old)

However, if we make an error and omit the xa_reserve step():

xa_cmpxchg(XA_ZERO_ENTRY) = 0
xa_load() = 0

Now it failed and still returned NULL..

So, you can't detect a failure from cmpxchg if old is NULL/ZERO? This
doesn't seem right.

At least I've already fallen in this trap:

static int ucma_destroy_private_ctx(struct ucma_context *ctx)
{
        WARN_ON(xa_cmpxchg(&ctx_table, ctx->id, XA_ZERO_ENTRY, NULL,
                           GFP_KERNEL) != NULL);

Which actually doesn't detect cmpxchg failure..

So what is the right thing here?

The general purpose of code like the above is to just validate that
the xa has not been corrupted, that the index we are storing to has
been reserved. Maybe we can't sleep or something.

Thanks,
Jason

Re: xa_cmpxchg and XA_ZERO_ENTRY?

[Matthew Wilcox]

On Wed, Oct 30, 2024 at 10:15:13AM -0300, Jason Gunthorpe wrote:
> Hi Matthew,
> 
> Nicolin pointed this out and I was wondering what is the right thing.
> 
> For instance this:
> 
> 	xa_init(&xa);
> 	ret = xa_reserve(&xa, 1, GFP_KERNEL);
> 	printk("xa_reserve() = %d\n", ret);
> 	old = xa_cmpxchg(&xa, 1, NULL, &xa, GFP_KERNEL);

You're really not supposed to be doing xa_cmpxchg() here.  Just use
xa_store().  That's the intended way to use these APIs.

> The general purpose of code like the above is to just validate that
> the xa has not been corrupted, that the index we are storing to has
> been reserved. Maybe we can't sleep or something.

Thr intent is to provide you with an array abstraction.  You don't
cmpxchg() pointers into an array, do you?  Almost everybody just does
array[i] = p.

Re: xa_cmpxchg and XA_ZERO_ENTRY?

[Jason Gunthorpe]

On Wed, Oct 30, 2024 at 04:51:40PM +0000, Matthew Wilcox wrote:
> On Wed, Oct 30, 2024 at 10:15:13AM -0300, Jason Gunthorpe wrote:
> > Hi Matthew,
> > 
> > Nicolin pointed this out and I was wondering what is the right thing.
> > 
> > For instance this:
> > 
> > 	xa_init(&xa);
> > 	ret = xa_reserve(&xa, 1, GFP_KERNEL);
> > 	printk("xa_reserve() = %d\n", ret);
> > 	old = xa_cmpxchg(&xa, 1, NULL, &xa, GFP_KERNEL);
> 
> You're really not supposed to be doing xa_cmpxchg() here.  Just use
> xa_store().  That's the intended way to use these APIs.

xa_store() also looses the XA_ZERO_ENTRY, it doesn't help to write an
assertion that the index was reserved.

> > The general purpose of code like the above is to just validate that
> > the xa has not been corrupted, that the index we are storing to has
> > been reserved. Maybe we can't sleep or something.
> 
> Thr intent is to provide you with an array abstraction.  You don't
> cmpxchg() pointers into an array, do you?  Almost everybody just does
> array[i] = p.

Sort of, what is desired here is test and store, not cmpxchg. You
would do that in normal arrays:

 if (WARN_ON(array[i] == NULL)
     return;
 array[i] = foo;

In xarray it would be nice to do both those under a single walk and
lock.

Jason