[PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[lizhe.67]

From: Li Zhe <lizhe.67@bytedance.com>

In the current implementation of the longterm pin_user_pages() function,
we invoke the collect_longterm_unpinnable_folios() function. This function
iterates through the list to check whether each folio belongs to the
"longterm_unpinnabled" category. The folios in this list essentially
correspond to a contiguous region of user-space addresses, with each folio
representing a physical address in increments of PAGESIZE. If this
user-space address range is mapped with large folio, we can optimize the
performance of function collect_longterm_unpinnable_folios() by reducing
the using of READ_ONCE() invoked in
pofs_get_folio()->page_folio()->_compound_head(). Also, we can simplify
the logic of collect_longterm_unpinnable_folios(). Instead of comparing
with prev_folio after calling pofs_get_folio(), we can check whether the
next page is within the same folio.

The performance test results, based on v6.15, obtained through the
gup_test tool from the kernel source tree are as follows. We achieve an
improvement of over 66% for large folio with pagesize=2M. For small folio,
we have only observed a very slight degradation in performance.

Without this patch:

    [root@localhost ~] ./gup_test -HL -m 8192 -n 512
    TAP version 13
    1..1
    # PIN_LONGTERM_BENCHMARK: Time: get:14391 put:10858 us#
    ok 1 ioctl status 0
    # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
    [root@localhost ~]# ./gup_test -LT -m 8192 -n 512
    TAP version 13
    1..1
    # PIN_LONGTERM_BENCHMARK: Time: get:130538 put:31676 us#
    ok 1 ioctl status 0
    # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0

With this patch:

    [root@localhost ~] ./gup_test -HL -m 8192 -n 512
    TAP version 13
    1..1
    # PIN_LONGTERM_BENCHMARK: Time: get:4867 put:10516 us#
    ok 1 ioctl status 0
    # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
    [root@localhost ~]# ./gup_test -LT -m 8192 -n 512
    TAP version 13
    1..1
    # PIN_LONGTERM_BENCHMARK: Time: get:131798 put:31328 us#
    ok 1 ioctl status 0
    # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0

Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
---
Changelogs:

v3->v4:
- Fix some issues of code formatting.

v2->v3:
- Update performance test data based on v6.15.
- Refine the description of the optimization approach in commit message.
- Fix some issues of code formatting.
- Fine-tune the conditions for entering the optimization path.

v1->v2:
- Modify some unreliable code.
- Update performance test data.

v3 patch: https://lore.kernel.org/all/20250605033430.83142-1-lizhe.67@bytedance.com/
v2 patch: https://lore.kernel.org/all/20250604031536.9053-1-lizhe.67@bytedance.com/
v1 patch: https://lore.kernel.org/all/20250530092351.32709-1-lizhe.67@bytedance.com/

 mm/gup.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 84461d384ae2..be968640b935 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2317,6 +2317,31 @@ static void pofs_unpin(struct pages_or_folios *pofs)
 		unpin_user_pages(pofs->pages, pofs->nr_entries);
 }
 
+static struct folio *pofs_next_folio(struct folio *folio,
+		struct pages_or_folios *pofs, long *index_ptr)
+{
+	long i = *index_ptr + 1;
+
+	if (!pofs->has_folios && folio_test_large(folio)) {
+		const unsigned long start_pfn = folio_pfn(folio);
+		const unsigned long end_pfn = start_pfn + folio_nr_pages(folio);
+
+		for (; i < pofs->nr_entries; i++) {
+			unsigned long pfn = page_to_pfn(pofs->pages[i]);
+
+			/* Is this page part of this folio? */
+			if (pfn < start_pfn || pfn >= end_pfn)
+				break;
+		}
+	}
+
+	if (unlikely(i == pofs->nr_entries))
+		return NULL;
+	*index_ptr = i;
+
+	return pofs_get_folio(pofs, i);
+}
+
 /*
  * Returns the number of collected folios. Return value is always >= 0.
  */
