Re: [PATCH 1/5] mm: simplify vma merge structure and expand comments

[Vlastimil Babka]

On 1/27/25 16:50, Lorenzo Stoakes wrote:
> The merge code, while much improved, still has a number of points of
> confusion. As part of a broader series cleaning this up to make this more
> maintainable, we start by addressing some confusion around vma_merge_struct
> fields.
> 
> So far, the caller either provides no vmg->vma (a new VMA) or supplies the
> existing VMA which is being altered, setting vmg->start,end,pgoff to the
> proposed VMA dimensions.
> 
> vmg->vma is then updated, as are vmg->start,end,pgoff as the merge process
> proceeds and the appropriate merge strategy is determined.
> 
> This is rather confusing, as vmg->vma starts off as the 'middle' VMA
> between vmg->prev,next, but becomes the 'target' VMA, except in one
> specific edge case (merge next, shrink middle).
> 
> Int his patch we introduce vmg->middle to describe the VMA that is between
> vmg->prev and vmg->next, and does NOT change during the merge operation.
> 
> We replace vmg->vma with vmg->target, and use this only during the merge
> operation itself.

Yeah that's much better.

> Aside from the merge right, shrink middle case, this becomes the VMA that
> forms the basis of the VMA that is returned. This edge case can be
> addressed in a future commit.
> 
> We also add a number of comments to explain what is going on.
> 
> Finally, we adjust the ASCII diagrams showing each merge case in
> vma_merge_existing_range() to be clearer - the arrow range previously
> showed the vmg->start, end spanned area, but it is clearer to change this
> to show the final merged VMA.
> 
> This patch has no change in functional behaviour.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>

Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>

> --- a/mm/vma.h
> +++ b/mm/vma.h
> @@ -69,16 +69,48 @@ enum vma_merge_flags {
>  	VMG_FLAG_JUST_EXPAND = 1 << 0,
>  };
>  
> -/* Represents a VMA merge operation. */
> +/*
> + * Describes a VMA merge operation and is threaded throughout it.
> + *
> + * Any of the fields may be mutated by the merge operation, so no guarantees are
> + * made to the contents of this structure after a merge operation has completed.
> + */

Well this patch seems like a step in the direction to limit what's mutated,
and perhaps defining some of the guarantees (via const?) could be then possible?

>  struct vma_merge_struct {
>  	struct mm_struct *mm;
>  	struct vma_iterator *vmi;
> -	pgoff_t pgoff;
> +	/*
> +	 * Adjacent VMAs, any of which may be NULL if not present:
> +	 *
> +	 * |------|--------|------|
> +	 * | prev | middle | next |
> +	 * |------|--------|------|
> +	 *
> +	 * middle may not yet exist in the case of a proposed new VMA being
> +	 * merged, or it may be an existing VMA.
> +	 *
> +	 * next may be assigned by the caller.

Caller of what?