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:57PM GMT, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [240808 11:46]:
> > 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.
>
> No, we probably should not unite them - the shrink happens in a single
> place on setup.
>

Ack will in that case un-vmg vma_shrink().