Re: [PATCH 04/10] mm: abstract parameters for vma_expand/shrink()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Aug 08, 2024 at 04:20:26PM GMT, Vlastimil Babka (SUSE) wrote:
> On 8/5/24 14:13, Lorenzo Stoakes wrote:
> > Equally use struct vma_merge_struct to abstract parameters for VMA
> > expansion and shrinking.
> >
> > This leads the way to further refactoring and de-duplication by
> > standardising the interface.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  mm/mmap.c               | 30 +++++++++++--------
> >  mm/vma.c                | 66 ++++++++++++++++++-----------------------
> >  mm/vma.h                |  8 ++---
> >  tools/testing/vma/vma.c | 18 +++++++++--
> >  4 files changed, 65 insertions(+), 57 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 721ced6e37b0..04145347c245 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1367,7 +1367,6 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  	pgoff_t pglen = len >> PAGE_SHIFT;
> >  	unsigned long charged = 0;
> >  	unsigned long end = addr + len;
> > -	unsigned long merge_start = addr, merge_end = end;
> >  	bool writable_file_mapping = false;
> >  	int error;
> >  	VMA_ITERATOR(vmi, mm, addr);
> > @@ -1423,28 +1422,26 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  	/* Attempt to expand an old mapping */
> >  	/* Check next */
> >  	if (next && next->vm_start == end && can_vma_merge_before(&vmg)) {
> > -		merge_end = next->vm_end;
> > -		vma = next;
> > +		/* We can adjust this as can_vma_merge_after() doesn't touch */
> > +		vmg.end = next->vm_end;
>
> Ugh, ok but wonder how fragile that is.

Yeah you're right this is a bit horrid, I'll find a way to make this less
brittle.

>
> > +		vma = vmg.vma = next;
> >  		vmg.pgoff = next->vm_pgoff - pglen;
> > -	}
> >
> > -	if (vma) {
> > +		/* We may merge our NULL anon_vma with non-NULL in next. */
>
> Hm now I realize the if (vma) block probably didn't need to be added in
> patch 2 only to removed here, it could have been part of the if (next &&
> ...) block above already? Which is not that important, but...

You're right, will fix.

>
> >  		vmg.anon_vma = vma->anon_vma;
> > -		vmg.uffd_ctx = vma->vm_userfaultfd_ctx;
>
> I don't see why it's now ok to remove this line? Was it intended? In patch 2
> it made sense to me to add it so the can_vma_merge_after() still has the
> right ctx for comparing, and this didn't change?

Yeah, yikes, I think I was lost in the maelstrom of considering edge cases,
and now this is broken for the whole prev vs. next uffd thing.

The fact the mmap stuff is not directly testable is a factor here.

TL;DR: I'll fix this, you're right.

>
> >  	}
> >
> >  	/* Check prev */
> >  	if (prev && prev->vm_end == addr && can_vma_merge_after(&vmg)) {
> > -		merge_start = prev->vm_start;
> > -		vma = prev;
> > +		vmg.start = prev->vm_start;
> > +		vma = vmg.vma = prev;
> >  		vmg.pgoff = prev->vm_pgoff;
> >  	} else if (prev) {
> >  		vma_iter_next_range(&vmi);
> >  	}
> >
> >  	/* Actually expand, if possible */
> > -	if (vma &&
> > -	    !vma_expand(&vmi, vma, merge_start, merge_end, vmg.pgoff, next)) {
> > +	if (vma && !vma_expand(&vmg)) {
> >  		khugepaged_enter_vma(vma, vm_flags);
> >  		goto expanded;
> >  	}
> > @@ -2359,6 +2356,13 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
> >  	VMA_ITERATOR(vmi, mm, new_start);
> >  	struct vm_area_struct *next;
> >  	struct mmu_gather tlb;
> > +	struct vma_merge_struct vmg = {
> > +		.vmi = &vmi,
> > +		.vma = vma,
> > +		.start = new_start,
> > +		.end = old_end,
> > +		.pgoff = vma->vm_pgoff,
> > +	};
> >
> >  	BUG_ON(new_start > new_end);
> >
> > @@ -2373,7 +2377,7 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
> >  	/*
> >  	 * cover the whole range: [new_start, old_end)
> >  	 */
> > -	if (vma_expand(&vmi, vma, new_start, old_end, vma->vm_pgoff, NULL))
> > +	if (vma_expand(&vmg))
> >  		return -ENOMEM;
> >
> >  	/*
> > @@ -2406,6 +2410,8 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
> >  	tlb_finish_mmu(&tlb);
> >
> >  	vma_prev(&vmi);
> > +	vmg.end = new_end;
> > +
> >  	/* Shrink the vma to just the new range */
> > -	return vma_shrink(&vmi, vma, new_start, new_end, vma->vm_pgoff);
> > +	return vma_shrink(&vmg);
>
> The vma_shrink() doesn't seem to benefit that much from vmg conversion but I
> guess why not. Maybe this will further change anyway...
>

No it doesn't, but it's more about being consistent with vma_expand(). We
maybe want to find a way to unite them possibly.