[PATCH 0/3] mm/vma: miss to restore vmi.index on expansion failure

[PATCH 0/3] mm/vma: miss to restore vmi.index on expansion failure

[Wei Yang]

This serial starts on spotting on miss to restore vmi.index in
vma_merge_new_range() when expansion is failed.

Patch 2 add a test to assert this.

While found build error on old platform, because parameter name missing. Patch
3 is to fix this.

Wei Yang (3):
  mm/vma: miss to restore vmi.index on expansion failure
  tools: test vmi.index would be restored on merge failure
  tools: fix build error on parameter name omitted

 mm/vma.c                         | 10 ++--
 tools/testing/vma/vma.c          |  8 +++-
 tools/testing/vma/vma_internal.h | 82 ++++++++++++++++----------------
 3 files changed, 55 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Wei Yang]

On expansion failure, we try to restore vmg state, but we missed to
restore vmi.index. The reason is we have reset vmg->vma before checking.
So let's put the operation before reset vmg->vma.

Also we don't need to do the restore if there is no mergeable adjacent
VMA. Let's take it out to skip the unnecessary operations.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/vma.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/vma.c b/mm/vma.c
index fb4f1863f88e..c94d953d453c 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -954,23 +954,27 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
 		vma_prev(vmg->vmi); /* Equivalent to going to the previous range */
 	}
 
+	/* No mergeable adjacent VMA, return */
+	if (!vmg->vma)
+		return NULL;
+
 	/*
 	 * Now try to expand adjacent VMA(s). This takes care of removing the
 	 * following VMA if we have VMAs on both sides.
 	 */
-	if (vmg->vma && !vma_expand(vmg)) {
+	if (!vma_expand(vmg)) {
 		khugepaged_enter_vma(vmg->vma, vmg->flags);
 		vmg->state = VMA_MERGE_SUCCESS;
 		return vmg->vma;
 	}
 
 	/* If expansion failed, reset state. Allows us to retry merge later. */
+	if (vmg->vma == prev)
+		vma_iter_set(vmg->vmi, start);
 	vmg->vma = NULL;
 	vmg->start = start;
 	vmg->end = end;
 	vmg->pgoff = pgoff;
-	if (vmg->vma == prev)
-		vma_iter_set(vmg->vmi, start);
 
 	return NULL;
 }
-- 
2.34.1

[PATCH 2/3] tools: test vmi.index would be restored on merge failure

[Wei Yang]

Add an assertion on restoring the correct index on failure.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 tools/testing/vma/vma.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c
index c53f220eb6cc..46e8989a90ae 100644
--- a/tools/testing/vma/vma.c
+++ b/tools/testing/vma/vma.c
@@ -243,6 +243,12 @@ static bool test_simple_merge(void)
 	ASSERT_FALSE(vma_link(&mm, vma_left));
 	ASSERT_FALSE(vma_link(&mm, vma_right));
 
+	fail_prealloc = true;
+	vma = merge_new(&vmg);
+	ASSERT_EQ(vma, NULL);
+	ASSERT_EQ(vmi.mas.index, 0x1000);
+
+	fail_prealloc = false;
 	vma = merge_new(&vmg);
 	ASSERT_NE(vma, NULL);
 
-- 
2.34.1

[PATCH 3/3] tools: fix build error on parameter name omitted

[Wei Yang]

On some old version gcc, e.g. 8.2.1, it is treated an error.

Just add a name for it.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 tools/testing/vma/vma.c          |  2 +-
 tools/testing/vma/vma_internal.h | 82 ++++++++++++++++----------------
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c
index 46e8989a90ae..212b14a52bd3 100644
--- a/tools/testing/vma/vma.c
+++ b/tools/testing/vma/vma.c
@@ -219,7 +219,7 @@ static bool vma_write_started(struct vm_area_struct *vma)
 }
 
 /* Helper function providing a dummy vm_ops->close() method.*/
-static void dummy_close(struct vm_area_struct *)
+static void dummy_close(struct vm_area_struct *vma)
 {
 }
 
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index c5b9da034511..b7aa9369796e 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -620,11 +620,11 @@ static inline unsigned long vma_pages(struct vm_area_struct *vma)
 	return (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
 }
 
-static inline void fput(struct file *)
+static inline void fput(struct file *f)
 {
 }
 
-static inline void mpol_put(struct mempolicy *)
+static inline void mpol_put(struct mempolicy *mp)
 {
 }
 
@@ -648,15 +648,15 @@ static inline void lru_add_drain(void)
 {
 }
 
-static inline void tlb_gather_mmu(struct mmu_gather *, struct mm_struct *)
+static inline void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm)
 {
 }
 
-static inline void update_hiwater_rss(struct mm_struct *)
+static inline void update_hiwater_rss(struct mm_struct *mm)
 {
 }
 
-static inline void update_hiwater_vm(struct mm_struct *)
+static inline void update_hiwater_vm(struct mm_struct *mm)
 {
 }
 
@@ -686,23 +686,23 @@ static inline void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
 	(void)mm_wr_locked;
 }
 
-static inline void mapping_unmap_writable(struct address_space *)
+static inline void mapping_unmap_writable(struct address_space *mapping)
 {
 }
 
-static inline void flush_dcache_mmap_lock(struct address_space *)
+static inline void flush_dcache_mmap_lock(struct address_space *mapping)
 {
 }
 
-static inline void tlb_finish_mmu(struct mmu_gather *)
+static inline void tlb_finish_mmu(struct mmu_gather *tlb)
 {
 }
 
-static inline void get_file(struct file *)
+static inline void get_file(struct file *f)
 {
 }
 
