[PATCH 0/4] mm/gup: page unpining improvements

[PATCH 0/4] mm/gup: page unpining improvements

[Joao Martins]

Hey,

This series improves page unpinning, with an eye on improving MR
deregistration for big swaths of memory (which is bound by the page
unpining), particularly:

 1) Decrement the head page by @ntails and thus reducing a lot the number of
atomic operations per compound page. This is done by comparing individual
tail pages heads, and counting number of consecutive tails on which they 
match heads and based on that update head page refcount. Should have a
visible improvement in all page (un)pinners which use compound pages.

 2) Introducing a new API for unpinning page ranges (to avoid the trick in the
previous item and be based on math), and use that in RDMA ib_mem_release
(used for mr deregistration).

Performance improvements: unpin_user_pages() for hugetlbfs and THP improves ~3x
(through gup_test) and RDMA MR dereg improves ~4.5x with the new API.
See patches 2 and 4 for those.

These patches used to be in this RFC:

https://lore.kernel.org/linux-mm/20201208172901.17384-1-joao.m.martins@oracle.com/,
"[PATCH RFC 0/9] mm, sparse-vmemmap: Introduce compound pagemaps"

But were moved separately at the suggestion of Jason, given it's applicable
to page unpinning in general. Thanks for all the comments in the RFC above.

These patches apply on top of linux-next tag next-20210202.

Suggestions, comments, welcomed as usual.

	Joao

Changelog since the RFC above:

* Introduce a head/ntails iterator and change unpin_*_pages() to use that,
 inspired by folio iterators (Jason)
 Introduce an alternative unpin_user_page_range_dirty_lock() to unpin based
on a consecutive page range without having to walk page arrays (Jason)
* Use unsigned for number of tails (Jason)

Joao Martins (4):
  mm/gup: add compound page list iterator
  mm/gup: decrement head page once for group of subpages
  mm/gup: add a range variant of unpin_user_pages_dirty_lock()
  RDMA/umem: batch page unpin in __ib_mem_release()

 drivers/infiniband/core/umem.c | 12 ++---
 include/linux/mm.h             |  2 +
 mm/gup.c                       | 90 +++++++++++++++++++++++++++-------
 3 files changed, 80 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/4] mm/gup: add compound page list iterator

[Joao Martins]

Add an helper that iterates over head pages in a list of pages. It
essentially counts the tails until the next page to process has a
different head that the current. This is going to be used by
unpin_user_pages() family of functions, to batch the head page refcount
updates once for all passed consecutive tail pages.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 mm/gup.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index d68bcb482b11..4f88dcef39f2 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -215,6 +215,35 @@ void unpin_user_page(struct page *page)
 }
 EXPORT_SYMBOL(unpin_user_page);
 
