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@xxxxxxxxx>
>> CC: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
>> ---
>> 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
