Re: [PATCH 5/5] mm: completely abstract unnecessary adj_start calculation

[Vlastimil Babka <vbabka@suse.cz>]

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@oracle.com>

Yeah this makes the preparations worth it. Nice!

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> +/*
> + * 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 {