+static inline unsigned int count_ntails(struct page **pages, unsigned long npages)
+{
+	struct page *head = compound_head(pages[0]);
+	unsigned int ntails;
+
+	for (ntails = 1; ntails < npages; ntails++) {
+		if (compound_head(pages[ntails]) != head)
+			break;
+	}
+
+	return ntails;
+}
+
+static inline void compound_next(unsigned long i, unsigned long npages,
+				 struct page **list, struct page **head,
+				 unsigned int *ntails)
+{
+	if (i >= npages)
+		return;
+
+	*ntails = count_ntails(list + i, npages - i);
+	*head = compound_head(list[i]);
+}
+
+#define for_each_compound_head(i, list, npages, head, ntails) \
+	for (i = 0, compound_next(i, npages, list, &head, &ntails); \
+	     i < npages; i += ntails, \
+	     compound_next(i, npages, list, &head, &ntails))
+
 /**
  * unpin_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages
  * @pages:  array of pages to be maybe marked dirty, and definitely released.
-- 
2.17.1

Re: [PATCH 1/4] mm/gup: add compound page list iterator

[John Hubbard]

On 2/3/21 2:00 PM, Joao Martins wrote:
> Add an helper that iterates over head pages in a list of pages. It
> essentially counts the tails until the next page to process has a
> different head that the current. This is going to be used by
> unpin_user_pages() family of functions, to batch the head page refcount
> updates once for all passed consecutive tail pages.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   mm/gup.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index d68bcb482b11..4f88dcef39f2 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -215,6 +215,35 @@ void unpin_user_page(struct page *page)
>   }
>   EXPORT_SYMBOL(unpin_user_page);
>   
> +static inline unsigned int count_ntails(struct page **pages, unsigned long npages)

Silly naming nit: could we please name this function count_pagetails()? count_ntails
is a bit redundant, plus slightly less clear.

> +{
> +	struct page *head = compound_head(pages[0]);
> +	unsigned int ntails;
> +
> +	for (ntails = 1; ntails < npages; ntails++) {
> +		if (compound_head(pages[ntails]) != head)
> +			break;
> +	}
> +
> +	return ntails;
> +}
> +
> +static inline void compound_next(unsigned long i, unsigned long npages,
> +				 struct page **list, struct page **head,
> +				 unsigned int *ntails)
> +{
> +	if (i >= npages)
> +		return;
> +
> +	*ntails = count_ntails(list + i, npages - i);
> +	*head = compound_head(list[i]);
> +}
> +
> +#define for_each_compound_head(i, list, npages, head, ntails) \

When using macros, which are dangerous in general, you have to worry about
things like name collisions. I really dislike that C has forced this unsafe
pattern upon us, but of course we are stuck with it, for iterator helpers.

Given that we're stuck, you should probably use names such as __i, __list, etc,
in the the above #define. Otherwise you could stomp on existing variables.

> +	for (i = 0, compound_next(i, npages, list, &head, &ntails); \
> +	     i < npages; i += ntails, \
> +	     compound_next(i, npages, list, &head, &ntails))
> +
>   /**
>    * unpin_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages
>    * @pages:  array of pages to be maybe marked dirty, and definitely released.
> 

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/4] mm/gup: add compound page list iterator

[Joao Martins]

On 2/3/21 11:00 PM, John Hubbard wrote:
> On 2/3/21 2:00 PM, Joao Martins wrote:
>> Add an helper that iterates over head pages in a list of pages. It
>> essentially counts the tails until the next page to process has a
>> different head that the current. This is going to be used by
>> unpin_user_pages() family of functions, to batch the head page refcount
>> updates once for all passed consecutive tail pages.
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>   mm/gup.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index d68bcb482b11..4f88dcef39f2 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -215,6 +215,35 @@ void unpin_user_page(struct page *page)
>>   }
>>   EXPORT_SYMBOL(unpin_user_page);
>>   
>> +static inline unsigned int count_ntails(struct page **pages, unsigned long npages)
> 
> Silly naming nit: could we please name this function count_pagetails()? count_ntails
> is a bit redundant, plus slightly less clear.
> 
Hmm, pagetails is also a tiny bit redundant. Perhaps count_subpages() instead?

count_ntails is meant to be 'count number of tails' i.e. to align terminology with head +
tails which was also suggested over the other series.

>> +{
>> +	struct page *head = compound_head(pages[0]);
>> +	unsigned int ntails;
>> +
>> +	for (ntails = 1; ntails < npages; ntails++) {
>> +		if (compound_head(pages[ntails]) != head)
>> +			break;
>> +	}
>> +
>> +	return ntails;
>> +}
>> +
>> +static inline void compound_next(unsigned long i, unsigned long npages,
>> +				 struct page **list, struct page **head,
>> +				 unsigned int *ntails)
>> +{
>> +	if (i >= npages)
>> +		return;
>> +
>> +	*ntails = count_ntails(list + i, npages - i);
>> +	*head = compound_head(list[i]);
>> +}
>> +
>> +#define for_each_compound_head(i, list, npages, head, ntails) \
> 
> When using macros, which are dangerous in general, you have to worry about
> things like name collisions. I really dislike that C has forced this unsafe
> pattern upon us, but of course we are stuck with it, for iterator helpers.
> 
/me nods

> Given that we're stuck, you should probably use names such as __i, __list, etc,
> in the the above #define. Otherwise you could stomp on existing variables.

Will do.

	Joao

Re: [PATCH 1/4] mm/gup: add compound page list iterator

[Joao Martins]

On 2/4/21 11:27 AM, Joao Martins wrote:
> On 2/3/21 11:00 PM, John Hubbard wrote:
>> On 2/3/21 2:00 PM, Joao Martins wrote:
>>> Add an helper that iterates over head pages in a list of pages. It
>>> essentially counts the tails until the next page to process has a
>>> different head that the current. This is going to be used by
>>> unpin_user_pages() family of functions, to batch the head page refcount
>>> updates once for all passed consecutive tail pages.
>>>
>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>>   mm/gup.c | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/mm/gup.c b/mm/gup.c
>>> index d68bcb482b11..4f88dcef39f2 100644
>>> --- a/mm/gup.c
>>> +++ b/mm/gup.c
>>> @@ -215,6 +215,35 @@ void unpin_user_page(struct page *page)
>>>   }
>>>   EXPORT_SYMBOL(unpin_user_page);
>>>   
>>> +static inline unsigned int count_ntails(struct page **pages, unsigned long npages)
>>
>> Silly naming nit: could we please name this function count_pagetails()? count_ntails
>> is a bit redundant, plus slightly less clear.
>>
> Hmm, pagetails is also a tiny bit redundant. Perhaps count_subpages() instead?
> 
> count_ntails is meant to be 'count number of tails' i.e. to align terminology with head +
> tails which was also suggested over the other series.
> 
Given your comment on the third patch, I reworked a bit and got rid of the count_ntails.

So it's looking like this, also the macro arguments renaming as well):

diff --git a/mm/gup.c b/mm/gup.c
index d68bcb482b11..d1549c61c2f6 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -215,6 +215,35 @@ void unpin_user_page(struct page *page)
 }
 EXPORT_SYMBOL(unpin_user_page);

+static inline void compound_next(unsigned long i, unsigned long npages,
+                                struct page **list, struct page **head,
+                                unsigned int *ntails)
+{
+       struct page *page;
+       unsigned int nr;
+
+       if (i >= npages)
+               return;
+
+       list += i;
+       npages -= i;
+       page = compound_head(*list);
+
+       for (nr = 1; nr < npages; nr++) {
+               if (compound_head(list[nr]) != page)
+                       break;
+       }
+
+       *head = page;
+       *ntails = nr;
+}
+
+#define for_each_compound_head(__i, __list, __npages, __head, __ntails) \
+       for (__i = 0, \
+            compound_next(__i, __npages, __list, &(__head), &(__ntails)); \
+            __i < __npages; __i += __ntails, \
+            compound_next(__i, __npages, __list, &(__head), &(__ntails)))
+
 /**
  * unpin_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages
  * @pages:  array of pages to be maybe marked dirty, and definitely released.

Re: [PATCH 1/4] mm/gup: add compound page list iterator

[Jason Gunthorpe]

On Wed, Feb 03, 2021 at 03:00:01PM -0800, John Hubbard wrote:
> > +static inline void compound_next(unsigned long i, unsigned long npages,
> > +				 struct page **list, struct page **head,
> > +				 unsigned int *ntails)
> > +{
> > +	if (i >= npages)
> > +		return;
> > +
> > +	*ntails = count_ntails(list + i, npages - i);
> > +	*head = compound_head(list[i]);
> > +}
> > +
> > +#define for_each_compound_head(i, list, npages, head, ntails) \
> 
> When using macros, which are dangerous in general, you have to worry about
> things like name collisions. I really dislike that C has forced this unsafe
> pattern upon us, but of course we are stuck with it, for iterator helpers.
> 
> Given that we're stuck, you should probably use names such as __i, __list, etc,
> in the the above #define. Otherwise you could stomp on existing variables.

Not this macro, it after cpp gets through with it all the macro names
vanish, it can't collide with variables.

The usual worry is you might collide with other #defines, but we don't
seem to worry about that in the kernel

Jason

Re: [PATCH 1/4] mm/gup: add compound page list iterator

[John Hubbard]

On 2/4/21 11:53 AM, Jason Gunthorpe wrote:
> On Wed, Feb 03, 2021 at 03:00:01PM -0800, John Hubbard wrote:
>>> +static inline void compound_next(unsigned long i, unsigned long npages,
>>> +				 struct page **list, struct page **head,
>>> +				 unsigned int *ntails)
>>> +{
>>> +	if (i >= npages)
>>> +		return;
>>> +
>>> +	*ntails = count_ntails(list + i, npages - i);
>>> +	*head = compound_head(list[i]);
>>> +}
>>> +
>>> +#define for_each_compound_head(i, list, npages, head, ntails) \
>>
>> When using macros, which are dangerous in general, you have to worry about
>> things like name collisions. I really dislike that C has forced this unsafe
>> pattern upon us, but of course we are stuck with it, for iterator helpers.
>>
>> Given that we're stuck, you should probably use names such as __i, __list, etc,
>> in the the above #define. Otherwise you could stomp on existing variables.
> 
> Not this macro, it after cpp gets through with it all the macro names
> vanish, it can't collide with variables.
> 

Yes, I guess it does just vaporize, because it turns all the args into
their substituted values. I was just having flashbacks from similar cases
I guess.

> The usual worry is you might collide with other #defines, but we don't
> seem to worry about that in the kernel
> 

Well, I worry about it a little anyway. haha :)


thanks,
-- 
John Hubbard
NVIDIA

[PATCH 2/4] mm/gup: decrement head page once for group of subpages

[Joao Martins]

Rather than decrementing the head page refcount one by one, we
walk the page array and checking which belong to the same
compound_head. Later on we decrement the calculated amount
of references in a single write to the head page. To that
end switch to for_each_compound_head() does most of the work.

set_page_dirty() needs no adjustment as it's a nop for
non-dirty head pages and it doesn't operate on tail pages.

This considerably improves unpinning of pages with THP and
hugetlbfs:

- THP
gup_test -t -m 16384 -r 10 [-L|-a] -S -n 512 -w
PIN_LONGTERM_BENCHMARK (put values): ~87.6k us -> ~23.2k us

- 16G with 1G huge page size
gup_test -f /mnt/huge/file -m 16384 -r 10 [-L|-a] -S -n 512 -w
PIN_LONGTERM_BENCHMARK: (put values): ~87.6k us -> ~27.5k us

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 mm/gup.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 4f88dcef39f2..971a24b4b73f 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -270,20 +270,15 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
 				 bool make_dirty)
 {
 	unsigned long index;
-
-	/*
-	 * TODO: this can be optimized for huge pages: if a series of pages is
-	 * physically contiguous and part of the same compound page, then a
-	 * single operation to the head page should suffice.
-	 */
+	struct page *head;
+	unsigned int ntails;
 
 	if (!make_dirty) {
 		unpin_user_pages(pages, npages);
 		return;
 	}
 