-static inline int vma_dup_policy(struct vm_area_struct *, struct vm_area_struct *)
+static inline int vma_dup_policy(struct vm_area_struct *src, struct vm_area_struct *dst)
 {
 	return 0;
 }
@@ -750,31 +750,31 @@ static inline void vm_acct_memory(long pages)
 {
 }
 
-static inline void vma_interval_tree_insert(struct vm_area_struct *,
-					    struct rb_root_cached *)
+static inline void vma_interval_tree_insert(struct vm_area_struct *vma,
+					    struct rb_root_cached *root)
 {
 }
 
-static inline void vma_interval_tree_remove(struct vm_area_struct *,
-					    struct rb_root_cached *)
+static inline void vma_interval_tree_remove(struct vm_area_struct *vma,
+					    struct rb_root_cached *root)
 {
 }
 
-static inline void flush_dcache_mmap_unlock(struct address_space *)
+static inline void flush_dcache_mmap_unlock(struct address_space *mapping)
 {
 }
 
-static inline void anon_vma_interval_tree_insert(struct anon_vma_chain*,
-						 struct rb_root_cached *)
+static inline void anon_vma_interval_tree_insert(struct anon_vma_chain *node,
+						 struct rb_root_cached *root)
 {
 }
 
-static inline void anon_vma_interval_tree_remove(struct anon_vma_chain*,
-						 struct rb_root_cached *)
+static inline void anon_vma_interval_tree_remove(struct anon_vma_chain *node,
+						 struct rb_root_cached *root)
 {
 }
 
-static inline void uprobe_mmap(struct vm_area_struct *)
+static inline void uprobe_mmap(struct vm_area_struct *vma)
 {
 }
 
@@ -786,15 +786,15 @@ static inline void uprobe_munmap(struct vm_area_struct *vma,
 	(void)end;
 }
 
-static inline void i_mmap_lock_write(struct address_space *)
+static inline void i_mmap_lock_write(struct address_space *mapping)
 {
 }
 
-static inline void anon_vma_lock_write(struct anon_vma *)
+static inline void anon_vma_lock_write(struct anon_vma *anon_vma)
 {
 }
 
-static inline void vma_assert_write_locked(struct vm_area_struct *)
+static inline void vma_assert_write_locked(struct vm_area_struct *vma)
 {
 }
 
@@ -804,16 +804,16 @@ static inline void unlink_anon_vmas(struct vm_area_struct *vma)
 	vma->anon_vma->was_unlinked = true;
 }
 
-static inline void anon_vma_unlock_write(struct anon_vma *)
+static inline void anon_vma_unlock_write(struct anon_vma *anon_vma)
 {
 }
 
-static inline void i_mmap_unlock_write(struct address_space *)
+static inline void i_mmap_unlock_write(struct address_space *mapping)
 {
 }
 
-static inline void anon_vma_merge(struct vm_area_struct *,
-				  struct vm_area_struct *)
+static inline void anon_vma_merge(struct vm_area_struct *vma,
+				  struct vm_area_struct *next)
 {
 }
 
@@ -830,15 +830,15 @@ static inline int userfaultfd_unmap_prep(struct vm_area_struct *vma,
 	return 0;
 }
 
-static inline void mmap_write_downgrade(struct mm_struct *)
+static inline void mmap_write_downgrade(struct mm_struct *mm)
 {
 }
 
-static inline void mmap_read_unlock(struct mm_struct *)
+static inline void mmap_read_unlock(struct mm_struct *mm)
 {
 }
 
-static inline void mmap_write_unlock(struct mm_struct *)
+static inline void mmap_write_unlock(struct mm_struct *mm)
 {
 }
 
@@ -862,11 +862,11 @@ static inline void arch_unmap(struct mm_struct *mm,
 	(void)end;
 }
 
-static inline void mmap_assert_locked(struct mm_struct *)
+static inline void mmap_assert_locked(struct mm_struct *mm)
 {
 }
 
-static inline bool mpol_equal(struct mempolicy *, struct mempolicy *)
+static inline bool mpol_equal(struct mempolicy *a, struct mempolicy *b)
 {
 	return true;
 }
@@ -878,44 +878,44 @@ static inline void khugepaged_enter_vma(struct vm_area_struct *vma,
 	(void)vm_flags;
 }
 
-static inline bool mapping_can_writeback(struct address_space *)
+static inline bool mapping_can_writeback(struct address_space *mapping)
 {
 	return true;
 }
 
-static inline bool is_vm_hugetlb_page(struct vm_area_struct *)
+static inline bool is_vm_hugetlb_page(struct vm_area_struct *vma)
 {
 	return false;
 }
 
-static inline bool vma_soft_dirty_enabled(struct vm_area_struct *)
+static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma)
 {
 	return false;
 }
 
-static inline bool userfaultfd_wp(struct vm_area_struct *)
+static inline bool userfaultfd_wp(struct vm_area_struct *vma)
 {
 	return false;
 }
 
-static inline void mmap_assert_write_locked(struct mm_struct *)
+static inline void mmap_assert_write_locked(struct mm_struct *mm)
 {
 }
 
-static inline void mutex_lock(struct mutex *)
+static inline void mutex_lock(struct mutex *lock)
 {
 }
 
-static inline void mutex_unlock(struct mutex *)
+static inline void mutex_unlock(struct mutex *lock)
 {
 }
 
-static inline bool mutex_is_locked(struct mutex *)
+static inline bool mutex_is_locked(struct mutex *lock)
 {
 	return true;
 }
 
-static inline bool signal_pending(void *)
+static inline bool signal_pending(void *p)
 {
 	return false;
 }
-- 
2.34.1

Re: [PATCH 3/3] tools: fix build error on parameter name omitted

[Lorenzo Stoakes]

