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
