[PATCH 0/3] A few gup refactorings and documentation updates

[PATCH 0/3] A few gup refactorings and documentation updates

[John Hubbard]

While reviewing some of the other things going on around gup.c, I
noticed that the documentation was wrong for a few of the routines that
I wrote. And then I noticed that there was some significant code
duplication too. So this fixes those issues.

This is not entirely risk-free, but after looking closely at this, I
think it's actually a useful improvement, getting rid of the code
duplication here. The try_get_page() in particular seems better now,
even if there are a few more "if" branches in there.

However, it is possible I've overlooked something. I did some local LTP
and other testing on an x86 test machine but failed to find any problems
yet.

Also, this is based on today's linux-next (next-20210806), in order to
stay reasonably close to mmotm. So I know that this short series doesn't
conflict with the folio-so-far patches in linux-next, anyway.


John Hubbard (3):
  mm/gup: documentation corrections for gup/pup
  mm/gup: small refactoring: simplify try_grab_page()
  mm/gup: refactor and simplify try_get_page()

 include/linux/mm.h | 11 ++++++-----
 mm/gup.c           | 49 ++++++++++++++--------------------------------
 2 files changed, 21 insertions(+), 39 deletions(-)

-- 
2.32.0

[PATCH 1/3] mm/gup: documentation corrections for gup/pup

[John Hubbard]

The documentation for try_grab_compound_head() and try_grab_page() has
fallen a little out of date. Update and clarify a few points.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/gup.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 77150624f77a..5cb18b62921c 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -103,8 +103,14 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)
  * same time. (That's true throughout the get_user_pages*() and
  * pin_user_pages*() APIs.) Cases:
  *
- *    FOLL_GET: page's refcount will be incremented by 1.
- *    FOLL_PIN: page's refcount will be incremented by GUP_PIN_COUNTING_BIAS.
+ *    FOLL_GET: page's refcount will be incremented by refs.
+ *
+ *    FOLL_PIN on compound pages that are > two pages long: page's refcount will
+ *        be incremented by refs, and page[2].hpage_pinned_refcount will be
+ *        incremented by refs * GUP_PIN_COUNTING_BIAS.
+ *
+ *    FOLL_PIN on normal pages, or compound pages that are two pages long:
+ *        page's refcount will be incremented by refs * GUP_PIN_COUNTING_BIAS.
  *
  * Return: head page (with refcount appropriately incremented) for success, or
  * NULL upon failure. If neither FOLL_GET nor FOLL_PIN was set, that's
@@ -143,6 +149,8 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
 		 *
 		 * However, be sure to *also* increment the normal page refcount
 		 * field at least once, so that the page really is pinned.
+		 * That's why the refcount from the earlier
+		 * try_get_compound_head() is left intact.
 		 */
 		if (hpage_pincount_available(page))
 			hpage_pincount_add(page, refs);
@@ -186,10 +194,8 @@ static void put_compound_head(struct page *page, int refs, unsigned int flags)
  * @flags:   gup flags: these are the FOLL_* flag values.
  *
  * Either FOLL_PIN or FOLL_GET (or neither) may be set, but not both at the same
- * time. Cases:
- *
- *    FOLL_GET: page's refcount will be incremented by 1.
- *    FOLL_PIN: page's refcount will be incremented by GUP_PIN_COUNTING_BIAS.
+ * time. Cases: please see the try_grab_compound_head() documentation, with
+ * "refs=1".
  *
  * Return: true for success, or if no action was required (if neither FOLL_PIN
  * nor FOLL_GET was set, nothing is done). False for failure: FOLL_GET or
-- 
2.32.0

Re: [PATCH 1/3] mm/gup: documentation corrections for gup/pup

[Matthew Wilcox]

On Sun, Aug 08, 2021 at 04:50:16PM -0700, John Hubbard wrote:
> @@ -103,8 +103,14 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)
>   * same time. (That's true throughout the get_user_pages*() and
>   * pin_user_pages*() APIs.) Cases:
>   *
> - *    FOLL_GET: page's refcount will be incremented by 1.
> - *    FOLL_PIN: page's refcount will be incremented by GUP_PIN_COUNTING_BIAS.
> + *    FOLL_GET: page's refcount will be incremented by refs.

