[PATCH v2 0/2] gup.c, gup_benchmark.c trivial fixes before the storm

[PATCH v2 0/2] gup.c, gup_benchmark.c trivial fixes before the storm

[John Hubbard]

Hi,

Changes since v1:

1) Fixed a krobot-reported mistake, which also uncovered a pre-existing
bug. Thanks to Kirill for recommending the fix.

2) Added another small fix: changed the data type to unsigned int, as
pointed out by Ira.

2) Added a "Fixes:" line, thanks to Kirill and Aneesh for pinpointing the
commit.

3) Collected Acked-by and Suggested-by's.

Original cover letter, edited slightly
======================================

These trivial fixes apply to today's linux.git (5.4-rc3).

I found these while polishing up the Next And Final get_user_pages()+dma
tracking patchset (which is in final testing and passing nicely...so far).

Anyway, as these two patches apply cleanly both before and after the larger
gup/dma upcoming patchset, I thought it best to send this out separately,
in order to avoid muddying the waters more than usual.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org

John Hubbard (2):
  mm/gup_benchmark: add a missing "w" to getopt string
  mm/gup: fix a misnamed "write" argument, and a related bug

 mm/gup.c                                   | 14 ++++++++------
 tools/testing/selftests/vm/gup_benchmark.c |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

-- 
2.23.0

[PATCH v2 1/2] mm/gup_benchmark: add a missing "w" to getopt string

[John Hubbard]

Even though gup_benchmark.c has code to handle the -w
command-line option, the "w" is not part of the getopt
string. It looks as if it has been missing the whole time.

On my machine, this leads naturally to the following
predictable result:

$ sudo ./gup_benchmark -w
./gup_benchmark: invalid option -- 'w'

...which is fixed, with this commit.

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/vm/gup_benchmark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index c0534e298b51..cb3fc09645c4 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -37,7 +37,7 @@ int main(int argc, char **argv)
 	char *file = "/dev/zero";
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:f:tTLUSH")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:tTLUwSH")) != -1) {
 		switch (opt) {
 		case 'm':
 			size = atoi(optarg) * MB;
-- 
2.23.0

[PATCH v2 2/2] mm/gup: fix a misnamed "write" argument, and a related bug

[John Hubbard]

In several routines, the "flags" argument is incorrectly
named "write". Change it to "flags".

Also, in one place, the misnaming led to an actual bug:
"flags & FOLL_WRITE" is required, rather than just "flags".
(That problem was flagged by krobot, in v1 of this patch.)

Also, change the flags argument from int, to unsigned int.

You can see that this was a simple oversight, because the
calling code passes "flags" to the fifth argument:

gup_pgd_range():
    ...
    if (!gup_huge_pd(__hugepd(pgd_val(pgd)), addr,
		    PGDIR_SHIFT, next, flags, pages, nr))

...which, until this patch, the callees referred to as "write".

Also, change two lines to avoid checkpatch line length
complaints, and another line to fix another oversight
that checkpatch called out: missing "int" on pdshift.

Fixes: b798bec4741b ("mm/gup: change write parameter to flags in fast walk")
Reported-by: kbuild test robot <lkp@intel.com>
Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/gup.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 23a9f9c9d377..8f236a335ae9 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1973,7 +1973,8 @@ static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end,
 }
 
 static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
-		       unsigned long end, int write, struct page **pages, int *nr)
+		       unsigned long end, unsigned int flags,
+		       struct page **pages, int *nr)
 {
 	unsigned long pte_end;
 	struct page *head, *page;
@@ -1986,7 +1987,7 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
 
 	pte = READ_ONCE(*ptep);
 
-	if (!pte_access_permitted(pte, write))
+	if (!pte_access_permitted(pte, flags & FOLL_WRITE))
 		return 0;
 
 	/* hugepages are never "special" */
@@ -2023,7 +2024,7 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
 }
 
 static int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
-		unsigned int pdshift, unsigned long end, int write,
+		unsigned int pdshift, unsigned long end, unsigned int flags,
 		struct page **pages, int *nr)
 {
 	pte_t *ptep;
@@ -2033,7 +2034,7 @@ static int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
 	ptep = hugepte_offset(hugepd, addr, pdshift);
 	do {
 		next = hugepte_addr_end(addr, end, sz);
-		if (!gup_hugepte(ptep, sz, addr, end, write, pages, nr))
+		if (!gup_hugepte(ptep, sz, addr, end, flags, pages, nr))
 			return 0;
 	} while (ptep++, addr = next, addr != end);
 
@@ -2041,7 +2042,7 @@ static int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
 }
 #else
 static inline int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