-	for (index = 0; index < npages; index++) {
-		struct page *page = compound_head(pages[index]);
+	for_each_compound_head(index, pages, npages, head, ntails) {
 		/*
 		 * Checking PageDirty at this point may race with
 		 * clear_page_dirty_for_io(), but that's OK. Two key
@@ -304,9 +299,9 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
 		 * written back, so it gets written back again in the
 		 * next writeback cycle. This is harmless.
 		 */
-		if (!PageDirty(page))
-			set_page_dirty_lock(page);
-		unpin_user_page(page);
+		if (!PageDirty(head))
+			set_page_dirty_lock(head);
+		put_compound_head(head, ntails, FOLL_PIN);
 	}
 }
 EXPORT_SYMBOL(unpin_user_pages_dirty_lock);
@@ -323,6 +318,8 @@ EXPORT_SYMBOL(unpin_user_pages_dirty_lock);
 void unpin_user_pages(struct page **pages, unsigned long npages)
 {
 	unsigned long index;
+	struct page *head;
+	unsigned int ntails;
 
 	/*
 	 * If this WARN_ON() fires, then the system *might* be leaking pages (by
@@ -331,13 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages)
 	 */
 	if (WARN_ON(IS_ERR_VALUE(npages)))
 		return;
-	/*
-	 * TODO: this can be optimized for huge pages: if a series of pages is
-	 * physically contiguous and part of the same compound page, then a
-	 * single operation to the head page should suffice.
-	 */
-	for (index = 0; index < npages; index++)
-		unpin_user_page(pages[index]);
+
+	for_each_compound_head(index, pages, npages, head, ntails)
+		put_compound_head(head, ntails, FOLL_PIN);
 }
 EXPORT_SYMBOL(unpin_user_pages);
 
-- 
2.17.1

Re: [PATCH 2/4] mm/gup: decrement head page once for group of subpages

[John Hubbard]