On Fri, Oct 25, 2024 at 03:18:47AM +0000, Wei Yang wrote:
> On some old version gcc, e.g. 8.2.1, it is treated an error.
>
> Just add a name for it.

No, sorry. This is just churn and no bot has an issue with this, and we
don't support ye olde compilers forever doing dumb things.

I'm guessing this is actually a warning but CONFIG_WERROR is turned on.

>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  tools/testing/vma/vma.c          |  2 +-
>  tools/testing/vma/vma_internal.h | 82 ++++++++++++++++----------------
>  2 files changed, 42 insertions(+), 42 deletions(-)
>
> diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c
> index 46e8989a90ae..212b14a52bd3 100644
> --- a/tools/testing/vma/vma.c
> +++ b/tools/testing/vma/vma.c
> @@ -219,7 +219,7 @@ static bool vma_write_started(struct vm_area_struct *vma)
>  }
>
>  /* Helper function providing a dummy vm_ops->close() method.*/
> -static void dummy_close(struct vm_area_struct *)
> +static void dummy_close(struct vm_area_struct *vma)
>  {
>  }
>
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index c5b9da034511..b7aa9369796e 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -620,11 +620,11 @@ static inline unsigned long vma_pages(struct vm_area_struct *vma)
>  	return (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
>  }
>
> -static inline void fput(struct file *)
> +static inline void fput(struct file *f)
>  {
>  }
>
> -static inline void mpol_put(struct mempolicy *)
> +static inline void mpol_put(struct mempolicy *mp)
>  {
>  }
>
> @@ -648,15 +648,15 @@ static inline void lru_add_drain(void)
>  {
>  }
>
> -static inline void tlb_gather_mmu(struct mmu_gather *, struct mm_struct *)
> +static inline void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm)
>  {
>  }
>
> -static inline void update_hiwater_rss(struct mm_struct *)
> +static inline void update_hiwater_rss(struct mm_struct *mm)
>  {
>  }
>
> -static inline void update_hiwater_vm(struct mm_struct *)
> +static inline void update_hiwater_vm(struct mm_struct *mm)
>  {
>  }
>
> @@ -686,23 +686,23 @@ static inline void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
>  	(void)mm_wr_locked;
>  }
>
> -static inline void mapping_unmap_writable(struct address_space *)
> +static inline void mapping_unmap_writable(struct address_space *mapping)
>  {
>  }
>
> -static inline void flush_dcache_mmap_lock(struct address_space *)
> +static inline void flush_dcache_mmap_lock(struct address_space *mapping)
>  {
>  }
>
> -static inline void tlb_finish_mmu(struct mmu_gather *)
> +static inline void tlb_finish_mmu(struct mmu_gather *tlb)
>  {
>  }
>
> -static inline void get_file(struct file *)
> +static inline void get_file(struct file *f)
>  {
>  }
>
> -static inline int vma_dup_policy(struct vm_area_struct *, struct vm_area_struct *)
> +static inline int vma_dup_policy(struct vm_area_struct *src, struct vm_area_struct *dst)
>  {
>  	return 0;
>  }
> @@ -750,31 +750,31 @@ static inline void vm_acct_memory(long pages)
>  {
>  }
>
> -static inline void vma_interval_tree_insert(struct vm_area_struct *,
> -					    struct rb_root_cached *)
> +static inline void vma_interval_tree_insert(struct vm_area_struct *vma,
> +					    struct rb_root_cached *root)
>  {
>  }
>
> -static inline void vma_interval_tree_remove(struct vm_area_struct *,
> -					    struct rb_root_cached *)
> +static inline void vma_interval_tree_remove(struct vm_area_struct *vma,
> +					    struct rb_root_cached *root)
>  {
>  }
>
> -static inline void flush_dcache_mmap_unlock(struct address_space *)
> +static inline void flush_dcache_mmap_unlock(struct address_space *mapping)
>  {
>  }
>
> -static inline void anon_vma_interval_tree_insert(struct anon_vma_chain*,
> -						 struct rb_root_cached *)
> +static inline void anon_vma_interval_tree_insert(struct anon_vma_chain *node,
> +						 struct rb_root_cached *root)
>  {
>  }
>
> -static inline void anon_vma_interval_tree_remove(struct anon_vma_chain*,
> -						 struct rb_root_cached *)
> +static inline void anon_vma_interval_tree_remove(struct anon_vma_chain *node,
> +						 struct rb_root_cached *root)
>  {
>  }
>
> -static inline void uprobe_mmap(struct vm_area_struct *)
> +static inline void uprobe_mmap(struct vm_area_struct *vma)
>  {
>  }
>
> @@ -786,15 +786,15 @@ static inline void uprobe_munmap(struct vm_area_struct *vma,
>  	(void)end;
>  }
>
> -static inline void i_mmap_lock_write(struct address_space *)
> +static inline void i_mmap_lock_write(struct address_space *mapping)
>  {
>  }
>
> -static inline void anon_vma_lock_write(struct anon_vma *)
> +static inline void anon_vma_lock_write(struct anon_vma *anon_vma)
>  {
>  }
>
> -static inline void vma_assert_write_locked(struct vm_area_struct *)
> +static inline void vma_assert_write_locked(struct vm_area_struct *vma)
>  {
>  }
>
> @@ -804,16 +804,16 @@ static inline void unlink_anon_vmas(struct vm_area_struct *vma)
>  	vma->anon_vma->was_unlinked = true;
>  }
>
> -static inline void anon_vma_unlock_write(struct anon_vma *)
> +static inline void anon_vma_unlock_write(struct anon_vma *anon_vma)
>  {
>  }
>
> -static inline void i_mmap_unlock_write(struct address_space *)
> +static inline void i_mmap_unlock_write(struct address_space *mapping)
>  {
>  }
>
> -static inline void anon_vma_merge(struct vm_area_struct *,
> -				  struct vm_area_struct *)
> +static inline void anon_vma_merge(struct vm_area_struct *vma,
> +				  struct vm_area_struct *next)
>  {
>  }
>
> @@ -830,15 +830,15 @@ static inline int userfaultfd_unmap_prep(struct vm_area_struct *vma,
>  	return 0;
>  }
>
> -static inline void mmap_write_downgrade(struct mm_struct *)
> +static inline void mmap_write_downgrade(struct mm_struct *mm)
>  {
>  }
>
> -static inline void mmap_read_unlock(struct mm_struct *)
> +static inline void mmap_read_unlock(struct mm_struct *mm)
>  {
>  }
>
> -static inline void mmap_write_unlock(struct mm_struct *)
> +static inline void mmap_write_unlock(struct mm_struct *mm)
>  {
>  }
>
> @@ -862,11 +862,11 @@ static inline void arch_unmap(struct mm_struct *mm,
>  	(void)end;
>  }
>
> -static inline void mmap_assert_locked(struct mm_struct *)
> +static inline void mmap_assert_locked(struct mm_struct *mm)
>  {
>  }
>
> -static inline bool mpol_equal(struct mempolicy *, struct mempolicy *)
> +static inline bool mpol_equal(struct mempolicy *a, struct mempolicy *b)
>  {
>  	return true;
>  }
> @@ -878,44 +878,44 @@ static inline void khugepaged_enter_vma(struct vm_area_struct *vma,
>  	(void)vm_flags;
>  }
>
> -static inline bool mapping_can_writeback(struct address_space *)
> +static inline bool mapping_can_writeback(struct address_space *mapping)
>  {
>  	return true;
>  }
>
> -static inline bool is_vm_hugetlb_page(struct vm_area_struct *)
> +static inline bool is_vm_hugetlb_page(struct vm_area_struct *vma)
>  {
>  	return false;
>  }
>
> -static inline bool vma_soft_dirty_enabled(struct vm_area_struct *)
> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma)
>  {
>  	return false;
>  }
>
> -static inline bool userfaultfd_wp(struct vm_area_struct *)
> +static inline bool userfaultfd_wp(struct vm_area_struct *vma)
>  {
>  	return false;
>  }
>
> -static inline void mmap_assert_write_locked(struct mm_struct *)
> +static inline void mmap_assert_write_locked(struct mm_struct *mm)
>  {
>  }
>
> -static inline void mutex_lock(struct mutex *)
> +static inline void mutex_lock(struct mutex *lock)
>  {
>  }
>
> -static inline void mutex_unlock(struct mutex *)
> +static inline void mutex_unlock(struct mutex *lock)
>  {
>  }
>
> -static inline bool mutex_is_locked(struct mutex *)
> +static inline bool mutex_is_locked(struct mutex *lock)
>  {
>  	return true;
>  }
>
> -static inline bool signal_pending(void *)
> +static inline bool signal_pending(void *p)
>  {
>  	return false;
>  }
> --
> 2.34.1
>
>

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Lorenzo Stoakes]

