Re: [PATCH v3] io_uring/rsrc: add huge page accounting for registered buffers

[Jens Axboe <axboe@kernel.dk>]

On 5/4/26 1:31 AM, Cl?ment L?ger wrote:
>>     static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
>> -            struct iovec *iov, struct page **last_hpage);
>> +                           struct iovec *iov);
>> +
>> +static int hpage_acct_ref(struct io_ring_ctx *ctx, struct page *hpage,
>> +              bool *acct_new)
>> +{
>> +    unsigned long key = (unsigned long) hpage;
>> +    unsigned long count;
>> +    void *entry;
>> +    int ret;
>> +
>> +    lockdep_assert_held(&ctx->uring_lock);
>> +
>> +    entry = xa_load(&ctx->hpage_acct, key);
>> +    if (!entry) {
>> +        ret = xa_reserve(&ctx->hpage_acct, key, GFP_KERNEL_ACCOUNT);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    count = 1;
>> +    if (entry)
>> +        count = xa_to_value(entry) + 1;
> 
> Hi Jens,
> 
> Can't most of this be merged in the previous if/else ? ie:
> 
>     entry = xa_load(&ctx->hpage_acct, key);>
>     count = 1;
>     if (!entry) {
>         ret = xa_reserve(&ctx->hpage_acct, key, GFP_KERNEL_ACCOUNT);
>         if (ret)
>             return ret;
>         *acct_new = true;
>     } else {
>         count = xa_to_value(entry) + 1;
>         *acct_new = false;
>     }

Agree, that's nicer. I'll make that change!

>> +static bool hpage_acct_unref(struct io_ring_ctx *ctx, struct page *hpage)
>> +{
>> +    unsigned long key = (unsigned long) hpage;
>> +    unsigned long count;
>> +    void *entry;
>> +
>> +    lockdep_assert_held(&ctx->uring_lock);
>> +
>> +    entry = xa_load(&ctx->hpage_acct, key);
>> +    if (WARN_ON_ONCE(!entry))
>> +        return false;
>> +    count = xa_to_value(entry);
>> +    if (count == 1)
>> +        xa_erase(&ctx->hpage_acct, key);
>> +    else
>> +        xa_store(&ctx->hpage_acct, key, xa_mk_value(count - 1), GFP_KERNEL_ACCOUNT);
>> +    return count == 1;
> 
> Maybe something like this could easier to read ?:
> 
>     if (count == 1) {
>         xa_erase(&ctx->hpage_acct, key);
>         return true;
>     }
>     
>     xa_store(&ctx->hpage_acct, key, xa_mk_value(count - 1), GFP_KERNEL_ACCOUNT);
>     return false;

Also agree, that's easier to read.

>> @@ -971,7 +1053,6 @@ int io_buffer_register_bvec(struct io_uring_cmd *cmd, struct request *rq,
>>         imu->ubuf = 0;
>>       imu->len = blk_rq_bytes(rq);
>> -    imu->acct_pages = 0;
>>       imu->folio_shift = PAGE_SHIFT;
>>       refcount_set(&imu->refs, 1);
>>       imu->release = release;
>> @@ -1137,6 +1218,56 @@ int io_import_reg_buf(struct io_kiocb *req, struct iov_iter *iter,
>>   }
>>     /* Lock two rings at once. The rings must be different! */
> 
> This comment should be before lock_two_rings().

Yeah, I think this happened during forward porting, the patch is
originally from 3 months ago. I'll fix that up too.

Thanks for taking a look!

-- 
Jens Axboe