| Message ID | 20210808235018.1924918-3-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few gup refactorings and documentation updates | expand |
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.
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,
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; }
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(-)