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

[Wei Yang <richard.weiyang@gmail.com>]

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