On Fri, Oct 25, 2024 at 03:18:45AM +0000, Wei Yang wrote:
> On expansion failure, we try to restore vmg state, but we missed to
> restore vmi.index. The reason is we have reset vmg->vma before checking.
> So let's put the operation before reset vmg->vma.
>
> Also we don't need to do the restore if there is no mergeable adjacent
> VMA. Let's take it out to skip the unnecessary operations.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  mm/vma.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index fb4f1863f88e..c94d953d453c 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -954,23 +954,27 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
>  		vma_prev(vmg->vmi); /* Equivalent to going to the previous range */
>  	}
>
> +	/* No mergeable adjacent VMA, return */
> +	if (!vmg->vma)
> +		return NULL;
> +

Kind of a pet peeve of mine is throwing in a random refactoring thats not
mentioned in the commit message. Please don't do that.

I think it's fine as it is.

>  	/*
>  	 * Now try to expand adjacent VMA(s). This takes care of removing the
>  	 * following VMA if we have VMAs on both sides.
>  	 */
> -	if (vmg->vma && !vma_expand(vmg)) {
> +	if (!vma_expand(vmg)) {
>  		khugepaged_enter_vma(vmg->vma, vmg->flags);
>  		vmg->state = VMA_MERGE_SUCCESS;
>  		return vmg->vma;
>  	}
>
>  	/* If expansion failed, reset state. Allows us to retry merge later. */
> +	if (vmg->vma == prev)
> +		vma_iter_set(vmg->vmi, start);

Good spot in that we've stupidly been setting the vma NULL each time before
comparing... (doh and mea culpa!), but this actually accidentally proves we
don't need to bother resetting this at all :)

The only case where we care about a reset is mmap_region(), and there we reset
the iterator _anyway_.

>  	vmg->vma = NULL;
>  	vmg->start = start;
>  	vmg->end = end;
>  	vmg->pgoff = pgoff;
> -	if (vmg->vma == prev)
> -		vma_iter_set(vmg->vmi, start);

So please replace this whole series with a patch that just removes these
lines, thanks!

Also what tree are you making this change against? All mm changes should be
against akpm's tree in the mm-unstable branch. This change looks like it's
against another tree, as the code for this function has changed.

>
>  	return NULL;
>  }
> --
> 2.34.1
>
>

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Wei Yang]