On 2/3/21 2:00 PM, Joao Martins wrote:
> Rather than decrementing the head page refcount one by one, we
> walk the page array and checking which belong to the same
> compound_head. Later on we decrement the calculated amount
> of references in a single write to the head page. To that
> end switch to for_each_compound_head() does most of the work.
> 
> set_page_dirty() needs no adjustment as it's a nop for
> non-dirty head pages and it doesn't operate on tail pages.
> 
> This considerably improves unpinning of pages with THP and
> hugetlbfs:
> 
> - THP
> gup_test -t -m 16384 -r 10 [-L|-a] -S -n 512 -w
> PIN_LONGTERM_BENCHMARK (put values): ~87.6k us -> ~23.2k us
> 
> - 16G with 1G huge page size
> gup_test -f /mnt/huge/file -m 16384 -r 10 [-L|-a] -S -n 512 -w
> PIN_LONGTERM_BENCHMARK: (put values): ~87.6k us -> ~27.5k us
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   mm/gup.c | 29 +++++++++++------------------
>   1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 4f88dcef39f2..971a24b4b73f 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -270,20 +270,15 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
>   				 bool make_dirty)
>   {
>   	unsigned long index;
> -
> -	/*
> -	 * TODO: this can be optimized for huge pages: if a series of pages is
> -	 * physically contiguous and part of the same compound page, then a
> -	 * single operation to the head page should suffice.
> -	 */

Great to see this TODO (and the related one below) finally done!

Everything looks correct here.

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA

> +	struct page *head;
> +	unsigned int ntails;
>   
>   	if (!make_dirty) {
>   		unpin_user_pages(pages, npages);
>   		return;
>   	}
>   
> -	for (index = 0; index < npages; index++) {
> -		struct page *page = compound_head(pages[index]);
> +	for_each_compound_head(index, pages, npages, head, ntails) {
>   		/*
>   		 * Checking PageDirty at this point may race with
>   		 * clear_page_dirty_for_io(), but that's OK. Two key
> @@ -304,9 +299,9 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
>   		 * written back, so it gets written back again in the
>   		 * next writeback cycle. This is harmless.
>   		 */
> -		if (!PageDirty(page))
> -			set_page_dirty_lock(page);
> -		unpin_user_page(page);
> +		if (!PageDirty(head))
> +			set_page_dirty_lock(head);
> +		put_compound_head(head, ntails, FOLL_PIN);
>   	}
>   }
>   EXPORT_SYMBOL(unpin_user_pages_dirty_lock);
> @@ -323,6 +318,8 @@ EXPORT_SYMBOL(unpin_user_pages_dirty_lock);
>   void unpin_user_pages(struct page **pages, unsigned long npages)
>   {
>   	unsigned long index;
> +	struct page *head;
> +	unsigned int ntails;
>   
>   	/*
>   	 * If this WARN_ON() fires, then the system *might* be leaking pages (by
> @@ -331,13 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages)
>   	 */
>   	if (WARN_ON(IS_ERR_VALUE(npages)))
>   		return;
> -	/*
> -	 * TODO: this can be optimized for huge pages: if a series of pages is
> -	 * physically contiguous and part of the same compound page, then a
> -	 * single operation to the head page should suffice.
> -	 */
> -	for (index = 0; index < npages; index++)
> -		unpin_user_page(pages[index]);
> +
> +	for_each_compound_head(index, pages, npages, head, ntails)
> +		put_compound_head(head, ntails, FOLL_PIN);
>   }
>   EXPORT_SYMBOL(unpin_user_pages);
>   
> 

Re: [PATCH 2/4] mm/gup: decrement head page once for group of subpages

[Joao Martins]

On 2/3/21 11:28 PM, John Hubbard wrote:
> On 2/3/21 2:00 PM, Joao Martins wrote:
>> Rather than decrementing the head page refcount one by one, we
>> walk the page array and checking which belong to the same
>> compound_head. Later on we decrement the calculated amount
>> of references in a single write to the head page. To that
>> end switch to for_each_compound_head() does most of the work.
>>
>> set_page_dirty() needs no adjustment as it's a nop for
>> non-dirty head pages and it doesn't operate on tail pages.
>>
>> This considerably improves unpinning of pages with THP and
>> hugetlbfs:
>>
>> - THP
>> gup_test -t -m 16384 -r 10 [-L|-a] -S -n 512 -w
>> PIN_LONGTERM_BENCHMARK (put values): ~87.6k us -> ~23.2k us
>>
>> - 16G with 1G huge page size
>> gup_test -f /mnt/huge/file -m 16384 -r 10 [-L|-a] -S -n 512 -w
>> PIN_LONGTERM_BENCHMARK: (put values): ~87.6k us -> ~27.5k us
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>   mm/gup.c | 29 +++++++++++------------------
>>   1 file changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 4f88dcef39f2..971a24b4b73f 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -270,20 +270,15 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
>>   				 bool make_dirty)
>>   {
>>   	unsigned long index;
>> -
>> -	/*
>> -	 * TODO: this can be optimized for huge pages: if a series of pages is
>> -	 * physically contiguous and part of the same compound page, then a
>> -	 * single operation to the head page should suffice.
>> -	 */
> 
> Great to see this TODO (and the related one below) finally done!
> 
> Everything looks correct here.
> 
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> 
Thank you!

	Joao

[PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock()

[Joao Martins]

Add a unpin_user_page_range() API which takes a starting page
and how many consecutive pages we want to dirty.

Given that we won't be iterating on a list of changes, change
compound_next() to receive a bool, whether to calculate from the starting
page, or walk the page array. Finally add a separate iterator,
for_each_compound_range() that just operate in page ranges as opposed
to page array.

For users (like RDMA mr_dereg) where each sg represents a
contiguous set of pages, we're able to more efficiently unpin
pages without having to supply an array of pages much of what
happens today with unpin_user_pages().

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 include/linux/mm.h |  2 ++
 mm/gup.c           | 48 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index a608feb0d42e..b76063f7f18a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1265,6 +1265,8 @@ static inline void put_page(struct page *page)
 void unpin_user_page(struct page *page);
 void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
 				 bool make_dirty);
+void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
+				      bool make_dirty);
 void unpin_user_pages(struct page **pages, unsigned long npages);
 
 /**
diff --git a/mm/gup.c b/mm/gup.c
index 971a24b4b73f..1b57355d5033 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -215,11 +215,16 @@ void unpin_user_page(struct page *page)
 }
 EXPORT_SYMBOL(unpin_user_page);
 
-static inline unsigned int count_ntails(struct page **pages, unsigned long npages)
+static inline unsigned int count_ntails(struct page **pages,
+					unsigned long npages, bool range)
 {
-	struct page *head = compound_head(pages[0]);
+	struct page *page = pages[0], *head = compound_head(page);
 	unsigned int ntails;
 
+	if (range)
+		return (!PageCompound(head) || compound_order(head) <= 1) ? 1 :
+		   min_t(unsigned int, (head + compound_nr(head) - page), npages);
+
 	for (ntails = 1; ntails < npages; ntails++) {
 		if (compound_head(pages[ntails]) != head)
 			break;
@@ -229,20 +234,32 @@ static inline unsigned int count_ntails(struct page **pages, unsigned long npage
 }
 
 static inline void compound_next(unsigned long i, unsigned long npages,
-				 struct page **list, struct page **head,
-				 unsigned int *ntails)
+				 struct page **list, bool range,
+				 struct page **head, unsigned int *ntails)
 {
+	struct page *p, **next = &p;
+
 	if (i >= npages)
 		return;
 
-	*ntails = count_ntails(list + i, npages - i);
-	*head = compound_head(list[i]);
+	if (range)
+		*next = *list + i;
+	else
+		next = list + i;
+
+	*ntails = count_ntails(next, npages - i, range);
+	*head = compound_head(*next);
 }
 
+#define for_each_compound_range(i, list, npages, head, ntails) \
+	for (i = 0, compound_next(i, npages, list, true, &head, &ntails); \
+	     i < npages; i += ntails, \
+	     compound_next(i, npages, list, true,  &head, &ntails))
+
 #define for_each_compound_head(i, list, npages, head, ntails) \
-	for (i = 0, compound_next(i, npages, list, &head, &ntails); \
+	for (i = 0, compound_next(i, npages, list, false, &head, &ntails); \
 	     i < npages; i += ntails, \
-	     compound_next(i, npages, list, &head, &ntails))
+	     compound_next(i, npages, list, false,  &head, &ntails))
 
 /**
  * unpin_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages
@@ -306,6 +323,21 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
 }
 EXPORT_SYMBOL(unpin_user_pages_dirty_lock);
 
+void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
+				      bool make_dirty)
+{
+	unsigned long index;
+	struct page *head;
+	unsigned int ntails;
+
+	for_each_compound_range(index, &page, npages, head, ntails) {
+		if (make_dirty && !PageDirty(head))
+			set_page_dirty_lock(head);
+		put_compound_head(head, ntails, FOLL_PIN);
+	}
+}
+EXPORT_SYMBOL(unpin_user_page_range_dirty_lock);
+
 /**
  * unpin_user_pages() - release an array of gup-pinned pages.
  * @pages:  array of pages to be marked dirty and released.
-- 
2.17.1

Re: [PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock()

[John Hubbard]

On 2/3/21 2:00 PM, Joao Martins wrote:
> Add a unpin_user_page_range() API which takes a starting page
> and how many consecutive pages we want to dirty.
> 
> Given that we won't be iterating on a list of changes, change
> compound_next() to receive a bool, whether to calculate from the starting
> page, or walk the page array. Finally add a separate iterator,

A bool arg is sometimes, but not always, a hint that you really just want
a separate set of routines. Below...

> for_each_compound_range() that just operate in page ranges as opposed
> to page array.
> 
> For users (like RDMA mr_dereg) where each sg represents a
> contiguous set of pages, we're able to more efficiently unpin
> pages without having to supply an array of pages much of what
> happens today with unpin_user_pages().
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   include/linux/mm.h |  2 ++
>   mm/gup.c           | 48 ++++++++++++++++++++++++++++++++++++++--------
>   2 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a608feb0d42e..b76063f7f18a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1265,6 +1265,8 @@ static inline void put_page(struct page *page)
>   void unpin_user_page(struct page *page);
>   void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
>   				 bool make_dirty);
> +void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
> +				      bool make_dirty);
>   void unpin_user_pages(struct page **pages, unsigned long npages);
>   
>   /**
> diff --git a/mm/gup.c b/mm/gup.c
> index 971a24b4b73f..1b57355d5033 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -215,11 +215,16 @@ void unpin_user_page(struct page *page)
>   }
>   EXPORT_SYMBOL(unpin_user_page);
>   
> -static inline unsigned int count_ntails(struct page **pages, unsigned long npages)
> +static inline unsigned int count_ntails(struct page **pages,
> +					unsigned long npages, bool range)
>   {
> -	struct page *head = compound_head(pages[0]);
> +	struct page *page = pages[0], *head = compound_head(page);
>   	unsigned int ntails;
>   
> +	if (range)
> +		return (!PageCompound(head) || compound_order(head) <= 1) ? 1 :
> +		   min_t(unsigned int, (head + compound_nr(head) - page), npages);

Here, you clearly should use a separate set of _range routines. Because you're basically
creating two different routines here! Keep it simple.

Once you're in a separate routine, you might feel more comfortable expanding that to
a more readable form, too:

	if (!PageCompound(head) || compound_order(head) <= 1)
		return 1;

	return min_t(unsigned int, (head + compound_nr(head) - page), npages);


thanks,
-- 
John Hubbard
NVIDIA

> +
>   	for (ntails = 1; ntails < npages; ntails++) {
>   		if (compound_head(pages[ntails]) != head)
>   			break;
> @@ -229,20 +234,32 @@ static inline unsigned int count_ntails(struct page **pages, unsigned long npage
>   }
>   
>   static inline void compound_next(unsigned long i, unsigned long npages,
> -				 struct page **list, struct page **head,
> -				 unsigned int *ntails)
> +				 struct page **list, bool range,
> +				 struct page **head, unsigned int *ntails)
>   {
> +	struct page *p, **next = &p;
> +
>   	if (i >= npages)
>   		return;
>   
> -	*ntails = count_ntails(list + i, npages - i);
> -	*head = compound_head(list[i]);
> +	if (range)
> +		*next = *list + i;
> +	else
> +		next = list + i;
> +
> +	*ntails = count_ntails(next, npages - i, range);
> +	*head = compound_head(*next);
>   }
>   
> +#define for_each_compound_range(i, list, npages, head, ntails) \
> +	for (i = 0, compound_next(i, npages, list, true, &head, &ntails); \
> +	     i < npages; i += ntails, \
> +	     compound_next(i, npages, list, true,  &head, &ntails))
> +
>   #define for_each_compound_head(i, list, npages, head, ntails) \
> -	for (i = 0, compound_next(i, npages, list, &head, &ntails); \
> +	for (i = 0, compound_next(i, npages, list, false, &head, &ntails); \
>   	     i < npages; i += ntails, \
> -	     compound_next(i, npages, list, &head, &ntails))
> +	     compound_next(i, npages, list, false,  &head, &ntails))
>   
>   /**
>    * unpin_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages
> @@ -306,6 +323,21 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
>   }
>   EXPORT_SYMBOL(unpin_user_pages_dirty_lock);
>   
> +void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
> +				      bool make_dirty)
> +{
> +	unsigned long index;
> +	struct page *head;
> +	unsigned int ntails;
> +
> +	for_each_compound_range(index, &page, npages, head, ntails) {
> +		if (make_dirty && !PageDirty(head))
> +			set_page_dirty_lock(head);
> +		put_compound_head(head, ntails, FOLL_PIN);
> +	}
> +}
> +EXPORT_SYMBOL(unpin_user_page_range_dirty_lock);
> +
>   /**
>    * unpin_user_pages() - release an array of gup-pinned pages.
>    * @pages:  array of pages to be marked dirty and released.
> 

Re: [PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock()

[Joao Martins]

On 2/3/21 11:37 PM, John Hubbard wrote:
> On 2/3/21 2:00 PM, Joao Martins wrote:
>> Add a unpin_user_page_range() API which takes a starting page
>> and how many consecutive pages we want to dirty.
>>
>> Given that we won't be iterating on a list of changes, change
>> compound_next() to receive a bool, whether to calculate from the starting
>> page, or walk the page array. Finally add a separate iterator,
> 
> A bool arg is sometimes, but not always, a hint that you really just want
> a separate set of routines. Below...
> 
Yes.

I was definitely wrestling back and forth a lot about having separate routines for two
different iterators helpers i.e. compound_next_head()or having it all merged into one
compound_next() / count_ntails().

>> for_each_compound_range() that just operate in page ranges as opposed
>> to page array.
>>
>> For users (like RDMA mr_dereg) where each sg represents a
>> contiguous set of pages, we're able to more efficiently unpin
>> pages without having to supply an array of pages much of what
>> happens today with unpin_user_pages().
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>   include/linux/mm.h |  2 ++
>>   mm/gup.c           | 48 ++++++++++++++++++++++++++++++++++++++--------
>>   2 files changed, 42 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index a608feb0d42e..b76063f7f18a 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1265,6 +1265,8 @@ static inline void put_page(struct page *page)
>>   void unpin_user_page(struct page *page);
>>   void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
>>   				 bool make_dirty);
>> +void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
>> +				      bool make_dirty);
>>   void unpin_user_pages(struct page **pages, unsigned long npages);
>>   
>>   /**
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 971a24b4b73f..1b57355d5033 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -215,11 +215,16 @@ void unpin_user_page(struct page *page)
>>   }
>>   EXPORT_SYMBOL(unpin_user_page);
>>   
>> -static inline unsigned int count_ntails(struct page **pages, unsigned long npages)
>> +static inline unsigned int count_ntails(struct page **pages,
>> +					unsigned long npages, bool range)
>>   {
>> -	struct page *head = compound_head(pages[0]);
>> +	struct page *page = pages[0], *head = compound_head(page);
>>   	unsigned int ntails;
>>   
>> +	if (range)
>> +		return (!PageCompound(head) || compound_order(head) <= 1) ? 1 :
>> +		   min_t(unsigned int, (head + compound_nr(head) - page), npages);
> 
> Here, you clearly should use a separate set of _range routines. Because you're basically
> creating two different routines here! Keep it simple.
> 
> Once you're in a separate routine, you might feel more comfortable expanding that to
> a more readable form, too:
> 
> 	if (!PageCompound(head) || compound_order(head) <= 1)
> 		return 1;
> 
> 	return min_t(unsigned int, (head + compound_nr(head) - page), npages);
> 
Yes.

Let me also try instead to put move everything into two sole iterator helper routines,
compound_next() and compound_next_range(), and thus get rid of this count_ntails(). It
should also help in removing a compound_head() call which should save cycles.

	Joao

Re: [PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock()

[Joao Martins]

On 2/4/21 11:35 AM, Joao Martins wrote:
> On 2/3/21 11:37 PM, John Hubbard wrote:
>> On 2/3/21 2:00 PM, Joao Martins wrote:
>>> -static inline unsigned int count_ntails(struct page **pages, unsigned long npages)
>>> +static inline unsigned int count_ntails(struct page **pages,
>>> +					unsigned long npages, bool range)
>>>   {
>>> -	struct page *head = compound_head(pages[0]);
>>> +	struct page *page = pages[0], *head = compound_head(page);
>>>   	unsigned int ntails;
>>>   
>>> +	if (range)
>>> +		return (!PageCompound(head) || compound_order(head) <= 1) ? 1 :
>>> +		   min_t(unsigned int, (head + compound_nr(head) - page), npages);
>>
>> Here, you clearly should use a separate set of _range routines. Because you're basically
>> creating two different routines here! Keep it simple.
>>
>> Once you're in a separate routine, you might feel more comfortable expanding that to
>> a more readable form, too:
>>
>> 	if (!PageCompound(head) || compound_order(head) <= 1)
>> 		return 1;
>>
>> 	return min_t(unsigned int, (head + compound_nr(head) - page), npages);
>>
> Yes.
> 
> Let me also try instead to put move everything into two sole iterator helper routines,
> compound_next() and compound_next_range(), and thus get rid of this count_ntails(). It
> should also help in removing a compound_head() call which should save cycles.
> 

As mentioned earlier, I got rid of count_ntails and the ugly boolean. Plus addressed the
missing docs -- fwiw, I borrowed unpin_user_pages_dirty_lock() docs and modified a bit.

Partial diff below, hopefully it is looking better now:

diff --git a/mm/gup.c b/mm/gup.c
index 5a3dd235017a..4ef36c8990e3 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -215,6 +215,34 @@ void unpin_user_page(struct page *page)
 }
 EXPORT_SYMBOL(unpin_user_page);

+static inline void range_next(unsigned long i, unsigned long npages,
+                             struct page **list, struct page **head,
+                             unsigned int *ntails)
+{
+       struct page *next, *page;
+       unsigned int nr = 1;
+
+       if (i >= npages)
+               return;
+
+       npages -= i;
+       next = *list + i;
+
+       page = compound_head(next);
+       if (PageCompound(page) && compound_order(page) > 1)
+               nr = min_t(unsigned int,
+                          page + compound_nr(page) - next, npages);
+
+       *head = page;
+       *ntails = nr;
+}
+
+#define for_each_compound_range(__i, __list, __npages, __head, __ntails) \
+       for (__i = 0, \
+            range_next(__i, __npages, __list, &(__head), &(__ntails)); \
+            __i < __npages; __i += __ntails, \
+            range_next(__i, __npages, __list, &(__head), &(__ntails)))
+
 static inline void compound_next(unsigned long i, unsigned long npages,
                                 struct page **list, struct page **head,
                                 unsigned int *ntails)
@@ -306,6 +334,42 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long
npages,
 }
 EXPORT_SYMBOL(unpin_user_pages_dirty_lock);

+/**
+ * unpin_user_page_range_dirty_lock() - release and optionally dirty
+ * gup-pinned page range
+ *
+ * @page:  the starting page of a range maybe marked dirty, and definitely released.
+ * @npages: number of consecutive pages to release.
+ * @make_dirty: whether to mark the pages dirty
+ *
+ * "gup-pinned page range" refers to a range of pages that has had one of the
+ * get_user_pages() variants called on that page.
+ *
+ * For the page ranges defined by [page .. page+npages], make that range (or
+ * its head pages, if a compound page) dirty, if @make_dirty is true, and if the
+ * page range was previously listed as clean.
+ *
+ * set_page_dirty_lock() is used internally. If instead, set_page_dirty() is
+ * required, then the caller should a) verify that this is really correct,
+ * because _lock() is usually required, and b) hand code it:
+ * set_page_dirty_lock(), unpin_user_page().
+ *
+ */
+void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
+                                     bool make_dirty)
+{
+       unsigned long index;
+       struct page *head;
+       unsigned int ntails;
+
+       for_each_compound_range(index, &page, npages, head, ntails) {
+               if (make_dirty && !PageDirty(head))
+                       set_page_dirty_lock(head);
+               put_compound_head(head, ntails, FOLL_PIN);
+       }
+}
+EXPORT_SYMBOL(unpin_user_page_range_dirty_lock);
+

