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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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