On Fri, Oct 25, 2024 at 08:06:06AM +0100, Lorenzo Stoakes wrote:
>On Fri, Oct 25, 2024 at 03:18:45AM +0000, Wei Yang wrote:
>> On expansion failure, we try to restore vmg state, but we missed to
>> restore vmi.index. The reason is we have reset vmg->vma before checking.
>> So let's put the operation before reset vmg->vma.
>>
>> Also we don't need to do the restore if there is no mergeable adjacent
>> VMA. Let's take it out to skip the unnecessary operations.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> ---
>>  mm/vma.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/vma.c b/mm/vma.c
>> index fb4f1863f88e..c94d953d453c 100644
>> --- a/mm/vma.c
>> +++ b/mm/vma.c
>> @@ -954,23 +954,27 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
>>  		vma_prev(vmg->vmi); /* Equivalent to going to the previous range */
>>  	}
>>
>> +	/* No mergeable adjacent VMA, return */
>> +	if (!vmg->vma)
>> +		return NULL;
>> +
>
>Kind of a pet peeve of mine is throwing in a random refactoring thats not
>mentioned in the commit message. Please don't do that.
>
>I think it's fine as it is.
>
>>  	/*
>>  	 * Now try to expand adjacent VMA(s). This takes care of removing the
>>  	 * following VMA if we have VMAs on both sides.
>>  	 */
>> -	if (vmg->vma && !vma_expand(vmg)) {
>> +	if (!vma_expand(vmg)) {
>>  		khugepaged_enter_vma(vmg->vma, vmg->flags);
>>  		vmg->state = VMA_MERGE_SUCCESS;
>>  		return vmg->vma;
>>  	}
>>
>>  	/* If expansion failed, reset state. Allows us to retry merge later. */
>> +	if (vmg->vma == prev)
>> +		vma_iter_set(vmg->vmi, start);
>
>Good spot in that we've stupidly been setting the vma NULL each time before
>comparing... (doh and mea culpa!), but this actually accidentally proves we
>don't need to bother resetting this at all :)
>
>The only case where we care about a reset is mmap_region(), and there we reset
>the iterator _anyway_.
>
>>  	vmg->vma = NULL;
>>  	vmg->start = start;
>>  	vmg->end = end;
>>  	vmg->pgoff = pgoff;
>> -	if (vmg->vma == prev)
>> -		vma_iter_set(vmg->vmi, start);
>
>So please replace this whole series with a patch that just removes these
>lines, thanks!

You mean this?

--- a/mm/vma.c
+++ b/mm/vma.c
@@ -964,14 +964,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
                return vmg->vma;
        }
 
-       /* If expansion failed, reset state. Allows us to retry merge later. */
-       vmg->vma = NULL;
-       vmg->start = start;
-       vmg->end = end;
-       vmg->pgoff = pgoff;
-       if (vmg->vma == prev)
-               vma_iter_set(vmg->vmi, start);
-
        return NULL;
 }


>
>Also what tree are you making this change against? All mm changes should be
>against akpm's tree in the mm-unstable branch. This change looks like it's
>against another tree, as the code for this function has changed.
>

I use the upstream.

Will rebase to mm-unstable next.

>>
>>  	return NULL;
>>  }
>> --
>> 2.34.1
>>
>>

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Wei Yang]

On Fri, Oct 25, 2024 at 08:06:06AM +0100, Lorenzo Stoakes wrote:
>On Fri, Oct 25, 2024 at 03:18:45AM +0000, Wei Yang wrote:
>> On expansion failure, we try to restore vmg state, but we missed to
>> restore vmi.index. The reason is we have reset vmg->vma before checking.
>> So let's put the operation before reset vmg->vma.
>>
>> Also we don't need to do the restore if there is no mergeable adjacent
>> VMA. Let's take it out to skip the unnecessary operations.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> ---
>>  mm/vma.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/vma.c b/mm/vma.c
>> index fb4f1863f88e..c94d953d453c 100644
>> --- a/mm/vma.c
>> +++ b/mm/vma.c
>> @@ -954,23 +954,27 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
>>  		vma_prev(vmg->vmi); /* Equivalent to going to the previous range */
>>  	}
>>
>> +	/* No mergeable adjacent VMA, return */
>> +	if (!vmg->vma)
>> +		return NULL;
>> +
>
>Kind of a pet peeve of mine is throwing in a random refactoring thats not
>mentioned in the commit message. Please don't do that.
>
>I think it's fine as it is.
>
>>  	/*
>>  	 * Now try to expand adjacent VMA(s). This takes care of removing the
>>  	 * following VMA if we have VMAs on both sides.
>>  	 */
>> -	if (vmg->vma && !vma_expand(vmg)) {
>> +	if (!vma_expand(vmg)) {
>>  		khugepaged_enter_vma(vmg->vma, vmg->flags);
>>  		vmg->state = VMA_MERGE_SUCCESS;
>>  		return vmg->vma;
>>  	}
>>
>>  	/* If expansion failed, reset state. Allows us to retry merge later. */
>> +	if (vmg->vma == prev)
>> +		vma_iter_set(vmg->vmi, start);
>
>Good spot in that we've stupidly been setting the vma NULL each time before
>comparing... (doh and mea culpa!), but this actually accidentally proves we
>don't need to bother resetting this at all :)
>
>The only case where we care about a reset is mmap_region(), and there we reset
>the iterator _anyway_.
>
>>  	vmg->vma = NULL;
>>  	vmg->start = start;
>>  	vmg->end = end;
>>  	vmg->pgoff = pgoff;
>> -	if (vmg->vma == prev)
>> -		vma_iter_set(vmg->vmi, start);
>
>So please replace this whole series with a patch that just removes these
>lines, thanks!
>
>Also what tree are you making this change against? All mm changes should be
>against akpm's tree in the mm-unstable branch. This change looks like it's
>against another tree, as the code for this function has changed.
>

For mm-unstable, this is what you expect?

diff --git a/mm/vma.c b/mm/vma.c
index b5c1adcb6992..03b4838026ab 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -1003,16 +1003,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
                return vmg->vma;
        }
 