Re: [PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock()

[John Hubbard]

On 2/3/21 2:00 PM, Joao Martins wrote:
...
> +void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
> +				      bool make_dirty)
> +{
> +	unsigned long index;
> +	struct page *head;
> +	unsigned int ntails;
> +
> +	for_each_compound_range(index, &page, npages, head, ntails) {
> +		if (make_dirty && !PageDirty(head))
> +			set_page_dirty_lock(head);
> +		put_compound_head(head, ntails, FOLL_PIN);
> +	}
> +}
> +EXPORT_SYMBOL(unpin_user_page_range_dirty_lock);
> +

Also, looking at this again while trying to verify the sg diffs in patch #4, I noticed
that the name is tricky. Usually a "range" would not have a single struct page* as the
argument. So in this case, perhaps a comment above the function would help, something
approximately like this:

/*
  * The "range" in the function name refers to the fact that the page may be a
  * compound page. If so, then that compound page will be treated as one or more
  * ranges of contiguous tail pages.
  */

...I guess. Or maybe the name is just wrong (a comment block explaining a name is
always a bad sign). Perhaps we should rename it to something like:

	unpin_user_compound_page_dirty_lock()

?

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock()

[Joao Martins]

On 2/4/21 12:11 AM, John Hubbard wrote:
> On 2/3/21 2:00 PM, Joao Martins wrote:
> ...
>> +void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
>> +				      bool make_dirty)
>> +{
>> +	unsigned long index;
>> +	struct page *head;
>> +	unsigned int ntails;
>> +
>> +	for_each_compound_range(index, &page, npages, head, ntails) {
>> +		if (make_dirty && !PageDirty(head))
>> +			set_page_dirty_lock(head);
>> +		put_compound_head(head, ntails, FOLL_PIN);
>> +	}
>> +}
>> +EXPORT_SYMBOL(unpin_user_page_range_dirty_lock);
>> +
> 
> Also, looking at this again while trying to verify the sg diffs in patch #4, I noticed
> that the name is tricky. Usually a "range" would not have a single struct page* as the
> argument. So in this case, perhaps a comment above the function would help, something
> approximately like this:
> 
> /*
>   * The "range" in the function name refers to the fact that the page may be a
>   * compound page. If so, then that compound page will be treated as one or more
>   * ranges of contiguous tail pages.
>   */
> 
> ...I guess. Or maybe the name is just wrong (a comment block explaining a name is
> always a bad sign). 

