xa_cmpxchg and XA_ZERO_ENTRY?

[Jason Gunthorpe <jgg@xxxxxxxxxx>]

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