@@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
 		struct list_head *movable_folio_list,
 		struct pages_or_folios *pofs)
 {
-	struct folio *prev_folio = NULL;
 	bool drain_allow = true;
-	unsigned long i;
-
-	for (i = 0; i < pofs->nr_entries; i++) {
-		struct folio *folio = pofs_get_folio(pofs, i);
+	struct folio *folio;
+	long i = 0;
 
-		if (folio == prev_folio)
-			continue;
-		prev_folio = folio;
+	for (folio = pofs_get_folio(pofs, i); folio;
+		 folio = pofs_next_folio(folio, pofs, &i)) {
 
 		if (folio_is_longterm_pinnable(folio))
 			continue;
-- 
2.20.1

Re: [PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[David Hildenbrand]

>    * Returns the number of collected folios. Return value is always >= 0.
>    */
> @@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
>   		struct list_head *movable_folio_list,
>   		struct pages_or_folios *pofs)
>   {
> -	struct folio *prev_folio = NULL;
>   	bool drain_allow = true;
> -	unsigned long i;
> -
> -	for (i = 0; i < pofs->nr_entries; i++) {
> -		struct folio *folio = pofs_get_folio(pofs, i);
> +	struct folio *folio;
> +	long i = 0;
>   
> -		if (folio == prev_folio)
> -			continue;
> -		prev_folio = folio;
> +	for (folio = pofs_get_folio(pofs, i); folio;
> +		 folio = pofs_next_folio(folio, pofs, &i)) {

Nit: indentation is still off?

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[lizhe.67]

On Fri, 6 Jun 2025 10:37:42 +0800, david@redhat.com wrote:

> >    * Returns the number of collected folios. Return value is always >= 0.
> >    */
> > @@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
> >   		struct list_head *movable_folio_list,
> >   		struct pages_or_folios *pofs)
> >   {
> > -	struct folio *prev_folio = NULL;
> >   	bool drain_allow = true;
> > -	unsigned long i;
> > -
> > -	for (i = 0; i < pofs->nr_entries; i++) {
> > -		struct folio *folio = pofs_get_folio(pofs, i);
> > +	struct folio *folio;
> > +	long i = 0;
> >   
> > -		if (folio == prev_folio)
> > -			continue;
> > -		prev_folio = folio;
> > +	for (folio = pofs_get_folio(pofs, i); folio;
> > +		 folio = pofs_next_folio(folio, pofs, &i)) {
> 
> Nit: indentation is still off?

In my editor (vim with ts=4), after applying this patch, the folio on
this line would be positioned directly below the folio on the previous
line.

The discrepancy here might be due to issues with web display or email
rendering?

> Acked-by: David Hildenbrand <david@redhat.com>

Thank you very much for your review!

Thanks,
Zhe

Re: [PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[David Hildenbrand]

On 06.06.25 09:37, lizhe.67@bytedance.com wrote:
> On Fri, 6 Jun 2025 10:37:42 +0800, david@redhat.com wrote:
> 
>>>     * Returns the number of collected folios. Return value is always >= 0.
>>>     */
>>> @@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
>>>    		struct list_head *movable_folio_list,
>>>    		struct pages_or_folios *pofs)
>>>    {
>>> -	struct folio *prev_folio = NULL;
>>>    	bool drain_allow = true;
>>> -	unsigned long i;
>>> -
>>> -	for (i = 0; i < pofs->nr_entries; i++) {
>>> -		struct folio *folio = pofs_get_folio(pofs, i);
>>> +	struct folio *folio;
>>> +	long i = 0;
>>>    
>>> -		if (folio == prev_folio)
>>> -			continue;
>>> -		prev_folio = folio;
>>> +	for (folio = pofs_get_folio(pofs, i); folio;
>>> +		 folio = pofs_next_folio(folio, pofs, &i)) {
>>
>> Nit: indentation is still off?
> 
> In my editor (vim with ts=4), after applying this patch, the folio on
> this line would be positioned directly below the folio on the previous
> line.

Documentation/process/coding-style.rst

"Tabs are 8 characters"

:)

Good choice on using vim. This is what I have in my .vimrc regarding tabs

set tabstop=8
set shiftwidth=8
set noexpandtab

set smartindent
set cindent

-- 
Cheers,

David / dhildenb

Re: [PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[lizhe.67]

On Fri, 6 Jun 2025 09:58:45 +0200, david@redhat.com wrote:

> On 06.06.25 09:37, lizhe.67@bytedance.com wrote:
> > On Fri, 6 Jun 2025 10:37:42 +0800, david@redhat.com wrote:
> > 
> >>>     * Returns the number of collected folios. Return value is always >= 0.
> >>>     */
> >>> @@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
> >>>    		struct list_head *movable_folio_list,
> >>>    		struct pages_or_folios *pofs)
> >>>    {
> >>> -	struct folio *prev_folio = NULL;
> >>>    	bool drain_allow = true;
> >>> -	unsigned long i;
> >>> -
> >>> -	for (i = 0; i < pofs->nr_entries; i++) {
> >>> -		struct folio *folio = pofs_get_folio(pofs, i);
> >>> +	struct folio *folio;
> >>> +	long i = 0;
> >>>    
> >>> -		if (folio == prev_folio)
> >>> -			continue;
> >>> -		prev_folio = folio;
> >>> +	for (folio = pofs_get_folio(pofs, i); folio;
> >>> +		 folio = pofs_next_folio(folio, pofs, &i)) {
> >>
> >> Nit: indentation is still off?
> > 
> > In my editor (vim with ts=4), after applying this patch, the folio on
> > this line would be positioned directly below the folio on the previous
> > line.
> 
> Documentation/process/coding-style.rst
> 
> "Tabs are 8 characters"
> 
> :)
> 
> Good choice on using vim. This is what I have in my .vimrc regarding tabs
> 
> set tabstop=8
> set shiftwidth=8
> set noexpandtab
> 
> set smartindent
> set cindent

I truly appreciate your correction and guidance. I sincerely apologize
for the formatting issue that I've caused.

I noticed that Andrew has already integrated this patch into the mm-new
branch. I'm just wondering if there's still a need for me to send out a
v5 patch. I'm happy to do whatever is necessary to ensure everything is
in order.

Thanks,
Zhe

Re: [PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[David Hildenbrand]

On 06.06.25 10:27, lizhe.67@bytedance.com wrote:
> On Fri, 6 Jun 2025 09:58:45 +0200, david@redhat.com wrote:
> 
>> On 06.06.25 09:37, lizhe.67@bytedance.com wrote:
>>> On Fri, 6 Jun 2025 10:37:42 +0800, david@redhat.com wrote:
>>>
>>>>>      * Returns the number of collected folios. Return value is always >= 0.
>>>>>      */
>>>>> @@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
>>>>>     		struct list_head *movable_folio_list,
>>>>>     		struct pages_or_folios *pofs)
>>>>>     {
>>>>> -	struct folio *prev_folio = NULL;
>>>>>     	bool drain_allow = true;
>>>>> -	unsigned long i;
>>>>> -
>>>>> -	for (i = 0; i < pofs->nr_entries; i++) {
>>>>> -		struct folio *folio = pofs_get_folio(pofs, i);
>>>>> +	struct folio *folio;
>>>>> +	long i = 0;
>>>>>     
>>>>> -		if (folio == prev_folio)
>>>>> -			continue;
>>>>> -		prev_folio = folio;
>>>>> +	for (folio = pofs_get_folio(pofs, i); folio;
>>>>> +		 folio = pofs_next_folio(folio, pofs, &i)) {
>>>>
>>>> Nit: indentation is still off?
>>>
>>> In my editor (vim with ts=4), after applying this patch, the folio on
>>> this line would be positioned directly below the folio on the previous
>>> line.
>>
>> Documentation/process/coding-style.rst
>>
>> "Tabs are 8 characters"
>>
>> :)
>>
>> Good choice on using vim. This is what I have in my .vimrc regarding tabs
>>
>> set tabstop=8
>> set shiftwidth=8
>> set noexpandtab
>>
>> set smartindent
>> set cindent
> 
> I truly appreciate your correction and guidance. I sincerely apologize
> for the formatting issue that I've caused.
> 
> I noticed that Andrew has already integrated this patch into the mm-new
> branch.

mm-new is for new stuff, unless it's in mm-unstable -> mm-stable, it's 
still considered rather "experimental".

> I'm just wondering if there's still a need for me to send out a
> v5 patch. I'm happy to do whatever is necessary to ensure everything is
> in order.

Feel free to just send a simple fixup as reply to this patch.

-- 
Cheers,

David / dhildenb

Re: [PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[lizhe.67]

On Fri, 6 Jun 2025 10:50:34 +0200, david@redhat.com wrote:

> On 06.06.25 10:27, lizhe.67@bytedance.com wrote:
> > On Fri, 6 Jun 2025 09:58:45 +0200, david@redhat.com wrote:
> > 
> >> On 06.06.25 09:37, lizhe.67@bytedance.com wrote:
> >>> On Fri, 6 Jun 2025 10:37:42 +0800, david@redhat.com wrote:
> >>>
> >>>>>      * Returns the number of collected folios. Return value is always >= 0.
> >>>>>      */
> >>>>> @@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
> >>>>>     		struct list_head *movable_folio_list,
> >>>>>     		struct pages_or_folios *pofs)
> >>>>>     {
> >>>>> -	struct folio *prev_folio = NULL;
> >>>>>     	bool drain_allow = true;
> >>>>> -	unsigned long i;
> >>>>> -
> >>>>> -	for (i = 0; i < pofs->nr_entries; i++) {
> >>>>> -		struct folio *folio = pofs_get_folio(pofs, i);
> >>>>> +	struct folio *folio;
> >>>>> +	long i = 0;
> >>>>>     
> >>>>> -		if (folio == prev_folio)
> >>>>> -			continue;
> >>>>> -		prev_folio = folio;
> >>>>> +	for (folio = pofs_get_folio(pofs, i); folio;
> >>>>> +		 folio = pofs_next_folio(folio, pofs, &i)) {
> >>>>
> >>>> Nit: indentation is still off?
> >>>
> >>> In my editor (vim with ts=4), after applying this patch, the folio on
> >>> this line would be positioned directly below the folio on the previous
> >>> line.
> >>
> >> Documentation/process/coding-style.rst
> >>
> >> "Tabs are 8 characters"
> >>
> >> :)
> >>
> >> Good choice on using vim. This is what I have in my .vimrc regarding tabs
> >>
> >> set tabstop=8
> >> set shiftwidth=8
> >> set noexpandtab
> >>
> >> set smartindent
> >> set cindent
> > 
> > I truly appreciate your correction and guidance. I sincerely apologize
> > for the formatting issue that I've caused.
> > 
> > I noticed that Andrew has already integrated this patch into the mm-new
> > branch.
> 
> mm-new is for new stuff, unless it's in mm-unstable -> mm-stable, it's 
> still considered rather "experimental".
> 
> > I'm just wondering if there's still a need for me to send out a
> > v5 patch. I'm happy to do whatever is necessary to ensure everything is
> > in order.
> 
> Feel free to just send a simple fixup as reply to this patch.

Thank you. I'll send a fixup reply right away.

Thanks,
Zhe

Re: [PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[lizhe.67]

On Fri, 6 Jun 2025 10:37:42 +0800, lizhe.67@bytedance.com wrote:

> In the current implementation of the longterm pin_user_pages() function,
> we invoke the collect_longterm_unpinnable_folios() function. This function
> iterates through the list to check whether each folio belongs to the
> "longterm_unpinnabled" category. The folios in this list essentially
> correspond to a contiguous region of user-space addresses, with each folio
> representing a physical address in increments of PAGESIZE. If this
> user-space address range is mapped with large folio, we can optimize the
> performance of function collect_longterm_unpinnable_folios() by reducing
> the using of READ_ONCE() invoked in
> pofs_get_folio()->page_folio()->_compound_head(). Also, we can simplify
> the logic of collect_longterm_unpinnable_folios(). Instead of comparing
> with prev_folio after calling pofs_get_folio(), we can check whether the
> next page is within the same folio.
> 
> The performance test results, based on v6.15, obtained through the
> gup_test tool from the kernel source tree are as follows. We achieve an
> improvement of over 66% for large folio with pagesize=2M. For small folio,
> we have only observed a very slight degradation in performance.
> 
> Without this patch:
> 
>     [root@localhost ~] ./gup_test -HL -m 8192 -n 512
>     TAP version 13
>     1..1
>     # PIN_LONGTERM_BENCHMARK: Time: get:14391 put:10858 us#
>     ok 1 ioctl status 0
>     # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
>     [root@localhost ~]# ./gup_test -LT -m 8192 -n 512
>     TAP version 13
>     1..1
>     # PIN_LONGTERM_BENCHMARK: Time: get:130538 put:31676 us#
>     ok 1 ioctl status 0
>     # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> With this patch:
> 
>     [root@localhost ~] ./gup_test -HL -m 8192 -n 512
>     TAP version 13
>     1..1
>     # PIN_LONGTERM_BENCHMARK: Time: get:4867 put:10516 us#
>     ok 1 ioctl status 0
>     # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
>     [root@localhost ~]# ./gup_test -LT -m 8192 -n 512
>     TAP version 13
>     1..1
>     # PIN_LONGTERM_BENCHMARK: Time: get:131798 put:31328 us#
>     ok 1 ioctl status 0
>     # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
> ---
> Changelogs:
> 
> v3->v4:
> - Fix some issues of code formatting.
> 
> v2->v3:
> - Update performance test data based on v6.15.
> - Refine the description of the optimization approach in commit message.
> - Fix some issues of code formatting.
> - Fine-tune the conditions for entering the optimization path.
> 
> v1->v2:
> - Modify some unreliable code.
> - Update performance test data.
> 
> v3 patch: https://lore.kernel.org/all/20250605033430.83142-1-lizhe.67@bytedance.com/
> v2 patch: https://lore.kernel.org/all/20250604031536.9053-1-lizhe.67@bytedance.com/
> v1 patch: https://lore.kernel.org/all/20250530092351.32709-1-lizhe.67@bytedance.com/
> 
>  mm/gup.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 84461d384ae2..be968640b935 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2317,6 +2317,31 @@ static void pofs_unpin(struct pages_or_folios *pofs)
>  		unpin_user_pages(pofs->pages, pofs->nr_entries);
>  }
>  
> +static struct folio *pofs_next_folio(struct folio *folio,
> +		struct pages_or_folios *pofs, long *index_ptr)
> +{
> +	long i = *index_ptr + 1;
> +
> +	if (!pofs->has_folios && folio_test_large(folio)) {
> +		const unsigned long start_pfn = folio_pfn(folio);
> +		const unsigned long end_pfn = start_pfn + folio_nr_pages(folio);
> +
> +		for (; i < pofs->nr_entries; i++) {
> +			unsigned long pfn = page_to_pfn(pofs->pages[i]);
> +
> +			/* Is this page part of this folio? */
> +			if (pfn < start_pfn || pfn >= end_pfn)
> +				break;
> +		}
> +	}
> +
> +	if (unlikely(i == pofs->nr_entries))
> +		return NULL;
> +	*index_ptr = i;
> +
> +	return pofs_get_folio(pofs, i);
> +}
> +
>  /*
>   * Returns the number of collected folios. Return value is always >= 0.
>   */
> @@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
>  		struct list_head *movable_folio_list,
>  		struct pages_or_folios *pofs)
>  {
> -	struct folio *prev_folio = NULL;
>  	bool drain_allow = true;
> -	unsigned long i;
> -
> -	for (i = 0; i < pofs->nr_entries; i++) {
> -		struct folio *folio = pofs_get_folio(pofs, i);
> +	struct folio *folio;
> +	long i = 0;
>  
> -		if (folio == prev_folio)
> -			continue;
> -		prev_folio = folio;
> +	for (folio = pofs_get_folio(pofs, i); folio;
> +		 folio = pofs_next_folio(folio, pofs, &i)) {
>  
>  		if (folio_is_longterm_pinnable(folio))
>  			continue;

Hi Andrew,

I apologize for the inconvenience I've caused. It seems that there are
still one formatting issue with the patch (thanks to David for pointing
it out). We need to apply the following fixup.

Thank you for your time and patience!

diff --git a/mm/gup.c b/mm/gup.c
index be968640b935..85112c904a4d 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2354,7 +2354,7 @@ static void collect_longterm_unpinnable_folios(
 	long i = 0;
 
 	for (folio = pofs_get_folio(pofs, i); folio;
-		 folio = pofs_next_folio(folio, pofs, &i)) {
+	     folio = pofs_next_folio(folio, pofs, &i)) {
 
 		if (folio_is_longterm_pinnable(folio))
 			continue;

Thanks,
Zhe

Re: [PATCH v4] gup: optimize longterm pin_user_pages() for large folio

[David Laight]

On Fri, 6 Jun 2025 09:21:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> >    * Returns the number of collected folios. Return value is always >= 0.
> >    */
> > @@ -2324,16 +2349,12 @@ static void collect_longterm_unpinnable_folios(
> >   		struct list_head *movable_folio_list,
> >   		struct pages_or_folios *pofs)
> >   {
> > -	struct folio *prev_folio = NULL;
> >   	bool drain_allow = true;
> > -	unsigned long i;
> > -
> > -	for (i = 0; i < pofs->nr_entries; i++) {
> > -		struct folio *folio = pofs_get_folio(pofs, i);
> > +	struct folio *folio;
> > +	long i = 0;
> >   
> > -		if (folio == prev_folio)
> > -			continue;
> > -		prev_folio = folio;
> > +	for (folio = pofs_get_folio(pofs, i); folio;
> > +		 folio = pofs_next_folio(folio, pofs, &i)) {  
> 
> Nit: indentation is still off?

I tend to move the 'initialisation' to the line above:
	folio = pofs_get_folio(pofs, i);
	for (; folio; folio = pofs_next_folio(folio, pofs, &i)) {
		code...
For 'search' loops you don't always want the conditional, so:
	folio = pofs_get_folio(pofs, i);
	for (;; folio = pofs_next_folio(folio, pofs, &i)) {
		if (!folio)
			return -ENOENT;
		code...

The 'really useful (tm)' part of a 'for' loop is the statement
executed by 'continue'.

:-)

	David