-       /* If expansion failed, reset state. Allows us to retry merge later. */
-       if (!just_expand) {
-               vmg->vma = NULL;
-               vmg->start = start;
-               vmg->end = end;
-               vmg->pgoff = pgoff;
-               if (vmg->vma == prev)
-                       vma_iter_set(vmg->vmi, start);
-       }
-
        return NULL;
 }


>>
>>  	return NULL;
>>  }
>> --
>> 2.34.1
>>
>>

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Lorenzo Stoakes]

On Fri, Oct 25, 2024 at 07:59:55AM +0000, Wei Yang wrote:
> On Fri, Oct 25, 2024 at 08:06:06AM +0100, Lorenzo Stoakes wrote:
> >>  	vmg->vma = NULL;
> >>  	vmg->start = start;
> >>  	vmg->end = end;
> >>  	vmg->pgoff = pgoff;
> >> -	if (vmg->vma == prev)
> >> -		vma_iter_set(vmg->vmi, start);
> >
> >So please replace this whole series with a patch that just removes these
> >lines, thanks!
> >
> >Also what tree are you making this change against? All mm changes should be
> >against akpm's tree in the mm-unstable branch. This change looks like it's
> >against another tree, as the code for this function has changed.
> >
>
> For mm-unstable, this is what you expect?
>
> diff --git a/mm/vma.c b/mm/vma.c
> index b5c1adcb6992..03b4838026ab 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -1003,16 +1003,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
>                 return vmg->vma;
>         }
>
> -       /* If expansion failed, reset state. Allows us to retry merge later. */
> -       if (!just_expand) {
> -               vmg->vma = NULL;
> -               vmg->start = start;
> -               vmg->end = end;
> -               vmg->pgoff = pgoff;
> -               if (vmg->vma == prev)
> -                       vma_iter_set(vmg->vmi, start);
> -       }
> -

Noooo! Sorry I wasn't clear :) We need this.

I mean:

 -               if (vmg->vma == prev)
 -                       vma_iter_set(vmg->vmi, start);

And with an explanation like:

	We incorrectly set vmg->vma = NULL before checking to see if we
	must reset the VMA iterator. However, since the only use case for
	this reset is mmap_region() and we always reset iterators there
	anyway, there is simply no need to do this.

	However, we absolutely do need to reset the vmg parameters to what
	they originally were, as well as resetting vmg->vma, as these may
	have been mutated to attempt a merge.

	There will be no change in behaviour, rather we simply avoid a
	pointless compare and, for cases where the VMA was the first in the
	mm, a pointless assignment to mas parameters.

>         return NULL;
>  }
>
>
> >>
> >>  	return NULL;
> >>  }
> >> --
> >> 2.34.1
> >>
> >>
>
> --
> Wei Yang
> Help you, Help me
>

I want to refactor this further, but only after recent mmap_region()
changes have settled down.

Thanks!

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Wei Yang]

On Fri, Oct 25, 2024 at 09:10:09AM +0100, Lorenzo Stoakes wrote:
>On Fri, Oct 25, 2024 at 07:59:55AM +0000, Wei Yang wrote:
>> On Fri, Oct 25, 2024 at 08:06:06AM +0100, Lorenzo Stoakes wrote:
>> >>  	vmg->vma = NULL;
>> >>  	vmg->start = start;
>> >>  	vmg->end = end;
>> >>  	vmg->pgoff = pgoff;
>> >> -	if (vmg->vma == prev)
>> >> -		vma_iter_set(vmg->vmi, start);
>> >
>> >So please replace this whole series with a patch that just removes these
>> >lines, thanks!
>> >
>> >Also what tree are you making this change against? All mm changes should be
>> >against akpm's tree in the mm-unstable branch. This change looks like it's
>> >against another tree, as the code for this function has changed.
>> >
>>
>> For mm-unstable, this is what you expect?
>>
>> diff --git a/mm/vma.c b/mm/vma.c
>> index b5c1adcb6992..03b4838026ab 100644
>> --- a/mm/vma.c
>> +++ b/mm/vma.c
>> @@ -1003,16 +1003,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
>>                 return vmg->vma;
>>         }
>>
>> -       /* If expansion failed, reset state. Allows us to retry merge later. */
>> -       if (!just_expand) {
>> -               vmg->vma = NULL;
>> -               vmg->start = start;
>> -               vmg->end = end;
>> -               vmg->pgoff = pgoff;
>> -               if (vmg->vma == prev)
>> -                       vma_iter_set(vmg->vmi, start);
>> -       }
>> -
>
>Noooo! Sorry I wasn't clear :) We need this.
>
>I mean:
>
> -               if (vmg->vma == prev)
> -                       vma_iter_set(vmg->vmi, start);
>
>And with an explanation like:
>
>	We incorrectly set vmg->vma = NULL before checking to see if we
>	must reset the VMA iterator. However, since the only use case for
>	this reset is mmap_region() and we always reset iterators there
>	anyway, there is simply no need to do this.
>
>	However, we absolutely do need to reset the vmg parameters to what
>	they originally were, as well as resetting vmg->vma, as these may
>	have been mutated to attempt a merge.
>
>	There will be no change in behaviour, rather we simply avoid a
>	pointless compare and, for cases where the VMA was the first in the
>	mm, a pointless assignment to mas parameters.
>

Ok, I get what you want.

But I have a question on your introduction of VMG_FLAG_JUST_EXPAND.

Lets say just_expand is true and can_merge_left is true. Now we will adjust
vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't
restore vmg->vma/start/pgoff, since just_expand is true.

Is this what you expect?