I think this would read more clearly if it said @refs (throughout).

> + *
> + *    FOLL_PIN on compound pages that are > two pages long: page's refcount will
> + *        be incremented by refs, and page[2].hpage_pinned_refcount will be
> + *        incremented by refs * GUP_PIN_COUNTING_BIAS.
> + *
> + *    FOLL_PIN on normal pages, or compound pages that are two pages long:
> + *        page's refcount will be incremented by refs * GUP_PIN_COUNTING_BIAS.
>   *
>   * Return: head page (with refcount appropriately incremented) for success, or
>   * NULL upon failure. If neither FOLL_GET nor FOLL_PIN was set, that's

Did you run 'make htmldocs' and see how it renders?  I haven't looked,
but this might work better as an rst list?

Re: [PATCH 1/3] mm/gup: documentation corrections for gup/pup

[John Hubbard]

On 8/8/21 6:39 PM, Matthew Wilcox wrote:
> On Sun, Aug 08, 2021 at 04:50:16PM -0700, John Hubbard wrote:
>> @@ -103,8 +103,14 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)
>>    * same time. (That's true throughout the get_user_pages*() and
>>    * pin_user_pages*() APIs.) Cases:
>>    *
>> - *    FOLL_GET: page's refcount will be incremented by 1.
>> - *    FOLL_PIN: page's refcount will be incremented by GUP_PIN_COUNTING_BIAS.
>> + *    FOLL_GET: page's refcount will be incremented by refs.
> 
> I think this would read more clearly if it said @refs (throughout).

OK, will change that for v2.

> 
>> + *
>> + *    FOLL_PIN on compound pages that are > two pages long: page's refcount will
>> + *        be incremented by refs, and page[2].hpage_pinned_refcount will be
>> + *        incremented by refs * GUP_PIN_COUNTING_BIAS.
>> + *
>> + *    FOLL_PIN on normal pages, or compound pages that are two pages long:
>> + *        page's refcount will be incremented by refs * GUP_PIN_COUNTING_BIAS.
>>    *
>>    * Return: head page (with refcount appropriately incremented) for success, or
>>    * NULL upon failure. If neither FOLL_GET nor FOLL_PIN was set, that's
> 
> Did you run 'make htmldocs' and see how it renders?  I haven't looked,
> but this might work better as an rst list?
> 

Hadn't occurred to me, due to my own incorrect mental separation between
comment kernel docs, and rst formatting ("rst == Documentation/"). I'll give it a try.

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/3] mm/gup: documentation corrections for gup/pup

[John Hubbard]

On 8/8/21 6:39 PM, Matthew Wilcox wrote:
...
>> + *    FOLL_PIN on compound pages that are > two pages long: page's refcount will
>> + *        be incremented by refs, and page[2].hpage_pinned_refcount will be
>> + *        incremented by refs * GUP_PIN_COUNTING_BIAS.
>> + *
>> + *    FOLL_PIN on normal pages, or compound pages that are two pages long:
>> + *        page's refcount will be incremented by refs * GUP_PIN_COUNTING_BIAS.
>>    *
>>    * Return: head page (with refcount appropriately incremented) for success, or
>>    * NULL upon failure. If neither FOLL_GET nor FOLL_PIN was set, that's
> 
> Did you run 'make htmldocs' and see how it renders?  I haven't looked,
> but this might work better as an rst list?
> 

OK, after a certain amount of sphinx and sphinx-extension-related
unhappiness, I can finally report, "nope, rst lists do not help here".
That's because the rendered kerneldoc HTML versions of non-numbered
lists look identical to paragraphs that are not lists. And the numbered
lists either look identical (if you used the "1." format), or different
in a way that hurts the source code (if you used the "#." format).

And the lists are also fussier to maintain, because if you do not
*exactly* line up the second and following lines in a paragraph, then
HTML version of the list breaks. Whereas, the HTML looks fine either way
if it is not a list.

