Re: [PATCH v6 2/2] io_uring/rsrc: enable multi-hugepage buffer coalescing

[Pavel Begunkov <asml.silence@gmail.com>]

On 7/16/24 07:08, Chenliang Li wrote:
> Add support for checking and coalescing multi-hugepage-backed fixed
> buffers. The coalescing optimizes both 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 processing later,
> 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>
> ---

The series looks good apart from the comment below, I'll run some
tests this week, I think you sent something for liburing/test

> +static bool io_try_coalesce_buffer(struct page ***pages, int *nr_pages,
> +					 struct io_imu_folio_data *data)
> +{
> +	struct page **page_array = *pages;
> +	struct folio *folio = page_folio(page_array[0]);
> +	unsigned int count = 1, nr_folios = 1;
> +	int i;
> +
> +	if (*nr_pages <= 1)
> +		return false;
> +
> +	data->nr_pages_mid = folio_nr_pages(folio);
> +	if (data->nr_pages_mid == 1)
> +		return false;
> +
> +	data->folio_shift = folio_shift(folio);
> +	/*
> +	 * Check if pages are contiguous inside a folio, and all folios have
> +	 * the same page count except for the head and tail.
> +	 */
> +	for (i = 1; i < *nr_pages; i++) {
> +		if (page_folio(page_array[i]) == folio &&
> +			page_array[i] == page_array[i-1] + 1) {
> +			count++;
> +			continue;
> +		}
> +
> +		if (nr_folios == 1) {
> +			if (folio_page_idx(folio, page_array[i-1])
> +				!= data->nr_pages_mid - 1)

Code style, comparison (i.e. "!=") should be on the previous line

> +				return false;
> +
> +			data->nr_pages_head = count;
> +		} else if (count != data->nr_pages_mid) {
> +			return false;
> +		}
> +
> +		folio = page_folio(page_array[i]);
> +		if (folio_size(folio) != (1UL << data->folio_shift) ||
> +			folio_page_idx(folio, page_array[i]) != 0)
> +			return false;
> +
> +		count = 1;
> +		nr_folios++;
> +	}
> +	if (nr_folios == 1) {
> +		if (folio_page_idx(folio, page_array[i-1])
> +			!= data->nr_pages_mid - 1)

That's too restrictive and would be a regression. We currently allow
registrations fully falling under a single huge page but not matching
the right side. E.g. for a [N; M) huge page, you're allowed to register
[N + 4KB, M - 4KB). We should just remove this check.

And same comment about code style.


> +			return false;
> +
> +		data->nr_pages_head = count;
> +	}
> +
> +	return io_do_coalesce_buffer(pages, nr_pages, data, nr_folios);
> +}
> +
-- 
Pavel Begunkov