Re: [PATCH 02/10] mm: introduce vma_merge_struct and abstract merge parameters

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

On Thu, Aug 08, 2024 at 02:49:03PM GMT, Vlastimil Babka wrote:
> On 8/5/24 14:13, Lorenzo Stoakes wrote:
> > Rather than passing around huge numbers of parameters to numerous helper
> > functions, abstract them into a single struct that we thread through the
> > operation.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> In general,
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>
> It would be great to have the tests already at this point but I understand
> they depend on this. At least the result can be tested later in the series...

Yeah it felt like too much churn (even for me ;) to do a before vmg/after
vmg version, but I could also still do this. At that point it might be
worth adding benchmarks too to assess impact...

>
> I haven't seen the final form yet, so some suggestions may become moot.

Umm...

>
> > +static struct vm_area_struct *vma_merge(struct vma_merge_struct *vmg)
> >  {
> > -	struct mm_struct *mm = src->vm_mm;
> > -	struct anon_vma *anon_vma = src->anon_vma;
> > -	struct file *file = src->vm_file;
> > +	struct mm_struct *mm = container_of(vmg->vmi->mas.tree, struct mm_struct, mm_mt);
>
> This feels weird, but I'll leave it to Liam. Can't we just pass the mm? Hope
> it's one of the things that will disappear in later patch :)

Yeah it is weird, I felt dirty and ashamed writing this so fully understand
if Liam wouldn't like.

Previously we'd actually dictate the need for a vma here, but that made it
trickier to write the tests I think.

Anyway we maybe just want to thread an mm?

>
> > +	struct vm_area_struct *prev = vmg->prev;
> >  	struct vm_area_struct *curr, *next, *res;
> >  	struct vm_area_struct *vma, *adjust, *remove, *remove2;
> >  	struct vm_area_struct *anon_dup = NULL;
>
> <snip>
>
> > +/* Assumes addr >= vma->vm_start. */
> > +static pgoff_t vma_pgoff_offset(struct vm_area_struct *vma, unsigned long addr)
> > +{
> > +	return vma->vm_pgoff + PHYS_PFN(addr - vma->vm_start);
> > +}
> > +
> > +struct vm_area_struct *vma_modify_flags(struct vma_iterator *vmi,
> > +					struct vm_area_struct *prev,
> > +					struct vm_area_struct *vma,
> > +					unsigned long start, unsigned long end,
> > +					unsigned long new_flags)
> > +{
> > +	struct vma_merge_struct vmg = {
> > +		.vmi = vmi,
> > +		.prev = prev,
> > +		.vma = vma,
> > +		.start = start,
> > +		.end = end,
> > +		.flags = new_flags,
> > +		.pgoff = vma_pgoff_offset(vma, start),
> > +		.file = vma->vm_file,
> > +		.anon_vma = vma->anon_vma,
> > +		.policy = vma_policy(vma),
> > +		.uffd_ctx = vma->vm_userfaultfd_ctx,
> > +		.anon_name = anon_vma_name(vma),
> > +	};
> > +
> > +	return vma_modify(&vmg);
> > +}
> > +
> > +struct vm_area_struct
> > +*vma_modify_flags_name(struct vma_iterator *vmi,
> > +		       struct vm_area_struct *prev,
> > +		       struct vm_area_struct *vma,
> > +		       unsigned long start,
> > +		       unsigned long end,
> > +		       unsigned long new_flags,
> > +		       struct anon_vma_name *new_name)
> > +{
> > +	struct vma_merge_struct vmg = {
> > +		.vmi = vmi,
> > +		.prev = prev,
> > +		.vma = vma,
> > +		.start = start,
> > +		.end = end,
> > +		.flags = new_flags,
> > +		.pgoff = vma_pgoff_offset(vma, start),
> > +		.file = vma->vm_file,
> > +		.anon_vma = vma->anon_vma,
> > +		.policy = vma_policy(vma),
> > +		.uffd_ctx = vma->vm_userfaultfd_ctx,
> > +		.anon_name = new_name,
> > +	};
> > +
> > +	return vma_modify(&vmg);
> > +}
> > +
> > +struct vm_area_struct
> > +*vma_modify_policy(struct vma_iterator *vmi,
> > +		   struct vm_area_struct *prev,
> > +		   struct vm_area_struct *vma,
> > +		   unsigned long start, unsigned long end,
> > +		   struct mempolicy *new_pol)
> > +{
> > +	struct vma_merge_struct vmg = {
> > +		.vmi = vmi,
> > +		.prev = prev,
> > +		.vma = vma,
> > +		.start = start,
> > +		.end = end,
> > +		.flags = vma->vm_flags,
> > +		.pgoff = vma_pgoff_offset(vma, start),
> > +		.file = vma->vm_file,
> > +		.anon_vma = vma->anon_vma,
> > +		.policy = new_pol,
> > +		.uffd_ctx = vma->vm_userfaultfd_ctx,
> > +		.anon_name = anon_vma_name(vma),
> > +	};
> > +
> > +	return vma_modify(&vmg);
> > +}
> > +
> > +struct vm_area_struct
> > +*vma_modify_flags_uffd(struct vma_iterator *vmi,
> > +		       struct vm_area_struct *prev,
> > +		       struct vm_area_struct *vma,
> > +		       unsigned long start, unsigned long end,
> > +		       unsigned long new_flags,
> > +		       struct vm_userfaultfd_ctx new_ctx)
> > +{
> > +	struct vma_merge_struct vmg = {
> > +		.vmi = vmi,
> > +		.prev = prev,
> > +		.vma = vma,
> > +		.start = start,
> > +		.end = end,
> > +		.flags = new_flags,
> > +		.file = vma->vm_file,
> > +		.anon_vma = vma->anon_vma,
> > +		.pgoff = vma_pgoff_offset(vma, start),
> > +		.policy = vma_policy(vma),
> > +		.uffd_ctx = new_ctx,
> > +		.anon_name = anon_vma_name(vma),
> > +	};
> > +
> > +	return vma_modify(&vmg);
> >  }
> >
> >  /*
> > @@ -1180,8 +1274,22 @@ struct vm_area_struct
> >  		   struct vm_area_struct *vma, unsigned long start,
> >  		   unsigned long end, pgoff_t pgoff)
> >  {
> > -	return vma_merge(vmi, prev, vma, start, end, vma->vm_flags, pgoff,
> > -			 vma_policy(vma), vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > +	struct vma_merge_struct vmg = {
> > +		.vmi = vmi,
> > +		.prev = prev,
> > +		.vma = vma,
> > +		.start = start,
> > +		.end = end,
> > +		.flags = vma->vm_flags,
> > +		.file = vma->vm_file,
> > +		.anon_vma = vma->anon_vma,
> > +		.pgoff = pgoff,
> > +		.policy = vma_policy(vma),
> > +		.uffd_ctx = vma->vm_userfaultfd_ctx,
> > +		.anon_name = anon_vma_name(vma),
> > +	};
> > +
> > +	return vma_merge(&vmg);
> >  }
> >
> >  /*
> > @@ -1193,11 +1301,22 @@ struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi,
> >  					unsigned long delta)
> >  {
> >  	pgoff_t pgoff = vma->vm_pgoff + vma_pages(vma);
> > +	struct vma_merge_struct vmg = {
> > +		.vmi = vmi,
> > +		.prev = vma,
> > +		.vma = vma,
> > +		.start = vma->vm_end,
> > +		.end = vma->vm_end + delta,
> > +		.flags = vma->vm_flags,
> > +		.file = vma->vm_file,
> > +		.pgoff = pgoff,
> > +		.policy = vma_policy(vma),
> > +		.uffd_ctx = vma->vm_userfaultfd_ctx,
> > +		.anon_name = anon_vma_name(vma),
> > +	};
>
> Wonder if there's a way to initialize a "standard" vmg and then apply the
> usage-specific differences on top, instead of needing many repeated but
> subtly different blocks like above.

Yes that'd be nice, I had the same thought but just hadn't got round to
doing it yet.

Will look at it on next respin.

>
> >
> >  	/* vma is specified as prev, so case 1 or 2 will apply. */
> > -	return vma_merge(vmi, vma, vma, vma->vm_end, vma->vm_end + delta,
> > -			 vma->vm_flags, pgoff, vma_policy(vma),
> > -			 vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > +	return vma_merge(&vmg);
> >  }
> >
> >  void unlink_file_vma_batch_init(struct unlink_vma_file_batch *vb)
> > diff --git a/mm/vma.h b/mm/vma.h
> > index 6efdf1768a0a..c31684cc1da6 100644
>