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