>>         return NULL;
>>  }
>>
>>
>> >>
>> >>  	return NULL;
>> >>  }
>> >> --
>> >> 2.34.1
>> >>
>> >>
>>
>> --
>> Wei Yang
>> Help you, Help me
>>
>
>I want to refactor this further, but only after recent mmap_region()
>changes have settled down.
>
>Thanks!

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Lorenzo Stoakes]

On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote:

>
> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND.
>
> Lets say just_expand is true and can_merge_left is true. Now we will adjust
> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't
> restore vmg->vma/start/pgoff, since just_expand is true.
>
> Is this what you expect?

Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't
realise :)

Actually at this point, I don't think we need a follow up patch, sorry.

As I think perhaps I will make this change as part of an existing series
where I am reworking mmap_region(), since this is the only place where it
matters, and it would make everything a hell of a lot clearer.

Thanks for pointing this out, it's very useful (and an embarrassing
oversight on my part...!), but I think it'd be better reworked this way.

Thanks!

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Wei Yang]

On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote:
>On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote:
>
>>
>> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND.
>>
>> Lets say just_expand is true and can_merge_left is true. Now we will adjust
>> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't
>> restore vmg->vma/start/pgoff, since just_expand is true.
>>
>> Is this what you expect?
>
>Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't
>realise :)
>
>Actually at this point, I don't think we need a follow up patch, sorry.
>
>As I think perhaps I will make this change as part of an existing series
>where I am reworking mmap_region(), since this is the only place where it
>matters, and it would make everything a hell of a lot clearer.
>
>Thanks for pointing this out, it's very useful (and an embarrassing
>oversight on my part...!), but I think it'd be better reworked this way.
>
>Thanks!

Ok, for now I would just remove these two lines with the change log you
suggested.

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Lorenzo Stoakes]

On Fri, Oct 25, 2024 at 08:49:19AM +0000, Wei Yang wrote:
> On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote:
> >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote:
> >
> >>
> >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND.
> >>
> >> Lets say just_expand is true and can_merge_left is true. Now we will adjust
> >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't
> >> restore vmg->vma/start/pgoff, since just_expand is true.
> >>
> >> Is this what you expect?
> >
> >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't
> >realise :)
> >
> >Actually at this point, I don't think we need a follow up patch, sorry.
> >
> >As I think perhaps I will make this change as part of an existing series
> >where I am reworking mmap_region(), since this is the only place where it
> >matters, and it would make everything a hell of a lot clearer.
> >
> >Thanks for pointing this out, it's very useful (and an embarrassing
> >oversight on my part...!), but I think it'd be better reworked this way.
> >
> >Thanks!
>
> Ok, for now I would just remove these two lines with the change log you
> suggested.

NO! Sorry I've not been clear - don't send any series.

I am going to make a change that eliminates the need for your change (sorry, but
in discussing this I've realised that's the best way forward).

>
> --
> Wei Yang
> Help you, Help me
>

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Wei Yang]

On Fri, Oct 25, 2024 at 09:51:12AM +0100, Lorenzo Stoakes wrote:
>On Fri, Oct 25, 2024 at 08:49:19AM +0000, Wei Yang wrote:
>> On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote:
>> >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote:
>> >
>> >>
>> >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND.
>> >>
>> >> Lets say just_expand is true and can_merge_left is true. Now we will adjust
>> >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't
>> >> restore vmg->vma/start/pgoff, since just_expand is true.
>> >>
>> >> Is this what you expect?
>> >
>> >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't
>> >realise :)
>> >
>> >Actually at this point, I don't think we need a follow up patch, sorry.
>> >
>> >As I think perhaps I will make this change as part of an existing series
>> >where I am reworking mmap_region(), since this is the only place where it
>> >matters, and it would make everything a hell of a lot clearer.
>> >
>> >Thanks for pointing this out, it's very useful (and an embarrassing
>> >oversight on my part...!), but I think it'd be better reworked this way.
>> >
>> >Thanks!
>>
>> Ok, for now I would just remove these two lines with the change log you
>> suggested.
>
>NO! Sorry I've not been clear - don't send any series.
>
>I am going to make a change that eliminates the need for your change (sorry, but
>in discussing this I've realised that's the best way forward).
>

Sure, go ahead.

>>
>> --
>> Wei Yang
>> Help you, Help me
>>

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Lorenzo Stoakes]

On Fri, Oct 25, 2024 at 08:54:09AM +0000, Wei Yang wrote:
> On Fri, Oct 25, 2024 at 09:51:12AM +0100, Lorenzo Stoakes wrote:
> >On Fri, Oct 25, 2024 at 08:49:19AM +0000, Wei Yang wrote:
> >> On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote:
> >> >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote:
> >> >
> >> >>
> >> >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND.
> >> >>
> >> >> Lets say just_expand is true and can_merge_left is true. Now we will adjust
> >> >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't
> >> >> restore vmg->vma/start/pgoff, since just_expand is true.
> >> >>
> >> >> Is this what you expect?
> >> >
> >> >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't
> >> >realise :)
> >> >
> >> >Actually at this point, I don't think we need a follow up patch, sorry.
> >> >
> >> >As I think perhaps I will make this change as part of an existing series
> >> >where I am reworking mmap_region(), since this is the only place where it
> >> >matters, and it would make everything a hell of a lot clearer.
> >> >
> >> >Thanks for pointing this out, it's very useful (and an embarrassing
> >> >oversight on my part...!), but I think it'd be better reworked this way.
> >> >
> >> >Thanks!
> >>
> >> Ok, for now I would just remove these two lines with the change log you
> >> suggested.
> >
> >NO! Sorry I've not been clear - don't send any series.
> >
> >I am going to make a change that eliminates the need for your change (sorry, but
> >in discussing this I've realised that's the best way forward).
> >
>
> Sure, go ahead.

