Re: [PATCH 5.12] io_uring: Convert personality_idr to XArray

[Jens Axboe <axboe@kernel.dk>]

On 3/13/21 12:54 PM, Matthew Wilcox wrote:
> On Sat, Mar 13, 2021 at 12:30:14PM -0700, Jens Axboe wrote:
>> @@ -2851,7 +2852,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len,
>>  			list_del(&kbuf->list);
>>  		} else {
>>  			kbuf = head;
>> -			idr_remove(&req->ctx->io_buffer_idr, bgid);
>> +			__xa_erase(&req->ctx->io_buffer, bgid);
> 
> Umm ... __xa_erase()?  Did you enable all the lockdep infrastructure?
> This should have tripped some of the debugging code because I don't think
> you're holding the xa_lock.

Not run with lockdep - and probably my misunderstanding, do we need xa_lock()
if we provide our own locking?

>> @@ -3993,21 +3994,20 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags)
>>  
>>  	lockdep_assert_held(&ctx->uring_lock);
>>  
>> -	list = head = idr_find(&ctx->io_buffer_idr, p->bgid);
>> +	list = head = xa_load(&ctx->io_buffer, p->bgid);
>>  
>>  	ret = io_add_buffers(p, &head);
>> -	if (ret < 0)
>> -		goto out;
>> +	if (ret >= 0 && !list) {
>> +		u32 id = -1U;
>>  
>> -	if (!list) {
>> -		ret = idr_alloc(&ctx->io_buffer_idr, head, p->bgid, p->bgid + 1,
>> -					GFP_KERNEL);
>> -		if (ret < 0) {
>> +		ret = __xa_alloc_cyclic(&ctx->io_buffer, &id, head,
>> +					XA_LIMIT(0, USHRT_MAX),
>> +					&ctx->io_buffer_next, GFP_KERNEL);
> 
> I don't understand why this works.  The equivalent transformation here
> would have been:
> 
> 		ret = xa_insert(&ctx->io_buffers, p->bgid, head, GFP_KERNEL);
> 
> with various options to handle it differently.

True, that does look kinda weird (and wrong). I'll fix that up.

>>  static void io_destroy_buffers(struct io_ring_ctx *ctx)
>>  {
>> -	idr_for_each(&ctx->io_buffer_idr, __io_destroy_buffers, ctx);
>> -	idr_destroy(&ctx->io_buffer_idr);
>> +	struct io_buffer *buf;
>> +	unsigned long index;
>> +
>> +	xa_for_each(&ctx->io_buffer, index, buf)
>> +		__io_remove_buffers(ctx, buf, index, -1U);
>> +	xa_destroy(&ctx->io_buffer);
> 
> Honestly, I'd do BUG_ON(!xa_empty(&ctx->io_buffers)) if anything.  If that
> loop didn't empty the array, something is terribly wrong and we should
> know about it somehow instead of making the memory leak harder to find.

Probably also my misunderstanding - do I not need to call xa_destroy()
if I prune all the members? Assumed we needed it to free some internal
state, but maybe that's not the case?

-- 
Jens Axboe