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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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?
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
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 <[email protected]>
> ---
> 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.
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?
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 <[email protected]>
>> ---
>> 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
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
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