On 1/27/25 16:50, Lorenzo Stoakes wrote:
> The adj_start calculation has been a constant source of confusion in the
> VMA merge code.
>
> There are two cases to consider, one where we adjust the start of the
> vmg->middle VMA (i.e. the __VMG_FLAG_ADJUST_MIDDLE_START merge flag is
> set), in which case adj_start is calculated as:
>
> (1) adj_start = vmg->end - vmg->middle->vm_start
>
> And the case where we adjust the start of the vmg->next VMA (i.e.t he
> __VMG_FLAG_ADJUST_NEXT_START merge flag is set), in which case adj_start is
> calculated as:
>
> (2) adj_start = -(vmg->middle->vm_end - vmg->end)
>
> We apply (1) thusly:
>
> vmg->middle->vm_start =
> vmg->middle->vm_start + vmg->end - vmg->middle->vm_start
>
> Which simplifies to:
>
> vmg->middle->vm_start = vmg->end
>
> Similarly, we apply (2) as:
>
> vmg->next->vm_start =
> vmg->next->vm_start + -(vmg->middle->vm_end - vmg->end)
>
> Noting that for these VMAs to be mergeable vmg->middle->vm_end ==
> vmg->next->vm_start and so this simplifies to:
>
> vmg->next->vm_start =
> vmg->next->vm_start + -(vmg->next->vm_start - vmg->end)
>
> Which simplifies to:
>
> vmg->next->vm_start = vmg->end
>
> Therefore in each case, we simply need to adjust the start of the VMA to
> vmg->end (!) and can do away with this adj_start calculation. The only
> caveat is that we must ensure we update the vm_pgoff field correctly.
>
> We therefore abstract this entire calculation to a new function
> vmg_adjust_set_range() which performs this calculation and sets the
> adjusted VMA's new range using the general vma_set_range() function.
>
> We also must update vma_adjust_trans_huge() which expects the
> now-abstracted adj_start parameter. It turns out this is wholly
> unnecessary.
>
> In vma_adjust_trans_huge() the relevant code is:
>
> if (adjust_next > 0) {
> struct vm_area_struct *next = find_vma(vma->vm_mm, vma->vm_end);
> unsigned long nstart = next->vm_start;
> nstart += adjust_next;
> split_huge_pmd_if_needed(next, nstart);
> }
>
> The only case where this is relevant is when __VMG_FLAG_ADJUST_MIDDLE_START
> is specified (in which case adj_next would have been positive), i.e. the
> one in which the vma specified is vmg->prev and this the sought 'next' VMA
> would be vmg->middle.
>
> We can therefore eliminate the find_vma() invocation altogether and simply
> provide the vmg->middle VMA in this instance, or NULL otherwise.
>
> Again we have an adj_next offset calculation:
>
> next->vm_start + vmg->end - vmg->middle->vm_start
>
> Where next == vmg->middle this simplifies to vmg->end as previously
> demonstrated.
>
> Therefore nstart is equal to vmg->end, which is already passed to
> vma_adjust_trans_huge() via the 'end' parameter and so this code (rather
> delightfully) simplifies to:
>
> if (next)
> split_huge_pmd_if_needed(next, end);
>
> With these changes in place, it becomes silly for commit_merge() to return
> vmg->target, as it is always the same and threaded through vmg, so we
> finally change commit_merge() to return an error value once again.
>
> This patch has no change in functional behaviour.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
Yeah this makes the preparations worth it. Nice!
Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>
> +/*
> + * Actually perform the VMA merge operation.
> + *
> + * On success, returns the merged VMA. Otherwise returns NULL.
Needs updating?
> + */
> +static int commit_merge(struct vma_merge_struct *vmg)
> +{
> + struct vm_area_struct *vma;
> + struct vma_prepare vp;
> + bool adj_middle = vmg->merge_flags & __VMG_FLAG_ADJUST_MIDDLE_START;
>
> - vma_iter_config(vmg->vmi, vmg->next->vm_start + adj_start,
> - vmg->next->vm_end);
> + if (vmg->merge_flags & __VMG_FLAG_ADJUST_NEXT_START) {
> + /* In this case we manipulate middle and return next. */
Also we don't return next anymore? At least not here.
vma_merge_existing_range() does, but here it's rather "the target is next"?
> + vma = vmg->middle;
> + vma_iter_config(vmg->vmi, vmg->end, vmg->next->vm_end);
> } else {