I probably shouldn't mention that the only function in gup.c that is
listed as "make this show up in the docs" is get_user_pages_fast(),
because that might lead to people asking to add more items to the
:functions: list in mm-api.rst. And then I'd have to explain that the
kerneldoc rendered output for functions is still mostly useless: kernel
developers need to see the implementation as well; non-kernel developers
will find it incomplete and cryptic; and it's hard to read for anyone,
due to being spread over a country mile's worth of whitespace. So I
won't bring that up. :)


thanks,
-- 
John Hubbard
NVIDIA

[PATCH 2/3] mm/gup: small refactoring: simplify try_grab_page()

[John Hubbard]

try_grab_page() does the same thing as try_grab_compound_head(...,
refs=1, ...), just with a different API. So there is a lot of code
duplication there.

Change try_grab_page() to call try_grab_compound_head(), while keeping
the API contract identical for callers.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/gup.c | 29 ++---------------------------
 1 file changed, 2 insertions(+), 27 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 5cb18b62921c..4be6f060fa0b 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -203,33 +203,8 @@ static void put_compound_head(struct page *page, int refs, unsigned int flags)
  */
 bool __must_check try_grab_page(struct page *page, unsigned int flags)
 {
-	WARN_ON_ONCE((flags & (FOLL_GET | FOLL_PIN)) == (FOLL_GET | FOLL_PIN));
-
-	if (flags & FOLL_GET)
-		return try_get_page(page);
-	else if (flags & FOLL_PIN) {
-		int refs = 1;
-
-		page = compound_head(page);
-
-		if (WARN_ON_ONCE(page_ref_count(page) <= 0))
-			return false;
-
-		if (hpage_pincount_available(page))
-			hpage_pincount_add(page, 1);
-		else
-			refs = GUP_PIN_COUNTING_BIAS;
-
-		/*
-		 * Similar to try_grab_compound_head(): even if using the
-		 * hpage_pincount_add/_sub() routines, be sure to
-		 * *also* increment the normal page refcount field at least
-		 * once, so that the page really is pinned.
-		 */
-		page_ref_add(page, refs);
-
-		mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_ACQUIRED, 1);
-	}
+	if (flags & (FOLL_GET | FOLL_PIN))
+		return try_grab_compound_head(page, 1, flags) != NULL;
 
 	return true;
 }
-- 
2.32.0

Re: [PATCH 2/3] mm/gup: small refactoring: simplify try_grab_page()

[Christoph Hellwig]

