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

Vlastimil Babka
2024-08-08T16:27:49Z

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

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