On Mon, Nov 11, 2024 at 01:23:40AM +0000, Wei Yang wrote:
> vma_expand() is called by relocate_vma_down() and vma_merge_new_range()
> with only vma_merge_new_range() has a chance to remove 'next'.
>
> By leveraging the knowledge in vma_merge_new_range(), only set vmg->next
> when removal is necessary, we can simplify the logic in vma_expand().
>
> Originally we have an assumption that VMG state could be safely reused
> after a merge. This assumption is removed after commit 5a689bac0bbc
> ("mm: remove unnecessary reset state logic on merge new VMA"). So we are
> safe to clear it.
While this is clever, I'm sorry but it's a NACK (I'll explain why!).
The intent of this code isn't to try to find the mathematically smallest
possible configuration of code, the whole point of the refactoring is to
make the code easier to follow and more flexible and obviously correct (or
incorrect, if an error is made).
Mutating unexpected state midway through an operation in order to remove
some code elsewhere is doing the opposite of that.
We may not now rely on being able to reuse state (huge relief that we
don't), but that isn't an open door to altering state like this.
>
> Signed-off-by: Wei Yang <richard.weiyang@xxxxxxxxx>
> CC: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> CC: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> CC: Vlastimil Babka <vbabka@xxxxxxx>
> CC: Jann Horn <jannh@xxxxxxxxxx>
> ---
> mm/vma.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index 8a454a7bbc80..85d82bc1eaed 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -984,6 +984,7 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
> can_merge_left = can_vma_merge_left(vmg);
> can_merge_right = !just_expand && can_vma_merge_right(vmg, can_merge_left);
>
> + vmg->next = NULL;
I mean from here on in we're super open to very easily written bugs which
assume that this value is what we expect it to be...
It's also confusing to a casual reader not knowing that we _require_ the
caller to set next, which we now just throw away.
The original vma_merge() implementation had a whole bunch of implicit
assumptions like this, the intent of this code is to try to get away from
this.
> /* If we can merge with the next VMA, adjust vmg accordingly. */
> if (can_merge_right) {
> vmg->end = next->vm_end;
> @@ -1001,8 +1002,12 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
> * are not permitted to do so, reduce the operation to merging
> * prev and vma.
> */
> - if (can_merge_right && !can_merge_remove_vma(next))
> - vmg->end = end;
> + if (can_merge_right) {
> + if (!can_merge_remove_vma(next))
> + vmg->end = end;
> + else
> + vmg->next = next;
> + }
>
This is confusing and now vmg->next ha a completely different meaning than
one might expect it's not the next VMA it's 'the next VMA that we can merge
with, but only after the merge operation has started'.
This is again working counter to the intent of this change in the first place.
> /* In expand-only case we are already positioned at prev. */
> if (!just_expand) {
> @@ -1030,9 +1035,9 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
> * @vmg: Describes a VMA expansion operation.
> *
> * Expand @vma to vmg->start and vmg->end. Can expand off the start and end.
> - * Will expand over vmg->next if it's different from vmg->vma and vmg->end ==
> - * vmg->next->vm_end. Checking if the vmg->vma can expand and merge with
> - * vmg->next needs to be handled by the caller.
> + * Will expand over vmg->next if it's set.
> + * Checking if the vmg->vma can expand and merge with vmg->next needs to be
> + * handled by the caller.
> *
> * Returns: 0 on success.
> *
> @@ -1043,17 +1048,15 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
> int vma_expand(struct vma_merge_struct *vmg)
> {
> struct vm_area_struct *anon_dup = NULL;
> - bool remove_next = false;
> struct vm_area_struct *vma = vmg->vma;
> struct vm_area_struct *next = vmg->next;
>
> mmap_assert_write_locked(vmg->mm);
>
> vma_start_write(vma);
> - if (next && (vma != next) && (vmg->end == next->vm_end)) {
> + if (next) {
> int ret;
Again simply reading this code you'd be confused as to what is going on
here. Previously this was plain.
>
> - remove_next = true;
> /* This should already have been checked by this point. */
> VM_WARN_ON(!can_merge_remove_vma(next));
> vma_start_write(next);
> @@ -1062,13 +1065,10 @@ int vma_expand(struct vma_merge_struct *vmg)
> return ret;
> }
>
> - /* Not merging but overwriting any part of next is not handled. */
> - VM_WARN_ON(next && !remove_next &&
> - next != vma && vmg->end > next->vm_start);
You'd want to leave this in minus the remove_next bit. But this is moot :)
> /* Only handles expanding */
> VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end);
>
> - if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true))
> + if (commit_merge(vmg, NULL, next, NULL, 0, true))
> goto nomem;
>
> return 0;
> --
> 2.34.1
>
>
This does inspire me to make some changes though so your concept here is
definitely not completely off-base, but I am not a fan of how you've done
this.
If I do go ahead and make any change inspired by this I will _of course_
give you Suggested-by credit!
I'd suggest focusing on finding bugs and logic errors rather than
refactorings like this unless the refactoring is obviously beneficial.
I appreciate your input and efforts here though, however! :)
Thanks, Lorenzo
