[PATCH 0/3] registrered buffer coalescing cleanup

[PATCH 0/3] registrered buffer coalescing cleanup

[Pavel Begunkov]

Improve how we handle huge page with registrered buffers, in particular
make io_coalesce_buffer() a bit more readable.

Pavel Begunkov (3):
  io_uring/rsrc: use unpin_user_folio
  io_uring/rsrc: clean up io_coalesce_buffer()
  io_uring/rsrc: remove null check on import

 io_uring/rsrc.c | 50 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

-- 
2.48.1

[PATCH 1/3] io_uring/rsrc: use unpin_user_folio

[Pavel Begunkov]

We want to have a full folio to be left pinned but with only one
reference, for that we "unpin" all but the first page with
unpin_user_pages(), which can be confusing. There is a new helper to
achieve that called unpin_user_folio(), so use that.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/rsrc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index f80a77c4973f..40061a31cc1f 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -699,10 +699,9 @@ static bool io_coalesce_buffer(struct page ***pages, int *nr_pages,
 	 * 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(&page_array[1], data->nr_pages_head - 1);
+		unpin_user_folio(page_folio(new_array[0]), data->nr_pages_head - 1);
 
 	j = data->nr_pages_head;
 	nr_pages_left -= data->nr_pages_head;
@@ -713,7 +712,7 @@ static bool io_coalesce_buffer(struct page ***pages, int *nr_pages,
 		nr_unpin = min_t(unsigned int, nr_pages_left - 1,
 					data->nr_pages_mid - 1);
 		if (nr_unpin)
-			unpin_user_pages(&page_array[j+1], nr_unpin);
+			unpin_user_folio(page_folio(new_array[i]), nr_unpin);
 		j += data->nr_pages_mid;
 		nr_pages_left -= data->nr_pages_mid;
 	}
-- 
2.48.1

[PATCH 2/3] io_uring/rsrc: clean up io_coalesce_buffer()

[Pavel Begunkov]

We don't need special handling for the first page in
io_coalesce_buffer(), move it inside the loop.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/rsrc.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 40061a31cc1f..21613e6074d4 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -685,37 +685,34 @@ static bool io_coalesce_buffer(struct page ***pages, int *nr_pages,
 				struct io_imu_folio_data *data)
 {
 	struct page **page_array = *pages, **new_array = NULL;
-	int nr_pages_left = *nr_pages, i, j;
-	int nr_folios = data->nr_folios;
+	unsigned nr_pages_left = *nr_pages;
+	unsigned nr_folios = data->nr_folios;
+	unsigned i, j;
 
 	/* Store head pages only*/
-	new_array = kvmalloc_array(nr_folios, sizeof(struct page *),
-					GFP_KERNEL);
+	new_array = kvmalloc_array(nr_folios, sizeof(struct page *), GFP_KERNEL);
 	if (!new_array)
 		return false;
 
-	new_array[0] = compound_head(page_array[0]);
-	/*
-	 * 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().
-	 */
-	if (data->nr_pages_head > 1)
-		unpin_user_folio(page_folio(new_array[0]), data->nr_pages_head - 1);
-
-	j = data->nr_pages_head;
-	nr_pages_left -= data->nr_pages_head;
-	for (i = 1; i < nr_folios; i++) {
-		unsigned int nr_unpin;
-
-		new_array[i] = page_array[j];
-		nr_unpin = min_t(unsigned int, nr_pages_left - 1,
-					data->nr_pages_mid - 1);
-		if (nr_unpin)
-			unpin_user_folio(page_folio(new_array[i]), nr_unpin);
-		j += data->nr_pages_mid;
-		nr_pages_left -= data->nr_pages_mid;
+	for (i = 0, j = 0; i < nr_folios; i++) {
+		struct page *p = compound_head(page_array[j]);
+		struct folio *folio = page_folio(p);
+		unsigned int nr;
+
+		WARN_ON_ONCE(i > 0 && p != page_array[j]);
+
+		nr = i ? data->nr_pages_mid : data->nr_pages_head;
+		nr = min(nr, nr_pages_left);
+		/* Drop all but one ref, the entire folio will remain pinned. */
+		if (nr > 1)
+			unpin_user_folio(folio, nr - 1);
+		j += nr;
+		nr_pages_left -= nr;
+		new_array[i] = p;
 	}
+
+	WARN_ON_ONCE(j != *nr_pages);
+
 	kvfree(page_array);
 	*pages = new_array;
 	*nr_pages = nr_folios;
-- 
2.48.1

[PATCH 3/3] io_uring/rsrc: remove null check on import

[Pavel Begunkov]

WARN_ON_ONCE() checking imu for NULL in io_import_fixed() is in the hot
path and too protective, it's time to get rid of it.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/rsrc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 21613e6074d4..b2b9053b31c7 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -1058,8 +1058,6 @@ static int io_import_fixed(int ddir, struct iov_iter *iter,
 	size_t offset;
 	int ret;
 
-	if (WARN_ON_ONCE(!imu))
-		return -EFAULT;
 	ret = validate_fixed_range(buf_addr, len, imu);
 	if (unlikely(ret))
 		return ret;
-- 
2.48.1

Re: [PATCH 1/3] io_uring/rsrc: use unpin_user_folio

[Anuj gupta]

Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>

Re: [PATCH 2/3] io_uring/rsrc: clean up io_coalesce_buffer()

[Anuj gupta]

Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>

Re: [PATCH 0/3] registrered buffer coalescing cleanup

[Jens Axboe]

On Sat, 19 Apr 2025 18:47:03 +0100, Pavel Begunkov wrote:
> Improve how we handle huge page with registrered buffers, in particular
> make io_coalesce_buffer() a bit more readable.
> 
> Pavel Begunkov (3):
>   io_uring/rsrc: use unpin_user_folio
>   io_uring/rsrc: clean up io_coalesce_buffer()
>   io_uring/rsrc: remove null check on import
> 
> [...]

Applied, thanks!

[1/3] io_uring/rsrc: use unpin_user_folio
      commit: ea76925614189bdcb6571e2ea8de68af409ebd56
[2/3] io_uring/rsrc: clean up io_coalesce_buffer()
      commit: 9cebcf7b0c38bca1b501d8716163aa254b230559
[3/3] io_uring/rsrc: remove null check on import
      commit: be6bad57b217491733754ae8113eec94a90a2769

Best regards,
-- 
Jens Axboe

Re: [PATCH 2/3] io_uring/rsrc: clean up io_coalesce_buffer()

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

On 19/04/25 06:47PM, Pavel Begunkov wrote:
>We don't need special handling for the first page in
>io_coalesce_buffer(), move it inside the loop.
>
>Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>---
> io_uring/rsrc.c | 47 ++++++++++++++++++++++-------------------------
> 1 file changed, 22 insertions(+), 25 deletions(-)
>
>diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
>index 40061a31cc1f..21613e6074d4 100644
>--- a/io_uring/rsrc.c
>+++ b/io_uring/rsrc.c
>@@ -685,37 +685,34 @@ static bool io_coalesce_buffer(struct page ***pages, int *nr_pages,
> 				struct io_imu_folio_data *data)
> {
> 	struct page **page_array = *pages, **new_array = NULL;
>-	int nr_pages_left = *nr_pages, i, j;
>-	int nr_folios = data->nr_folios;
>+	unsigned nr_pages_left = *nr_pages;
>+	unsigned nr_folios = data->nr_folios;
>+	unsigned i, j;
checkpatch.pl complains about just "unsigned", "unsigned int" is preferred.

>
> 	/* Store head pages only*/
>-	new_array = kvmalloc_array(nr_folios, sizeof(struct page *),
>-					GFP_KERNEL);
>+	new_array = kvmalloc_array(nr_folios, sizeof(struct page *), GFP_KERNEL);
Not sure whether 80 line length is preferred in uring subsystem, if yes
then this breaks it.

--Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/3] io_uring/rsrc: clean up io_coalesce_buffer()

[Jens Axboe]

On 4/21/25 5:50 AM, Nitesh Shetty wrote:
> On 19/04/25 06:47PM, Pavel Begunkov wrote:
>> We don't need special handling for the first page in
>> io_coalesce_buffer(), move it inside the loop.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>> io_uring/rsrc.c | 47 ++++++++++++++++++++++-------------------------
>> 1 file changed, 22 insertions(+), 25 deletions(-)
>>
>> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
>> index 40061a31cc1f..21613e6074d4 100644
>> --- a/io_uring/rsrc.c
>> +++ b/io_uring/rsrc.c
>> @@ -685,37 +685,34 @@ static bool io_coalesce_buffer(struct page ***pages, int *nr_pages,
>>                 struct io_imu_folio_data *data)
>> {
>>     struct page **page_array = *pages, **new_array = NULL;
>> -    int nr_pages_left = *nr_pages, i, j;
>> -    int nr_folios = data->nr_folios;
>> +    unsigned nr_pages_left = *nr_pages;
>> +    unsigned nr_folios = data->nr_folios;
>> +    unsigned i, j;
> checkpatch.pl complains about just "unsigned", "unsigned int" is preferred.
> 
>>
>>     /* Store head pages only*/
>> -    new_array = kvmalloc_array(nr_folios, sizeof(struct page *),
>> -                    GFP_KERNEL);
>> +    new_array = kvmalloc_array(nr_folios, sizeof(struct page *), GFP_KERNEL);
> Not sure whether 80 line length is preferred in uring subsystem, if yes
> then this breaks it.

It's fine to use checkpatch for new contributors, but it's generally
just pretty useless imho. io_uring does exceed 80 chars regularly, if it
improves readability. And we also do use unsigned, not sure why
checkpatch is against that.

-- 
Jens Axboe