Re: [PATCH] mm/mmap: remove the mm parameter in vma_complete()

[Yajun Deng <yajun.deng@xxxxxxxxx>]

Adding Vlastimil and Lorenzo to discuss this patch.


On 2024/1/29 23:04, Liam R. Howlett wrote:
> * Yajun Deng <yajun.deng@xxxxxxxxx> [240129 02:53]:
> > There are vma_merge() and do_brk_flags() pass mm to vma_complete(), others
> > would pass the vma->vm_mm. The following explains that the mm is the
> > vma->vm_mm in vma_merge() and do_brk_flags().
> >
> > All vma will point to the same mm struct if the vma_merge() is successful.
> > So the mm and the vma->mm are the same.
> Absolutely, they must be the same.  I don't think vma_merge() checks
> this, but it is true.
>
> > vm_brk_flags() and brk syscall will initialize vmi with current->mm,
> > so the vma->vm_mm and the current->mm are the same if vma exists in
> > do_brk_flags().
> >
> > Remove the mm parameter in vma_complete() and get mm from the vma in vp.
> You have added a dereference to the two paths that don't need it to
> reduce the argument list from 3 to 2.  It's the same number of lines as
> well.  vma_shrink() is only used on process creation, but brk is more
> common.  Note that this function is marked as inline.
>
> I'm not sure this change is worth making.

If we can make sure the mm is  vma->vm_mm, I don't think we need to pass 
the mm.

If we can't make sure that, this change is not worth it.

> > Signed-off-by: Yajun Deng <yajun.deng@xxxxxxxxx>
> > ---
> >   mm/mmap.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index e97b9144c61a..9b968d1edf55 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -509,11 +509,11 @@ static inline void vma_prepare(struct vma_prepare *vp)
> >    *
> >    * @vp: The vma_prepare struct
> >    * @vmi: The vma iterator
> > - * @mm: The mm_struct
> >    */
> > -static inline void vma_complete(struct vma_prepare *vp,
> > -				struct vma_iterator *vmi, struct mm_struct *mm)
> > +static inline void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi)
> >   {
> > +	struct mm_struct *mm = vp->vma->vm_mm;
> > +
> >   	if (vp->file) {
> >   		if (vp->adj_next)
> >   			vma_interval_tree_insert(vp->adj_next,
> > @@ -666,7 +666,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >   	vma_set_range(vma, start, end, pgoff);
> >   	vma_iter_store(vmi, vma);
> >   
> > -	vma_complete(&vp, vmi, vma->vm_mm);
> > +	vma_complete(&vp, vmi);
> >   	return 0;
> >   
> >   nomem:
> > @@ -707,7 +707,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >   
> >   	vma_iter_clear(vmi);
> >   	vma_set_range(vma, start, end, pgoff);
> > -	vma_complete(&vp, vmi, vma->vm_mm);
> > +	vma_complete(&vp, vmi);
> >   	return 0;
> >   }
> >   
> > @@ -1030,7 +1030,7 @@ static struct vm_area_struct
> >   		}
> >   	}
> >   
> > -	vma_complete(&vp, vmi, mm);
> > +	vma_complete(&vp, vmi);
> >   	khugepaged_enter_vma(res, vm_flags);
> >   	return res;
> >   
> > @@ -2377,7 +2377,7 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >   	}
> >   
> >   	/* vma_complete stores the new vma */
> > -	vma_complete(&vp, vmi, vma->vm_mm);
> > +	vma_complete(&vp, vmi);
> >   
> >   	/* Success. */
> >   	if (new_below)
> > @@ -3145,7 +3145,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >   		vm_flags_set(vma, VM_SOFTDIRTY);
> >   		vma_iter_store(vmi, vma);
> >   
> > -		vma_complete(&vp, vmi, mm);
> > +		vma_complete(&vp, vmi);
> >   		khugepaged_enter_vma(vma, flags);
> >   		goto out;
> >   	}
> > --
> > 2.25.1