Naming aside, a comment is probably worth nonetheless to explain what the function does.

> Perhaps we should rename it to something like:
> 
> 	unpin_user_compound_page_dirty_lock()
> 
> ?

The thing though, is that it doesn't *only* unpin a compound page. It unpins a contiguous
range of pages (hence page_range) *and* if these are compound pages it further accelerates
things.

Albeit, your name suggestion then probably hints the caller that you should be passing a
compound page anyways, so your suggestion does have a ring to it.

	Joao

[PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release()

[Joao Martins]

Use the newly added unpin_user_page_range_dirty_lock()
for more quickly unpinning a consecutive range of pages
represented as compound pages. This will also calculate
number of pages to unpin (for the tail pages which matching
head page) and thus batch the refcount update.

Running a test program which calls mr reg/unreg on a 1G in size
and measures cost of both operations together (in a guest using rxe)
with THP and hugetlbfs:

Before:
590 rounds in 5.003 sec: 8480.335 usec / round
6898 rounds in 60.001 sec: 8698.367 usec / round

After:
2631 rounds in 5.001 sec: 1900.618 usec / round
31625 rounds in 60.001 sec: 1897.267 usec / round

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/infiniband/core/umem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
index 2dde99a9ba07..ea4ebb3261d9 100644
--- a/drivers/infiniband/core/umem.c
+++ b/drivers/infiniband/core/umem.c
@@ -47,17 +47,17 @@
 
 static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
 {
-	struct sg_page_iter sg_iter;
-	struct page *page;
+	bool make_dirty = umem->writable && dirty;
+	struct scatterlist *sg;
+	int i;
 
 	if (umem->nmap > 0)
 		ib_dma_unmap_sg(dev, umem->sg_head.sgl, umem->sg_nents,
 				DMA_BIDIRECTIONAL);
 
-	for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
-		page = sg_page_iter_page(&sg_iter);
-		unpin_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
-	}
+	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)
+		unpin_user_page_range_dirty_lock(sg_page(sg),
+			DIV_ROUND_UP(sg->length, PAGE_SIZE), make_dirty);
 
 	sg_free_table(&umem->sg_head);
 }
