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

[Chenliang Li <cliang01.li@samsung.com>]

On Sat, 11 May 2024 10:48:18 -0600 Jens Axboe wrote:
> 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.

will delete that in V3.

>> +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;

Yes, will change it in V3.

>> +	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;

Will change it.

>> +	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.

Will change it.