On Fri, Aug 09, 2024 at 12:15:24PM GMT, Vlastimil Babka wrote:
> On 8/5/24 14:13, Lorenzo Stoakes wrote:
> > Pull this operation into its own function and have vma_expand() call
> > commit_merge() instead.
> >
> > This lays the groundwork for a subsequent patch which replaces vma_merge()
> > with a simpler function which can share the same code.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
>
> In general,
>
> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
>
> If you consider the following suggestions, great:
>
> > ---
> > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 45 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index a404cf718f9e..b7e3c64d5d68 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm)
> > }
> > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */
> >
> > +/* Actually perform the VMA merge operation. */
> > +static int commit_merge(struct vma_merge_struct *vmg,
> > + struct vm_area_struct *adjust,
> > + struct vm_area_struct *remove,
> > + struct vm_area_struct *remove2,
> > + long adj_start,
> > + bool expanded)
>
> I've read the subthread with Petr. I understand it's hard to organize such
> big changes in self-contained units. But maybe it would still be possible to
> introduce this function now without the parameters, and as part of the the
> next patch add the two parameters and the code using them. Maybe it would
> even make git detect the added code as code move from where it's now, so it
> would be more obviousl.
Since both you and Petr make the same point (sorry Petr, I should have
perhaps been a little less resistant to this), I will do this.
As discussed on IRC my position on this is that we're introducing a _really
fundamental_ and important bit of the logic here, intentionally broken out
as a separate commit, and this is why I preferred to introduce it 'fully
formed'.
This function is absolutely fundamental to eliminating the duplication in
do_brk_flags() + mmap_region() and to maintain two separate new/modified
vma versions of vma_merge().
HOWEVER, I totally accept that this makes review much more of a pain in the
arse, and in practice almost certainly the only thing that matters is
reviewability here as to how I structure this.
So TL;DR: I'll do what you both ask and introduce new params only when we
use them.
>
> > +{
> > + struct vma_prepare vp;
> > +
> > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2);
> > +
> > + if (expanded) {
> > + vma_iter_config(vmg->vmi, vmg->start, vmg->end);
>
> This originally had a comment
>
> /* Note: vma iterator must be pointing to 'start' */
>
> and now it's gone.
Will check and re-add if it makes sense. I mean we're now setting the iterator
to start anyway so I don't see that this has value? Maybe I'm missing something
and Liam has thoughts...
>
> > + } else {
> > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start,
> > + adjust->vm_end);
>
> And this less obvious one has none either :(
I will add a comment.
>
> > + }
> > +
> > + if (vma_iter_prealloc(vmg->vmi, vmg->vma))
> > + return -ENOMEM;
> > +
> > + vma_prepare(&vp);
> > + vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start);
> > + vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff);
> > +
> > + if (expanded)
> > + vma_iter_store(vmg->vmi, vmg->vma);
> > +
> > + if (adj_start) {
> > + adjust->vm_start += adj_start;
> > + adjust->vm_pgoff += PHYS_PFN(adj_start);
> > + if (adj_start < 0) {
> > + WARN_ON(expanded);
> > + vma_iter_store(vmg->vmi, adjust);
> > + }
> > + }
> > +
> > + vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm);
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * vma_merge_new_vma - Attempt to merge a new VMA into address space
> > *
> > @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg)
> > bool remove_next = false;
> > struct vm_area_struct *vma = vmg->vma;
> > struct vm_area_struct *next = vmg->next;
> > - struct vma_prepare vp;
> >
> > vma_start_write(vma);
> > if (next && (vma != next) && (vmg->end == next->vm_end)) {
> > @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg)
> > return ret;
> > }
> >
> > - init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL);
> > /* Not merging but overwriting any part of next is not handled. */
> > - VM_WARN_ON(next && !vp.remove &&
> > + VM_WARN_ON(next && !remove_next &&
> > next != vma && vmg->end > next->vm_start);
> > /* Only handles expanding */
> > VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end);
> >
> > - /* Note: vma iterator must be pointing to 'start' */
> > - vma_iter_config(vmg->vmi, vmg->start, vmg->end);
> > - if (vma_iter_prealloc(vmg->vmi, vma))
> > + if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true))
> > goto nomem;
> >
> > - vma_prepare(&vp);
> > - vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0);
> > - vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff);
> > - vma_iter_store(vmg->vmi, vma);
> > -
> > - vma_complete(&vp, vmg->vmi, vma->vm_mm);
> > return 0;
> >
> > nomem:
>