-		unsigned pdshift, unsigned long end, int write,
+		unsigned int pdshift, unsigned long end, unsigned int flags,
 		struct page **pages, int *nr)
 {
 	return 0;
@@ -2049,7 +2050,8 @@ static inline int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
 #endif /* CONFIG_ARCH_HAS_HUGEPD */
 
 static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
-		unsigned long end, unsigned int flags, struct page **pages, int *nr)
+			unsigned long end, unsigned int flags,
+			struct page **pages, int *nr)
 {
 	struct page *head, *page;
 	int refs;
-- 
2.23.0

Re: [PATCH v2 2/2] mm/gup: fix a misnamed "write" argument, and a related bug

[Ira Weiny]

On Mon, Oct 14, 2019 at 11:46:39AM -0700, John Hubbard wrote:
> In several routines, the "flags" argument is incorrectly
> named "write". Change it to "flags".
> 
> Also, in one place, the misnaming led to an actual bug:
> "flags & FOLL_WRITE" is required, rather than just "flags".
> (That problem was flagged by krobot, in v1 of this patch.)
> 
> Also, change the flags argument from int, to unsigned int.
> 
> You can see that this was a simple oversight, because the
> calling code passes "flags" to the fifth argument:
> 
> gup_pgd_range():
>     ...
>     if (!gup_huge_pd(__hugepd(pgd_val(pgd)), addr,
> 		    PGDIR_SHIFT, next, flags, pages, nr))
> 
> ...which, until this patch, the callees referred to as "write".
> 
> Also, change two lines to avoid checkpatch line length
> complaints, and another line to fix another oversight
> that checkpatch called out: missing "int" on pdshift.
> 
> Fixes: b798bec4741b ("mm/gup: change write parameter to flags in fast walk")
> Reported-by: kbuild test robot <lkp@intel.com>
> Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  mm/gup.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 23a9f9c9d377..8f236a335ae9 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1973,7 +1973,8 @@ static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end,
>  }
>  
>  static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
> -		       unsigned long end, int write, struct page **pages, int *nr)
> +		       unsigned long end, unsigned int flags,
> +		       struct page **pages, int *nr)
>  {
>  	unsigned long pte_end;
>  	struct page *head, *page;
> @@ -1986,7 +1987,7 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
>  
>  	pte = READ_ONCE(*ptep);
>  
> -	if (!pte_access_permitted(pte, write))
> +	if (!pte_access_permitted(pte, flags & FOLL_WRITE))
>  		return 0;
>  
>  	/* hugepages are never "special" */
> @@ -2023,7 +2024,7 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
>  }
>  
>  static int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
> -		unsigned int pdshift, unsigned long end, int write,
> +		unsigned int pdshift, unsigned long end, unsigned int flags,
>  		struct page **pages, int *nr)
>  {
>  	pte_t *ptep;
> @@ -2033,7 +2034,7 @@ static int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
>  	ptep = hugepte_offset(hugepd, addr, pdshift);
>  	do {
>  		next = hugepte_addr_end(addr, end, sz);
> -		if (!gup_hugepte(ptep, sz, addr, end, write, pages, nr))
> +		if (!gup_hugepte(ptep, sz, addr, end, flags, pages, nr))
>  			return 0;
>  	} while (ptep++, addr = next, addr != end);
>  
> @@ -2041,7 +2042,7 @@ static int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
>  }
>  #else
>  static inline int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
> -		unsigned pdshift, unsigned long end, int write,
> +		unsigned int pdshift, unsigned long end, unsigned int flags,
>  		struct page **pages, int *nr)
>  {
>  	return 0;
> @@ -2049,7 +2050,8 @@ static inline int gup_huge_pd(hugepd_t hugepd, unsigned long addr,
>  #endif /* CONFIG_ARCH_HAS_HUGEPD */
>  
>  static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
> -		unsigned long end, unsigned int flags, struct page **pages, int *nr)
> +			unsigned long end, unsigned int flags,
> +			struct page **pages, int *nr)
>  {
>  	struct page *head, *page;
>  	int refs;
> -- 
> 2.23.0
> 
>