[PATCH 0/6] Minor improvements for pagewalk code

[PATCH 0/6] Minor improvements for pagewalk code

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

For some project I had to use the pagewalk API for certain things and during 
this have read through the code multiple times. Our usage has changed several 
times depending on our current state of research as well.

During all of this I have made some tweaks to the code to be able to follow it 
better when hunting my own problems, and not call into some things that I 
actually don't need. The patches are more or less independent of each other. 
Especially the last one may heavily depend on personal taste, so if you don't 
like it, just ignore it.

I would welcome if you could just pick those that you think are fitting and 
provide feedback on either of the remaining ones. At the end none of them 
should make any functional difference.

Regards,

Eike
-- 
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 313 bytes --]

[PATCH 2/6] mm: pagewalk: don't check vma in walk_page_range_novma()

[Rolf Eike Beer]

Directly call walk_pgd_range() as that is everything that will actually
happen in __walk_page_range() besides checking if the vma is set.

Signed-off-by: Rolf Eike Beer <eb@emlix.com>
---
 mm/pagewalk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index c67b2532db8f..a06eb100fe4e 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -517,7 +517,7 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
 
 	mmap_assert_locked(walk.mm);
 
-	return __walk_page_range(start, end, &walk);
+	return walk_pgd_range(start, end, &walk);
 }
 
 int walk_page_vma(struct vm_area_struct *vma, const struct mm_walk_ops *ops,
-- 
2.37.2


-- 
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[PATCH 3/6] mm: pagewalk: fix documentation of PTE hole handling

[Rolf Eike Beer]

Empty PTEs are passed to the pte_entry callback, not to pte_hole.

Signed-off-by: Rolf Eike Beer <eb@emlix.com>
---
 include/linux/pagewalk.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
index ac7b38ad5903..f3fafb731ffd 100644
--- a/include/linux/pagewalk.h
+++ b/include/linux/pagewalk.h
@@ -15,12 +15,12 @@ struct mm_walk;
  *			this handler is required to be able to handle
  *			pmd_trans_huge() pmds.  They may simply choose to
  *			split_huge_page() instead of handling it explicitly.
- * @pte_entry:		if set, called for each non-empty PTE (lowest-level)
- *			entry
+ * @pte_entry:		if set, called for each PTE (lowest-level) entry,
+ *			including empty ones
  * @pte_hole:		if set, called for each hole at all levels,
- *			depth is -1 if not known, 0:PGD, 1:P4D, 2:PUD, 3:PMD
- *			4:PTE. Any folded depths (where PTRS_PER_P?D is equal
- *			to 1) are skipped.
+ *			depth is -1 if not known, 0:PGD, 1:P4D, 2:PUD, 3:PMD.
+ *			Any folded depths (where PTRS_PER_P?D is equal to 1)
+ *			are skipped.
  * @hugetlb_entry:	if set, called for each hugetlb entry
  * @test_walk:		caller specific callback function to determine whether
  *			we walk over the current vma or not. Returning 0 means
-- 
2.37.2

-- 
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[PATCH 4/6] mm: pagewalk: add api documentation for walk_page_range_novma()

[Rolf Eike Beer]

Signed-off-by: Rolf Eike Beer <eb@emlix.com>
---
 mm/pagewalk.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index a06eb100fe4e..dc3c850cd148 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -493,7 +493,15 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
 	return 0;
 }
 
-/*
+/**
+ * walk_page_range_novma - walk a range of pagetables not backed by a vma
+ * @mm:		mm_struct representing the target process of page table walk
+ * @start:	start address of the virtual address range
+ * @end:	end address of the virtual address range
+ * @ops:	operation to call during the walk
+ * @pgd:	pgd to walk if different from mm->pgd
+ * @private:	private data for callbacks' usage
+ *
  * Similar to walk_page_range() but can walk any page tables even if they are
  * not backed by VMAs. Because 'unusual' entries may be walked this function
  * will also not lock the PTEs for the pte_entry() callback. This is useful for
-- 
2.37.2

-- 
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[PATCH 5/6] mm: pagewalk: allow walk_page_range_novma() without mm

[Rolf Eike Beer]

Since e47690d756a7 ("x86: mm: avoid allocating struct mm_struct on the
stack") a pgd can be passed to walk_page_range_novma(). In case it is set
no place in the pagewalk code use the walk.mm anymore, so permit to pass a
NULL mm instead. It is up to the caller to ensure proper locking on the pgd
in this case.

Signed-off-by: Rolf Eike Beer <eb@emlix.com>
---
 mm/pagewalk.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index dc3c850cd148..f816f86588be 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -506,6 +506,8 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
  * not backed by VMAs. Because 'unusual' entries may be walked this function
  * will also not lock the PTEs for the pte_entry() callback. This is useful for
  * walking the kernel pages tables or page tables for firmware.
+ *
+ * Either mm or pgd may be NULL, but not both.
  */
 int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
 			  unsigned long end, const struct mm_walk_ops *ops,