On Sun, Aug 08, 2021 at 04:50:17PM -0700, John Hubbard wrote:
> try_grab_page() does the same thing as try_grab_compound_head(...,
> refs=1, ...), just with a different API. So there is a lot of code
> duplication there.
> 
> Change try_grab_page() to call try_grab_compound_head(), while keeping
> the API contract identical for callers.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  mm/gup.c | 29 ++---------------------------
>  1 file changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 5cb18b62921c..4be6f060fa0b 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -203,33 +203,8 @@ static void put_compound_head(struct page *page, int refs, unsigned int flags)
>   */
>  bool __must_check try_grab_page(struct page *page, unsigned int flags)
>  {
> +	if (flags & (FOLL_GET | FOLL_PIN))
> +		return try_grab_compound_head(page, 1, flags) != NULL;
>  
>  	return true;

Nit: something like:

	if (!(flags & (FOLL_GET | FOLL_PIN)))
		return true;
	return try_grab_compound_head(page, 1, flags) != NULL;

would be a little easier to read.

Re: [PATCH 2/3] mm/gup: small refactoring: simplify try_grab_page()

[John Hubbard]

On 8/8/21 11:38 PM, Christoph Hellwig wrote:
> On Sun, Aug 08, 2021 at 04:50:17PM -0700, John Hubbard wrote:
>> try_grab_page() does the same thing as try_grab_compound_head(...,
>> refs=1, ...), just with a different API. So there is a lot of code
>> duplication there.
>>
>> Change try_grab_page() to call try_grab_compound_head(), while keeping
>> the API contract identical for callers.
>>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>   mm/gup.c | 29 ++---------------------------
>>   1 file changed, 2 insertions(+), 27 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 5cb18b62921c..4be6f060fa0b 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -203,33 +203,8 @@ static void put_compound_head(struct page *page, int refs, unsigned int flags)
>>    */
>>   bool __must_check try_grab_page(struct page *page, unsigned int flags)
>>   {
>> +	if (flags & (FOLL_GET | FOLL_PIN))
>> +		return try_grab_compound_head(page, 1, flags) != NULL;
>>   
>>   	return true;
> 
> Nit: something like:
> 
> 	if (!(flags & (FOLL_GET | FOLL_PIN)))
> 		return true;
> 	return try_grab_compound_head(page, 1, flags) != NULL;
> 
> would be a little easier to read.
> 

Really? Well I'll be darned, that's what I wrote in my first draft. And then
I looked at the diffs and thought, "positive logic is clearer, and the diffs
are smaller too", and went with the current version. Which now is apparently
a little worse. oops.

Well, "50-50/90", as we used to say in an earlier job: 50% chance of either
outcome, and due to The Way Things Go, a 90% chance of picking the wrong one!

I can no longer tell which one is easier to read now, so I'll be glad to change
it. :)

thanks,
-- 
John Hubbard
NVIDIA

[PATCH 3/3] mm/gup: refactor and simplify try_get_page()

[John Hubbard]

try_get_page() is very similar to try_get_compound_head(), and in fact
try_get_page() has fallen a little behind in terms of maintenance:
try_get_compound_head() handles speculative page references more
thoroughly.

Change try_get_page() so that it is implemented in terms of
try_get_compound_head(), but without changing try_get_page()'s API
contract.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 include/linux/mm.h | 11 ++++++-----
 mm/gup.c           |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ce8fc0fd6d6e..92d3b37357d5 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1207,14 +1207,15 @@ bool __must_check try_grab_page(struct page *page, unsigned int flags);
 __maybe_unused struct page *try_grab_compound_head(struct page *page, int refs,
 						   unsigned int flags);
 
+struct page *try_get_compound_head(struct page *page, int refs);
 
+/*
+ * This has the same functionality as try_get_compound_head(), just with a
+ * slightly different API.
+ */
 static inline __must_check bool try_get_page(struct page *page)
 {
-	page = compound_head(page);
-	if (WARN_ON_ONCE(page_ref_count(page) <= 0))
-		return false;
-	page_ref_inc(page);
-	return true;
+	return try_get_compound_head(page, 1) != NULL;
 }
 
 /**
diff --git a/mm/gup.c b/mm/gup.c
index 4be6f060fa0b..ba75906ba7f7 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -66,7 +66,7 @@ static void put_page_refs(struct page *page, int refs)
  * Return the compound head page with ref appropriately incremented,
  * or NULL if that failed.
  */
-static inline struct page *try_get_compound_head(struct page *page, int refs)
+struct page *try_get_compound_head(struct page *page, int refs)
 {
 	struct page *head = compound_head(page);
 
-- 
2.32.0

Re: [PATCH 3/3] mm/gup: refactor and simplify try_get_page()

[Christoph Hellwig]

On Sun, Aug 08, 2021 at 04:50:18PM -0700, John Hubbard wrote:
> +/*
> + * This has the same functionality as try_get_compound_head(), just with a
> + * slightly different API.
> + */
>  static inline __must_check bool try_get_page(struct page *page)
>  {
> +	return try_get_compound_head(page, 1) != NULL;

What about removing try_get_page entirely instead?

Re: [PATCH 3/3] mm/gup: refactor and simplify try_get_page()

[John Hubbard]

On 8/8/21 11:29 PM, Christoph Hellwig wrote:
> On Sun, Aug 08, 2021 at 04:50:18PM -0700, John Hubbard wrote:
>> +/*
>> + * This has the same functionality as try_get_compound_head(), just with a
>> + * slightly different API.
>> + */
>>   static inline __must_check bool try_get_page(struct page *page)
>>   {
>> +	return try_get_compound_head(page, 1) != NULL;
> 
> What about removing try_get_page entirely instead?
> 

This thought admittedly crossed my mind, but I was worried maybe I was
getting carried away with cleanups. But now that at least one other
person thinks that's reasonable, I'll be glad to cleanup the callers
too.

thanks,
-- 
John Hubbard
NVIDIA