From: Yury Norov <yury.norov@gmail.com>
To: John Hubbard <jhubbard@nvidia.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Minchan Kim <minchan@kernel.org>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re:
Date: Thu, 21 Apr 2022 16:17:31 -0700 [thread overview]
Message-ID: <YmHmC1VVxEuViqJx@yury-laptop> (raw)
In-Reply-To: <92a2715c-3c98-251d-9195-872d1cf01f9d@nvidia.com>
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/YgWA0ghrrzHONehH@google.com/
>
> Let's add a Cc: line for Michan as well:
>
> Cc: Minchan Kim <minchan@kernel.org>
He's in CC already, I think...
> > Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
> > ---
> > 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
next prev parent reply other threads:[~2022-04-21 23:17 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20220421164138.1250943-1-yury.norov@gmail.com>
2022-04-21 23:04 ` John Hubbard
2022-04-21 23:09 ` Re: John Hubbard
2022-04-21 23:17 ` Yury Norov [this message]
2022-04-21 23:21 ` Re: John Hubbard
2024-02-27 17:42 [PATCH v3 00/18] Rearrange batched folio freeing Matthew Wilcox (Oracle)
2024-02-27 17:42 ` [PATCH v3 10/18] mm: Allow non-hugetlb large folios to be batch processed Matthew Wilcox (Oracle)
2024-03-06 13:42 ` Ryan Roberts
2024-03-06 16:09 ` Matthew Wilcox
2024-03-06 16:19 ` Ryan Roberts
2024-03-06 17:41 ` Ryan Roberts
2024-03-06 18:41 ` Zi Yan
2024-03-06 19:55 ` Matthew Wilcox
2024-03-06 21:55 ` Matthew Wilcox
2024-03-07 8:56 ` Ryan Roberts
2024-03-07 13:50 ` Yin, Fengwei
2024-03-07 14:05 ` Re: Matthew Wilcox
2024-03-07 15:24 ` Re: Ryan Roberts
2024-03-07 16:24 ` Re: Ryan Roberts
2024-03-07 23:02 ` Re: Matthew Wilcox
2024-03-08 1:06 ` Re: Yin, Fengwei
-- strict thread matches above, loose matches on Subject: below --
2024-01-18 22:19 [RFC] [PATCH 0/3] xfs: use large folios for buffers Dave Chinner
2024-01-22 10:13 ` Andi Kleen
2024-01-22 11:53 ` Dave Chinner
2023-05-11 12:58 Ryan Roberts
2023-05-11 13:13 ` Ryan Roberts
2022-08-26 22:03 Zach O'Keefe
2022-08-31 21:47 ` Yang Shi
2022-09-01 0:24 ` Re: Zach O'Keefe
2021-08-12 9:21 Valdis Klētnieks
2021-08-12 9:42 ` SeongJae Park
2021-08-12 20:19 ` Re: Andrew Morton
2021-08-13 8:14 ` Re: SeongJae Park
[not found] <bug-200209-27@https.bugzilla.kernel.org/>
2018-06-28 3:48 ` Andrew Morton
2018-06-29 18:44 ` Andrey Ryabinin
2018-06-24 20:09 [PATCH 3/3] mm: list_lru: Add lock_irq member to __list_lru_init() Vladimir Davydov
2018-07-03 14:52 ` Sebastian Andrzej Siewior
2018-07-03 21:14 ` Andrew Morton
2018-07-03 21:44 ` Re: Sebastian Andrzej Siewior
2018-07-04 14:44 ` Re: Vladimir Davydov
2007-04-03 18:41 Royal VIP Casino
[not found] <F265RQAOCop3wyv9kI3000143b1@hotmail.com>
2001-10-08 11:48 ` Joseph A Knapka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YmHmC1VVxEuViqJx@yury-laptop \
--to=yury.norov@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=jhubbard@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=minchan@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).