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

[Vlastimil Babka]

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@xxxxxxxxxx>

In general,

Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

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...

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

> +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 :)

> +	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.

>  
>  	/* 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