Thanks, apologies I know it sucks, to be clear I very much appreciate your
input here!

For more detail - we are refactoring how the '2nd attempt' at a merge
works, also avoiding any abuse of vmg-> fields to keep track of start/end
of a range, so it becomes better to just open-code 'reset' of these fields
precisely at the point we need them.

Do please let us know if you notice any other silly mistakes like this :>)

Thanks, Lorenzo

>
> >>
> >> --
> >> Wei Yang
> >> Help you, Help me
> >>
>
> --
> Wei Yang
> Help you, Help me
>

Re: [PATCH 1/3] mm/vma: miss to restore vmi.index on expansion failure

[Wei Yang]

On Fri, Oct 25, 2024 at 10:01:36AM +0100, Lorenzo Stoakes wrote:
>On Fri, Oct 25, 2024 at 08:54:09AM +0000, Wei Yang wrote:
>> On Fri, Oct 25, 2024 at 09:51:12AM +0100, Lorenzo Stoakes wrote:
>> >On Fri, Oct 25, 2024 at 08:49:19AM +0000, Wei Yang wrote:
>> >> On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote:
>> >> >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote:
>> >> >
>> >> >>
>> >> >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND.
>> >> >>
>> >> >> Lets say just_expand is true and can_merge_left is true. Now we will adjust
>> >> >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't
>> >> >> restore vmg->vma/start/pgoff, since just_expand is true.
>> >> >>
>> >> >> Is this what you expect?
>> >> >
>> >> >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't
>> >> >realise :)
>> >> >
>> >> >Actually at this point, I don't think we need a follow up patch, sorry.
>> >> >
>> >> >As I think perhaps I will make this change as part of an existing series
>> >> >where I am reworking mmap_region(), since this is the only place where it
>> >> >matters, and it would make everything a hell of a lot clearer.
>> >> >
>> >> >Thanks for pointing this out, it's very useful (and an embarrassing
>> >> >oversight on my part...!), but I think it'd be better reworked this way.
>> >> >
>> >> >Thanks!
>> >>
>> >> Ok, for now I would just remove these two lines with the change log you
>> >> suggested.
>> >
>> >NO! Sorry I've not been clear - don't send any series.
>> >
>> >I am going to make a change that eliminates the need for your change (sorry, but
>> >in discussing this I've realised that's the best way forward).
>> >
>>
>> Sure, go ahead.
>
>Thanks, apologies I know it sucks, to be clear I very much appreciate your
>input here!
>

Ha, np. Glad to talk with you.

>For more detail - we are refactoring how the '2nd attempt' at a merge
>works, also avoiding any abuse of vmg-> fields to keep track of start/end
>of a range, so it becomes better to just open-code 'reset' of these fields
>precisely at the point we need them.
>

I am not sure we mention the same thing.

One confusion during code reading is on the vmg->start/end and
vmg->vmi->mas->index/last.

For many times I lost who is who...

>Do please let us know if you notice any other silly mistakes like this :>)
>

Let me clear my glasses :-)

>Thanks, Lorenzo
>
>>
>> >>
>> >> --
>> >> Wei Yang
>> >> Help you, Help me
>> >>
>>
>> --
>> Wei Yang
>> Help you, Help me
>>

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/3] mm/vma: miss to restore vmi.index on expansion failure

[Liam R. Howlett]

* Wei Yang <richard.weiyang@gmail.com> [241024 23:18]:
> This serial starts on spotting on miss to restore vmi.index in
> vma_merge_new_range() when expansion is failed.
> 
> Patch 2 add a test to assert this.
> 
> While found build error on old platform, because parameter name missing. Patch
> 3 is to fix this.

Just to make sure I understand; this series isn't happening now?

Thanks,
Liam

> 
> Wei Yang (3):
>   mm/vma: miss to restore vmi.index on expansion failure
>   tools: test vmi.index would be restored on merge failure
>   tools: fix build error on parameter name omitted
> 
>  mm/vma.c                         | 10 ++--
>  tools/testing/vma/vma.c          |  8 +++-
>  tools/testing/vma/vma_internal.h | 82 ++++++++++++++++----------------
>  3 files changed, 55 insertions(+), 45 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH 0/3] mm/vma: miss to restore vmi.index on expansion failure

[Lorenzo Stoakes]

On Fri, Oct 25, 2024 at 04:06:39PM -0400, Liam R. Howlett wrote:
> * Wei Yang <richard.weiyang@gmail.com> [241024 23:18]:
> > This serial starts on spotting on miss to restore vmi.index in
> > vma_merge_new_range() when expansion is failed.
> >
> > Patch 2 add a test to assert this.
> >
> > While found build error on old platform, because parameter name missing. Patch
> > 3 is to fix this.
>
> Just to make sure I understand; this series isn't happening now?

Nope.

I already folded the pertinent bit (me mistakenly doing a pointless compare and
mas->index/end write which later gets overwritten) into the mmap-region series
where we just get rid of this code altogether as it's nnnnnecessary and
horrible.

Great spot from Wei but I think this is the better approach.

>
> Thanks,
> Liam
>
> >
> > Wei Yang (3):
> >   mm/vma: miss to restore vmi.index on expansion failure
> >   tools: test vmi.index would be restored on merge failure
> >   tools: fix build error on parameter name omitted
> >
> >  mm/vma.c                         | 10 ++--
> >  tools/testing/vma/vma.c          |  8 +++-
> >  tools/testing/vma/vma_internal.h | 82 ++++++++++++++++----------------
> >  3 files changed, 55 insertions(+), 45 deletions(-)
> >
> > --
> > 2.34.1
> >
>