Re:

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?

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>

Please delete each and every one of these one-line comments, because
they merely echo what the code says.

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

On 4/21/22 16:04, John Hubbard wrote:

On 4/21/22 09:41, Yury Norov wrote:

quoted

Subject: [PATCH] mm/gup: fix comments to pin_user_pages_*()

Hi Yuri,

...and I see that I have typo'd both Yury's and Minchan's name (further
down), in the same email!

Really apologize for screwing that up. It's Yury-with-a-"y", I know. :)


thanks,
-- 
John Hubbard
NVIDIA

On Thu, Apr 21, 2022 at 04:04:44PM -0700, John Hubbard wrote:

On 4/21/22 09:41, Yury Norov wrote:

quoted

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.
 

quoted

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.

quoted

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...
 

quoted

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.
 

quoted

+	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:

quoted

Let's add a Cc: line for Michan as well:

Cc: Minchan Kim <minchan@kernel.org>

  
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 <minchan@kernel.org>


thanks,
-- 
John Hubbard
NVIDIA