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