-- 
2.17.1

Re: [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release()

[John Hubbard]

On 2/3/21 2:00 PM, Joao Martins wrote:
> Use the newly added unpin_user_page_range_dirty_lock()
> for more quickly unpinning a consecutive range of pages
> represented as compound pages. This will also calculate
> number of pages to unpin (for the tail pages which matching
> head page) and thus batch the refcount update.
> 
> Running a test program which calls mr reg/unreg on a 1G in size
> and measures cost of both operations together (in a guest using rxe)
> with THP and hugetlbfs:

In the patch subject line:

    s/__ib_mem_release/__ib_umem_release/

> 
> Before:
> 590 rounds in 5.003 sec: 8480.335 usec / round
> 6898 rounds in 60.001 sec: 8698.367 usec / round
> 
> After:
> 2631 rounds in 5.001 sec: 1900.618 usec / round
> 31625 rounds in 60.001 sec: 1897.267 usec / round
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   drivers/infiniband/core/umem.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> index 2dde99a9ba07..ea4ebb3261d9 100644
> --- a/drivers/infiniband/core/umem.c
> +++ b/drivers/infiniband/core/umem.c
> @@ -47,17 +47,17 @@
>   
>   static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
>   {
> -	struct sg_page_iter sg_iter;
> -	struct page *page;
> +	bool make_dirty = umem->writable && dirty;
> +	struct scatterlist *sg;
> +	int i;

Maybe unsigned int is better, so as to perfectly match the scatterlist.length.

>   
>   	if (umem->nmap > 0)
>   		ib_dma_unmap_sg(dev, umem->sg_head.sgl, umem->sg_nents,
>   				DMA_BIDIRECTIONAL);
>   
> -	for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
> -		page = sg_page_iter_page(&sg_iter);
> -		unpin_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
> -	}
> +	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)

The change from umem->sg_nents to umem->nmap looks OK, although we should get
IB people to verify that there is not some odd bug or reason to leave it as is.

> +		unpin_user_page_range_dirty_lock(sg_page(sg),
> +			DIV_ROUND_UP(sg->length, PAGE_SIZE), make_dirty);

Is it really OK to refer directly to sg->length? The scatterlist library goes
to some effort to avoid having callers directly access the struct member variables.

Actually, the for_each_sg() code and its behavior with sg->length and sg_page(sg)
confuses me because I'm new to it, and I don't quite understand how this works.
Especially with SG_CHAIN. I'm assuming that you've monitored /proc/vmstat for
nr_foll_pin* ?

>   
>   	sg_free_table(&umem->sg_head);
>   }
> 

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release()

[Joao Martins]

