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