Re: [PATCH v2 3/4] io_uring/rsrc: add init and account functions for coalesced imus

[Jens Axboe <axboe@kernel.dk>]

On 5/10/24 11:52 PM, Chenliang Li wrote:
> This patch depends on patch 1 and 2.

What does "patch 1 and 2" mean here, once it's in the git log? It
doesn't really mean anything. It's quite natural for patches in a series
to have dependencies on each other, eg patch 3 requirest 1 and 2.
Highlighting it doesn't really add anything, so just get rid of that.

> Introduces two functions to separate the coalesced imu alloc and
> accounting path from the original one. This helps to keep the original
> code path clean.
> 
> Signed-off-by: Chenliang Li <cliang01.li@samsung.com>
> ---
>  io_uring/rsrc.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> index 578d382ca9bc..7f95eba72f1c 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -871,6 +871,42 @@ static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages,
>  	return ret;
>  }
>  
> +static int io_coalesced_buffer_account_pin(struct io_ring_ctx *ctx,
> +					   struct page **pages,
> +					   struct io_mapped_ubuf *imu,
> +					   struct page **last_hpage,
> +					   struct io_imu_folio_data *data)
> +{
> +	int i, j, ret;
> +
> +	imu->acct_pages = 0;
> +	j = 0;
> +	for (i = 0; i < data->nr_folios; i++) {
> +		struct page *hpage = pages[j];
> +
> +		if (hpage == *last_hpage)
> +			continue;
> +		*last_hpage = hpage;
> +		/*
> +		 * Already checked the page array in try coalesce,
> +		 * so pass in nr_pages=0 here to waive that.
> +		 */
> +		if (headpage_already_acct(ctx, pages, 0, hpage))
> +			continue;
> +		imu->acct_pages += data->nr_pages_mid;
> +		j += (i == 0) ?
> +			data->nr_pages_head : data->nr_pages_mid;

Can we just initialize 'j' to data->nr_pages_head and change this to be:

	if (i)
		j += data->nr_pages_mid;

That would be a lot cleaner.

> +	if (!imu->acct_pages)
> +		return 0;
> +
> +	ret = io_account_mem(ctx, imu->acct_pages);
> +	if (ret)
> +		imu->acct_pages = 0;
> +	return ret;
> +}

	ret = io_account_mem(ctx, imu->acct_pages);
	if (!ret)
		return 0;
	imu->acct_pages = 0;
	return ret;

> +				  struct io_mapped_ubuf **pimu,
> +				  struct page **last_hpage, struct page **pages,
> +				  struct io_imu_folio_data *data)
> +{
> +	struct io_mapped_ubuf *imu = NULL;
> +	unsigned long off;
> +	size_t size, vec_len;
> +	int ret, i, j;
> +
> +	ret = -ENOMEM;
> +	imu = kvmalloc(struct_size(imu, bvec, data->nr_folios), GFP_KERNEL);
> +	if (!imu)
> +		return ret;
> +
> +	ret = io_coalesced_buffer_account_pin(ctx, pages, imu, last_hpage,
> +						data);
> +	if (ret) {
> +		j = 0;
> +		for (i = 0; i < data->nr_folios; i++) {
> +			unpin_user_page(pages[j]);
> +			j += (i == 0) ?
> +				data->nr_pages_head : data->nr_pages_mid;
> +		}
> +		return ret;

Same comment here.

-- 
Jens Axboe