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