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