On Thu, Apr 21, 2022 at 04:04:44PM -0700, John Hubbard wrote:
> On 4/21/22 09:41, Yury Norov wrote:
> > Subject: [PATCH] mm/gup: fix comments to pin_user_pages_*()
> >
>
> Hi Yuri,
>
> Thanks for picking this up. I have been distracted and didn't trust
> myself to focus on this properly, so it's good to have help!
>
> IT/admin point: somehow the first line of the commit description didn't
> make it into an actual email subject. The subject line was blank when it
> arrived in my inbox, and the subject is in the body here instead. Not
> sure how that happened.
>
> Maybe check your git-sendemail setup?
git-sendmail is OK. I just accidentally added empty line above Subject,
which broke format. My bad, sorry for this.
> > pin_user_pages API forces FOLL_PIN in gup_flags, which means that the
> > API requires struct page **pages to be provided (not NULL). However,
> > the comment to pin_user_pages() says:
> >
> > * @pages: array that receives pointers to the pages pinned.
> > * Should be at least nr_pages long. Or NULL, if caller
> > * only intends to ensure the pages are faulted in.
> >
> > This patch fixes comments along the pin_user_pages code, and also adds
> > WARN_ON(!pages), so that API users will have better understanding
> > on how to use it.
>
> No need to quote the code in the commit log. Instead, just summarize.
> For example:
>
> pin_user_pages API forces FOLL_PIN in gup_flags, which means that the
> API requires struct page **pages to be provided (not NULL). However, the
> comment to pin_user_pages() clearly allows for passing in a NULL @pages
> argument.
>
> Remove the incorrect comments, and add WARN_ON_ONCE(!pages) calls to
> enforce the API.
>
> >
> > It has been independently spotted by Minchan Kim and confirmed with
> > John Hubbard:
> >
> > https://lore.kernel.org/all/[email protected]/
>
> Let's add a Cc: line for Michan as well:
>
> Cc: Minchan Kim <[email protected]>
He's in CC already, I think...
> > Signed-off-by: Yury Norov (NVIDIA) <[email protected]>
> > ---
> > mm/gup.c | 26 ++++++++++++++++++++++----
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index f598a037eb04..559626457585 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -2871,6 +2871,10 @@ int pin_user_pages_fast(unsigned long start, int nr_pages,
> > if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> > return -EINVAL;
> > + /* FOLL_PIN requires pages != NULL */
>
> Please delete each and every one of these one-line comments, because
> they merely echo what the code says.
Sure.
> > + if (WARN_ON_ONCE(!pages))
> > + return -EINVAL;
> > +
> > gup_flags |= FOLL_PIN;
> > return internal_get_user_pages_fast(start, nr_pages, gup_flags, pages);
> > }
> > @@ -2893,6 +2897,10 @@ int pin_user_pages_fast_only(unsigned long start, int nr_pages,
> > */
> > if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> > return 0;
> > +
> > + /* FOLL_PIN requires pages != NULL */
> > + if (WARN_ON_ONCE(!pages))
> > + return 0;
> > /*
> > * FOLL_FAST_ONLY is required in order to match the API description of
> > * this routine: no fall back to regular ("slow") GUP.
> > @@ -2920,8 +2928,7 @@ EXPORT_SYMBOL_GPL(pin_user_pages_fast_only);
> > * @nr_pages: number of pages from start to pin
> > * @gup_flags: flags modifying lookup behaviour
> > * @pages: array that receives pointers to the pages pinned.
> > - * Should be at least nr_pages long. Or NULL, if caller
> > - * only intends to ensure the pages are faulted in.
> > + * Should be at least nr_pages long.
> > * @vmas: array of pointers to vmas corresponding to each page.
> > * Or NULL if the caller does not require them.
> > * @locked: pointer to lock flag indicating whether lock is held and
> > @@ -2944,6 +2951,10 @@ long pin_user_pages_remote(struct mm_struct *mm,
> > if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> > return -EINVAL;
> > + /* FOLL_PIN requires pages != NULL */
> > + if (WARN_ON_ONCE(!pages))
> > + return -EINVAL;
> > +
> > gup_flags |= FOLL_PIN;
> > return __get_user_pages_remote(mm, start, nr_pages, gup_flags,
> > pages, vmas, locked);
> > @@ -2957,8 +2968,7 @@ EXPORT_SYMBOL(pin_user_pages_remote);
> > * @nr_pages: number of pages from start to pin
> > * @gup_flags: flags modifying lookup behaviour
> > * @pages: array that receives pointers to the pages pinned.
> > - * Should be at least nr_pages long. Or NULL, if caller
> > - * only intends to ensure the pages are faulted in.
> > + * Should be at least nr_pages long.
> > * @vmas: array of pointers to vmas corresponding to each page.
> > * Or NULL if the caller does not require them.
> > *
> > @@ -2976,6 +2986,10 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages,
> > if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> > return -EINVAL;
> > + /* FOLL_PIN requires pages != NULL */
> > + if (WARN_ON_ONCE(!pages))
> > + return -EINVAL;
> > +
> > gup_flags |= FOLL_PIN;
> > return __gup_longterm_locked(current->mm, start, nr_pages,
> > pages, vmas, gup_flags);
> > @@ -2994,6 +3008,10 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
> > if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> > return -EINVAL;
> > + /* FOLL_PIN requires pages != NULL */
> > + if (WARN_ON_ONCE(!pages))
> > + return -EINVAL;
> > +
> > gup_flags |= FOLL_PIN;
> > return get_user_pages_unlocked(start, nr_pages, pages, gup_flags);
> > }
>
> I hope we don't break any callers with the newly enforced !pages, but it's
> the right thing to do, in order to avoid misunderstandings.
>
> thanks,
> --
> John Hubbard
> NVIDIA
Let me test v2 and resend shortly.
Thanks,
Yury
On 4/21/22 16:17, Yury Norov wrote:
>> Let's add a Cc: line for Michan as well:
>>
>> Cc: Minchan Kim <[email protected]>
>
> He's in CC already, I think...
>
Here, I am talking about attribution in the commit log, as opposed
to the email Cc. In other words, I'm suggesting that you literally
add this line to the commit description:
Cc: Minchan Kim <[email protected]>
thanks,
--
John Hubbard
NVIDIA