@@ -520,10 +522,11 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
 		.no_vma		= true
 	};
 
-	if (start >= end || !walk.mm)
+	if (start >= end || (!walk.mm && !walk.pgd))
 		return -EINVAL;
 
-	mmap_assert_locked(walk.mm);
+	if (walk.mm)
+		mmap_assert_locked(walk.mm);
 
 	return walk_pgd_range(start, end, &walk);
 }
-- 
2.37.2

-- 
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[PATCH 6/6] mm: pagewalk: move variables to more local scope, tweak loops

[Rolf Eike Beer]

Move some variables to more local scopes to make it obvious that they don't
carry state. Put the end additions into the for loop instructions to make
them easier to read.

Signed-off-by: Rolf Eike Beer <eb@emlix.com>
---
 mm/pagewalk.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index f816f86588be..d8be8a30b272 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -25,14 +25,12 @@ static int walk_pte_range_inner(pte_t *pte, unsigned long addr,
 {
 	const struct mm_walk_ops *ops = walk->ops;
 
-	for (;;) {
+	for (;; addr += PAGE_SIZE, pte++) {
 		int err = ops->pte_entry(pte, addr, addr + PAGE_SIZE, walk);
 		if (err)
 			return err;
 		if (addr >= end - PAGE_SIZE)
 			break;
-		addr += PAGE_SIZE;
-		pte++;
 	}
 	return 0;
 }
@@ -42,13 +40,14 @@ static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 {
 	pte_t *pte;
 	int err;
-	spinlock_t *ptl;
 
 	if (walk->no_vma) {
 		pte = pte_offset_map(pmd, addr);
 		err = walk_pte_range_inner(pte, addr, end, walk);
 		pte_unmap(pte);
 	} else {
+		spinlock_t *ptl;
+
 		pte = pte_offset_map_lock(walk->mm, pmd, addr, &ptl);
 		err = walk_pte_range_inner(pte, addr, end, walk);
 		pte_unmap_unlock(pte, ptl);
@@ -71,7 +70,7 @@ static int walk_hugepd_range(hugepd_t *phpd, unsigned long addr,
 	if (addr & (page_size - 1))
 		return 0;
 
-	for (;;) {
+	for (;; addr += page_size) {
 		pte_t *pte;
 		int err;
 
@@ -84,7 +83,6 @@ static int walk_hugepd_range(hugepd_t *phpd, unsigned long addr,
 			return err;
 		if (addr >= end - page_size)
 			break;
-		addr += page_size;
 	}
 	return 0;
 }
@@ -307,14 +305,13 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
 	unsigned long next;
 	unsigned long hmask = huge_page_mask(h);
 	unsigned long sz = huge_page_size(h);
-	pte_t *pte;
 	const struct mm_walk_ops *ops = walk->ops;
 
-	do {
+	for (; addr < end; addr = next) {
 		int err;
+		pte_t *pte = huge_pte_offset(walk->mm, addr & hmask, sz);
 
 		next = hugetlb_entry_end(h, addr, end);
-		pte = huge_pte_offset(walk->mm, addr & hmask, sz);
 
 		if (pte)
 			err = ops->hugetlb_entry(pte, hmask, addr, next, walk);
@@ -323,7 +320,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
 
 		if (err)
 			return err;
-	} while (addr = next, addr != end);
+	}
 
 	return 0;
 }
@@ -461,14 +458,13 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
 	do {
 		int err;
 
+		walk.vma = vma;
 		if (!vma) { /* after the last vma */
-			walk.vma = NULL;
 			next = end;
 		} else if (start < vma->vm_start) { /* outside vma */
 			walk.vma = NULL;
 			next = min(end, vma->vm_start);
 		} else { /* inside vma */
-			walk.vma = vma;
 			next = min(end, vma->vm_end);
 			vma = vma->vm_next;
 
@@ -595,11 +591,11 @@ int walk_page_mapping(struct address_space *mapping, pgoff_t first_index,
 	};
 	struct vm_area_struct *vma;
 	pgoff_t vba, vea, cba, cea;
-	unsigned long start_addr, end_addr;
 
 	lockdep_assert_held(&mapping->i_mmap_rwsem);
 	vma_interval_tree_foreach(vma, &mapping->i_mmap, first_index,
 				  first_index + nr - 1) {
+		unsigned long start_addr, end_addr;
 		int err;
 
 		/* Clip to the vma */
-- 
2.37.2

-- 
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

Re: [PATCH 1/6] mm: pagewalk: make error checks more obvious

[Andrew Morton]

On Mon, 22 Aug 2022 15:00:05 +0200 Rolf Eike Beer <eb@emlix.com> wrote:

> The err variable only needs to be checked when it was assigned directly
> before, it is not carried on to any later checks. Move the checks into the
> same "if" conditions where they are assigned. Also just return the error at
> the relevant places. While at it move these err variables to a more local
> scope at some places.
> 
> ...
>
> @@ -593,16 +608,15 @@ int walk_page_mapping(struct address_space *mapping, pgoff_t first_index,
>  		walk.mm = vma->vm_mm;
>  
>  		err = walk_page_test(vma->vm_start, vma->vm_end, &walk);
> -		if (err > 0) {
> -			err = 0;
> -			break;
> -		} else if (err < 0)
> -			break;
> +		if (err > 0)
> +			return 0;
> +		else if (err < 0)
> +			return err;
>  
>  		err = __walk_page_range(start_addr, end_addr, &walk);
>  		if (err)
> -			break;
> +			return err;
>  	}
>  
> -	return err;
> +	return 0;
>  }

I'm not really a fan of multiple return points - it tends to lead to
locking/resource leaks as the code evolves.  I don't really think it's
worth redoing the patch for this reason though; the rest looks good.

[PATCH 7/6] mm: pagewalk: add back missing variable initializations

[Rolf Eike Beer]

These initializations accidentially got lost during refactoring.

The first one can't actually be used without initialization, because
walk_p4d_range() is only called when one of the 4 callbacks is set, but relying
on this seems fragile.

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Rolf Eike Beer <eb@emlix.com>
---
 mm/pagewalk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Looks like my testing had just the right callbacks set so I never hit this. In 
case you want to have this squashed into the original one just drop me a note 
and I'll redo the whole series.

diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index d8be8a30b272..22e4bcca9bee 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -223,7 +223,7 @@ static int walk_p4d_range(pgd_t *pgd, unsigned long addr, unsigned long end,
 
 	p4d = p4d_offset(pgd, addr);
 	do {
-		int err;
+		int err = 0;
 
 		next = p4d_addr_end(addr, end);
 		if (p4d_none_or_clear_bad(p4d)) {
@@ -262,7 +262,7 @@ static int walk_pgd_range(unsigned long addr, unsigned long end,
 	else
 		pgd = pgd_offset(walk->mm, addr);
 	do {
-		int err;
+		int err = 0;
 
 		next = pgd_addr_end(addr, end);
 		if (pgd_none_or_clear_bad(pgd)) {
@@ -308,7 +308,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
 	const struct mm_walk_ops *ops = walk->ops;
 
 	for (; addr < end; addr = next) {
-		int err;
+		int err = 0;
 		pte_t *pte = huge_pte_offset(walk->mm, addr & hmask, sz);
 
 		next = hugetlb_entry_end(h, addr, end);
-- 
2.37.2

-- 
Rolf Eike Beer, emlix GmbH, https://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

Re: [PATCH 7/6] mm: pagewalk: add back missing variable initializations

[Nathan Chancellor]

On Wed, Aug 24, 2022 at 01:00:11PM +0200, Rolf Eike Beer wrote:
> These initializations accidentially got lost during refactoring.
> 
> The first one can't actually be used without initialization, because
> walk_p4d_range() is only called when one of the 4 callbacks is set, but relying
> on this seems fragile.
> 
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Rolf Eike Beer <eb@emlix.com>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  mm/pagewalk.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Looks like my testing had just the right callbacks set so I never hit this. In 
> case you want to have this squashed into the original one just drop me a note 
> and I'll redo the whole series.
> 
> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> index d8be8a30b272..22e4bcca9bee 100644
> --- a/mm/pagewalk.c
> +++ b/mm/pagewalk.c
> @@ -223,7 +223,7 @@ static int walk_p4d_range(pgd_t *pgd, unsigned long addr, unsigned long end,
>  
>  	p4d = p4d_offset(pgd, addr);
>  	do {
> -		int err;
> +		int err = 0;
>  
>  		next = p4d_addr_end(addr, end);
>  		if (p4d_none_or_clear_bad(p4d)) {
> @@ -262,7 +262,7 @@ static int walk_pgd_range(unsigned long addr, unsigned long end,
>  	else
>  		pgd = pgd_offset(walk->mm, addr);
>  	do {
> -		int err;
> +		int err = 0;
>  
>  		next = pgd_addr_end(addr, end);
>  		if (pgd_none_or_clear_bad(pgd)) {
> @@ -308,7 +308,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
>  	const struct mm_walk_ops *ops = walk->ops;
>  
>  	for (; addr < end; addr = next) {
> -		int err;
> +		int err = 0;
>  		pte_t *pte = huge_pte_offset(walk->mm, addr & hmask, sz);
>  
>  		next = hugetlb_entry_end(h, addr, end);
> -- 
> 2.37.2
> 
> -- 
> Rolf Eike Beer, emlix GmbH, https://www.emlix.com
> Fon +49 551 30664-0, Fax +49 551 30664-11
> Gothaer Platz 3, 37083 Göttingen, Germany
> Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
> Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055
> 
> emlix - smart embedded open source
> 
>