On Tue, Jan 28, 2025 at 12:38:48PM +0100, Vlastimil Babka wrote:
> 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.
Yes, and part of a number of steps that gradually improve things (though
some of the incremental states are not quite beautiful, the final result is
good :)
>
> > 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>
Thanks!
>
> > --- 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?
Yeah, I was thinking about doing this, but perhaps as a follow-up. We'd
have to differentiate between:
const struct foo *bar;
and
struct foo * const bar;
To indicate in some cases we are fine with changing the pointer but not the
underlying struct and vice-versa.
>
> > 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?
vma_merge_new_range() requires you to specify it, we document this there:
* ASSUMPTIONS:
...
* - The caller must have specified the next vma in @vmg->next.
In the case of vma_merge_existing_range() ,the caller should _not_ specify
next:
* ASSUMPTIONS:
* - The caller must not set @vmg->next, as we determine this.
Yes, this is insane, and I tried in the original series to avoid the need
for this stupid situation, but it ended up being more complicated than just
documenting and checking for this in the vma_merge_existing_range() case
(in the vma_merge_new_range() case we can't, since it may legitimately be
NULL if the proposed VMA is the last in the virtual address space).
Another one for a future fixup :)
>
>
