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?
