Re: [PATCH v2 0/4] io_uring/rsrc: coalescing multi-hugepage registered buffers

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

On Sun, 16 Jun 2024 19:04:38 +0100, Pavel Begunkov wrote:
> On 5/14/24 08:54, Chenliang Li wrote:
>> Introduce helper functions to check whether a buffer can
>> be coalesced or not, and gather folio data for later use.
>> 
>> The coalescing optimizes time and space consumption caused
>> by mapping and storing multi-hugepage fixed buffers.
>> 
>> A coalescable multi-hugepage buffer should fully cover its folios
>> (except potentially the first and last one), and these folios should
>> have the same size. These requirements are for easier later process,
>> also we need same size'd chunks in io_import_fixed for fast iov_iter
>> adjust.
>> 
>> Signed-off-by: Chenliang Li <cliang01.li@samsung.com>
>> ---
>>   io_uring/rsrc.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   io_uring/rsrc.h | 10 +++++++
>>   2 files changed, 88 insertions(+)
>> 
>> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
>> index 65417c9553b1..d08224c0c5b0 100644
>> --- a/io_uring/rsrc.c
>> +++ b/io_uring/rsrc.c
>> @@ -871,6 +871,84 @@ static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages,
>>   	return ret;
>>   }
>>   
>> +static bool __io_sqe_buffer_try_coalesce(struct page **pages, int nr_pages,
>> +					 struct io_imu_folio_data *data)
> io_can_coalesce_buffer(), you're not actually trying to
> do it here.

Will change it.

>> +static bool io_sqe_buffer_try_coalesce(struct page **pages, int nr_pages,
>> +				       struct io_imu_folio_data *data)
>> +{
>> +	int i, j;
>> +
>> +	if (nr_pages <= 1 ||
>> +		!__io_sqe_buffer_try_coalesce(pages, nr_pages, data))
>> +		return false;
>> +
>> +	/*
>> +	 * The pages are bound to the folio, it doesn't
>> +	 * actually unpin them but drops all but one reference,
>> +	 * which is usually put down by io_buffer_unmap().
>> +	 * Note, needs a better helper.
>> +	 */
>> +	if (data->nr_pages_head > 1)
>> +		unpin_user_pages(&pages[1], data->nr_pages_head - 1);
> Should be pages[0]. page[1] can be in another folio, and even
> though data->nr_pages_head > 1 protects against touching it,
> it's still flimsy.

But here it is unpinning the tail pages inside those coalesceable folios,
I think we only unpin pages[0] when failure, am I right? And in
__io_sqe_buffer_try_coalesce we have ensured that pages[1:nr_head_pages] are
in same folio and contiguous.

>> +
>> +	j = data->nr_pages_head;
>> +	nr_pages -= data->nr_pages_head;
>> +	for (i = 1; i < data->nr_folios; i++) {
>> +		unsigned int nr_unpin;
>> +
>> +		nr_unpin = min_t(unsigned int, nr_pages - 1,
>> +					data->nr_pages_mid - 1);
>> +		if (nr_unpin == 0)
>> +			break;
>> +		unpin_user_pages(&pages[j+1], nr_unpin);
> same
>> +		j += data->nr_pages_mid;
> And instead of duplicating this voodoo iteration later,
> please just assemble a new compacted ->nr_folios sized
> page array.

Indeed, a new page array would make things a lot easier.
If alloc overhead is not a concern here, then yeah I'll change it.

Thanks,
Chenliang Li