On 2/4/21 12:15 AM, John Hubbard wrote:
> On 2/3/21 2:00 PM, Joao Martins wrote:
>> Use the newly added unpin_user_page_range_dirty_lock()
>> for more quickly unpinning a consecutive range of pages
>> represented as compound pages. This will also calculate
>> number of pages to unpin (for the tail pages which matching
>> head page) and thus batch the refcount update.
>>
>> Running a test program which calls mr reg/unreg on a 1G in size
>> and measures cost of both operations together (in a guest using rxe)
>> with THP and hugetlbfs:
> 
> In the patch subject line:
> 
>     s/__ib_mem_release/__ib_umem_release/
> 
Ah, yes.

>>
>> Before:
>> 590 rounds in 5.003 sec: 8480.335 usec / round
>> 6898 rounds in 60.001 sec: 8698.367 usec / round
>>
>> After:
>> 2631 rounds in 5.001 sec: 1900.618 usec / round
>> 31625 rounds in 60.001 sec: 1897.267 usec / round
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>   drivers/infiniband/core/umem.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
>> index 2dde99a9ba07..ea4ebb3261d9 100644
>> --- a/drivers/infiniband/core/umem.c
>> +++ b/drivers/infiniband/core/umem.c
>> @@ -47,17 +47,17 @@
>>   
>>   static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
>>   {
>> -	struct sg_page_iter sg_iter;
>> -	struct page *page;
>> +	bool make_dirty = umem->writable && dirty;
>> +	struct scatterlist *sg;
>> +	int i;
> 
> Maybe unsigned int is better, so as to perfectly match the scatterlist.length.
> 
Will fix.

>>   
>>   	if (umem->nmap > 0)
>>   		ib_dma_unmap_sg(dev, umem->sg_head.sgl, umem->sg_nents,
>>   				DMA_BIDIRECTIONAL);
>>   
>> -	for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
>> -		page = sg_page_iter_page(&sg_iter);
>> -		unpin_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
>> -	}
>> +	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)
> 
> The change from umem->sg_nents to umem->nmap looks OK, although we should get
> IB people to verify that there is not some odd bug or reason to leave it as is.
> 
/me nods

fwiw this was suggested by Jason :) as the way I had done was unnecessarily allocating a
page to unpin pages.

>> +		unpin_user_page_range_dirty_lock(sg_page(sg),
>> +			DIV_ROUND_UP(sg->length, PAGE_SIZE), make_dirty);
> 
> Is it really OK to refer directly to sg->length? The scatterlist library goes
> to some effort to avoid having callers directly access the struct member variables.
> 
> Actually, the for_each_sg() code and its behavior with sg->length and sg_page(sg)
> confuses me because I'm new to it, and I don't quite understand how this works.

So IIUC this can be done given how ib_umem_get allocates scatterlists (i.e. see the call
to __sg_alloc_table_from_pages()). It builds a scatterlist with a segment size in device
DMA max segment size  (e.g. 64K, 2G or etc depending on what the device sets it to) and
after created each scatterlist I am iterating represents a contiguous range of PFNs with a
starting page. And if you keep pinning contiguous amounts of memory, it keeps coalescing
this to the previously allocated sgl.

> Especially with SG_CHAIN. I'm assuming that you've monitored /proc/vmstat for
> nr_foll_pin* ?
> 
Yeap I did. I see no pages left unpinned.

>>   
>>   	sg_free_table(&umem->sg_head);
>>   }
>>
> 
> thanks,
> 

Re: [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release()

[Jason Gunthorpe]

On Wed, Feb 03, 2021 at 04:15:53PM -0800, John Hubbard wrote:
> > diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> > index 2dde99a9ba07..ea4ebb3261d9 100644
> > +++ b/drivers/infiniband/core/umem.c
> > @@ -47,17 +47,17 @@
> >   static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
> >   {
> > -	struct sg_page_iter sg_iter;
> > -	struct page *page;
> > +	bool make_dirty = umem->writable && dirty;
> > +	struct scatterlist *sg;
> > +	int i;
> 
> Maybe unsigned int is better, so as to perfectly match the scatterlist.length.

Yes please

> >   	if (umem->nmap > 0)
> >   		ib_dma_unmap_sg(dev, umem->sg_head.sgl, umem->sg_nents,
> >   				DMA_BIDIRECTIONAL);
> > -	for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
> > -		page = sg_page_iter_page(&sg_iter);
> > -		unpin_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
> > -	}
> > +	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)
> 
> The change from umem->sg_nents to umem->nmap looks OK, although we should get
> IB people to verify that there is not some odd bug or reason to leave it as is.

No, nmap wouldn't be right here. nmap is the number of dma mapped SGLs
in the list and should only be used by things doing sg_dma* stuff.

umem->sg_nents is the number of CPU SGL entries and is the correct
thing here.

> > +		unpin_user_page_range_dirty_lock(sg_page(sg),
> > +			DIV_ROUND_UP(sg->length, PAGE_SIZE), make_dirty);
> 
> Is it really OK to refer directly to sg->length? The scatterlist library goes
> to some effort to avoid having callers directly access the struct member variables.

Yes, only the dma length has acessors

Jason

Re: [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release()

[Joao Martins]

On 2/4/21 8:00 PM, Jason Gunthorpe wrote:
> On Wed, Feb 03, 2021 at 04:15:53PM -0800, John Hubbard wrote:
>>> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
>>> index 2dde99a9ba07..ea4ebb3261d9 100644
>>> +++ b/drivers/infiniband/core/umem.c
>>> @@ -47,17 +47,17 @@
>>>   static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
>>>   {
>>> -	struct sg_page_iter sg_iter;
>>> -	struct page *page;
>>> +	bool make_dirty = umem->writable && dirty;
>>> +	struct scatterlist *sg;
>>> +	int i;
>>
>> Maybe unsigned int is better, so as to perfectly match the scatterlist.length.
> 
> Yes please
> 
Fixed in v2.

>>>   	if (umem->nmap > 0)
>>>   		ib_dma_unmap_sg(dev, umem->sg_head.sgl, umem->sg_nents,
>>>   				DMA_BIDIRECTIONAL);
>>> -	for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
>>> -		page = sg_page_iter_page(&sg_iter);
>>> -		unpin_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
>>> -	}
>>> +	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)
>>
>> The change from umem->sg_nents to umem->nmap looks OK, although we should get
>> IB people to verify that there is not some odd bug or reason to leave it as is.
> 
> No, nmap wouldn't be right here. nmap is the number of dma mapped SGLs
> in the list and should only be used by things doing sg_dma* stuff.
> 
> umem->sg_nents is the number of CPU SGL entries and is the correct
> thing here.
> 

And this was fixed in v2 as well.