[v2,4/5] mm: abstract merge for new VMAs into vma_merge_new_vma()

Commit Message

Lorenzo Stoakes Oct. 9, 2023, 8:53 p.m. UTC

Only in mmap_region() and copy_vma() do we attempt to merge VMAs which
occupy entirely new regions of virtual memory.

We can abstract this logic and make the intent of this invocations of it
completely explicit, rather than invoking vma_merge() with an inscrutable
wall of parameters.

This also paves the way for a simplification of the core vma_merge()
implementation, as we seek to make it entirely an implementation detail.

Note that on mmap_region(), VMA fields are initialised to zero, so we can
simply reference these rather than explicitly specifying NULL.

Reviewed-by: Vlastimil Babka <[e-mail]>
Signed-off-by: Lorenzo Stoakes <[e-mail]>
---
 mm/mmap.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

Comments

Liam R. Howlett Oct. 11, 2023, 1:51 a.m. UTC | #1

* Lorenzo Stoakes <[e-mail]> [231009 16:53]:
> Only in mmap_region() and copy_vma() do we attempt to merge VMAs which
> occupy entirely new regions of virtual memory.
> 
> We can abstract this logic and make the intent of this invocations of it
> completely explicit, rather than invoking vma_merge() with an inscrutable
> wall of parameters.
> 
> This also paves the way for a simplification of the core vma_merge()
> implementation, as we seek to make it entirely an implementation detail.
> 
> Note that on mmap_region(), VMA fields are initialised to zero, so we can
> simply reference these rather than explicitly specifying NULL.

I don't think that's accurate.. mmap_region() sets the start, end,
offset, flags.  It also passes this vma into a driver, so I'm not sure
we can rely on them being anything after that?  The whole reason
vma_merge() is attempted in this case is because the driver may have
changed vma->vm_flags on us.  Your way may actually be better since the
driver may set something we assume is NULL today.

> 
> Reviewed-by: Vlastimil Babka <[e-mail]>
> Signed-off-by: Lorenzo Stoakes <[e-mail]>
> ---
>  mm/mmap.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 17c0dcfb1527..33aafd23823b 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2482,6 +2482,22 @@ struct vm_area_struct *vma_modify(struct vma_iterator *vmi,
>  	return NULL;
>  }
>  
> +/*
> + * Attempt to merge a newly mapped VMA with those adjacent to it. The caller
> + * must ensure that [start, end) does not overlap any existing VMA.
> + */
> +static struct vm_area_struct *vma_merge_new_vma(struct vma_iterator *vmi,
> +						struct vm_area_struct *prev,
> +						struct vm_area_struct *vma,
> +						unsigned long start,
> +						unsigned long end,
> +						pgoff_t pgoff)

It's not a coding style, but if you used two tabs here, it may make this
more condensed.

> +{
> +	return vma_merge(vmi, vma->vm_mm, prev, start, end, vma->vm_flags,
> +			 vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> +			 vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> +}
> +
>  /*
>   * do_vmi_align_munmap() - munmap the aligned region from @start to @end.
>   * @vmi: The vma iterator
> @@ -2837,10 +2853,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  		 * vma again as we may succeed this time.
>  		 */
>  		if (unlikely(vm_flags != vma->vm_flags && prev)) {
> -			merge = vma_merge(&vmi, mm, prev, vma->vm_start,
> -				    vma->vm_end, vma->vm_flags, NULL,
> -				    vma->vm_file, vma->vm_pgoff, NULL,
> -				    NULL_VM_UFFD_CTX, NULL);
> +			merge = vma_merge_new_vma(&vmi, prev, vma,
> +						  vma->vm_start, vma->vm_end,
> +						  pgoff);
                                                   └ vma->vm_pgoff
>  			if (merge) {
>  				/*
>  				 * ->mmap() can change vma->vm_file and fput
> @@ -3382,9 +3397,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>  	if (new_vma && new_vma->vm_start < addr + len)
>  		return NULL;	/* should never get here */
>  
> -	new_vma = vma_merge(&vmi, mm, prev, addr, addr + len, vma->vm_flags,
> -			    vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> -			    vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> +	new_vma = vma_merge_new_vma(&vmi, prev, vma, addr, addr + len, pgoff);
>  	if (new_vma) {
>  		/*
>  		 * Source vma may have been merged into new_vma
> -- 
> 2.42.0
>

Lorenzo Stoakes Oct. 11, 2023, 6:48 a.m. UTC | #2

On Tue, Oct 10, 2023 at 09:51:40PM -0400, Liam R. Howlett wrote:
> * Lorenzo Stoakes <[e-mail]> [231009 16:53]:
> > Only in mmap_region() and copy_vma() do we attempt to merge VMAs which
> > occupy entirely new regions of virtual memory.
> >
> > We can abstract this logic and make the intent of this invocations of it
> > completely explicit, rather than invoking vma_merge() with an inscrutable
> > wall of parameters.
> >
> > This also paves the way for a simplification of the core vma_merge()
> > implementation, as we seek to make it entirely an implementation detail.
> >
> > Note that on mmap_region(), VMA fields are initialised to zero, so we can
> > simply reference these rather than explicitly specifying NULL.
>
> I don't think that's accurate.. mmap_region() sets the start, end,
> offset, flags.  It also passes this vma into a driver, so I'm not sure
> we can rely on them being anything after that?  The whole reason
> vma_merge() is attempted in this case is because the driver may have
> changed vma->vm_flags on us.  Your way may actually be better since the
> driver may set something we assume is NULL today.

Yeah I think I wasn't clear here - I meant to say that we memset -> 0 so
all fields that are not specified (e.g. not start, end, offset, flags).

However you make a very good point re: the driver, which I hadn't thought
of, also it's worth saying here that we specifically only do this for a
file-backed mapping just for complete clarity.

I will add a note to this part of the v3 series asking Andrew to update the
comment.

>
> >
> > Reviewed-by: Vlastimil Babka <[e-mail]>
> > Signed-off-by: Lorenzo Stoakes <[e-mail]>
> > ---
> >  mm/mmap.c | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 17c0dcfb1527..33aafd23823b 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2482,6 +2482,22 @@ struct vm_area_struct *vma_modify(struct vma_iterator *vmi,
> >  	return NULL;
> >  }
> >
> > +/*
> > + * Attempt to merge a newly mapped VMA with those adjacent to it. The caller
> > + * must ensure that [start, end) does not overlap any existing VMA.
> > + */
> > +static struct vm_area_struct *vma_merge_new_vma(struct vma_iterator *vmi,
> > +						struct vm_area_struct *prev,
> > +						struct vm_area_struct *vma,
> > +						unsigned long start,
> > +						unsigned long end,
> > +						pgoff_t pgoff)
>
> It's not a coding style, but if you used two tabs here, it may make this
> more condensed.

Checkpatch shouts at me about aligning to the paren, I obviously could just
put "static struct vm_area_struct *" on the line before to make this a bit
better though. If we go to a v4 will fix, otherwise I think probably ok to
leave even if a bit squished for now?

>
> > +{
> > +	return vma_merge(vmi, vma->vm_mm, prev, start, end, vma->vm_flags,
> > +			 vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> > +			 vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > +}
> > +
> >  /*
> >   * do_vmi_align_munmap() - munmap the aligned region from @start to @end.
> >   * @vmi: The vma iterator
> > @@ -2837,10 +2853,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  		 * vma again as we may succeed this time.
> >  		 */
> >  		if (unlikely(vm_flags != vma->vm_flags && prev)) {
> > -			merge = vma_merge(&vmi, mm, prev, vma->vm_start,
> > -				    vma->vm_end, vma->vm_flags, NULL,
> > -				    vma->vm_file, vma->vm_pgoff, NULL,
> > -				    NULL_VM_UFFD_CTX, NULL);
> > +			merge = vma_merge_new_vma(&vmi, prev, vma,
> > +						  vma->vm_start, vma->vm_end,
> > +						  pgoff);
>                                                    └ vma->vm_pgoff
> >  			if (merge) {
> >  				/*
> >  				 * ->mmap() can change vma->vm_file and fput
> > @@ -3382,9 +3397,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> >  	if (new_vma && new_vma->vm_start < addr + len)
> >  		return NULL;	/* should never get here */
> >
> > -	new_vma = vma_merge(&vmi, mm, prev, addr, addr + len, vma->vm_flags,
> > -			    vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> > -			    vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > +	new_vma = vma_merge_new_vma(&vmi, prev, vma, addr, addr + len, pgoff);
> >  	if (new_vma) {
> >  		/*
> >  		 * Source vma may have been merged into new_vma
> > --
> > 2.42.0
> >

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 17c0dcfb1527..33aafd23823b 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2482,6 +2482,22 @@  struct vm_area_struct *vma_modify(struct vma_iterator *vmi,
 	return NULL;
 }
 
+/*
+ * Attempt to merge a newly mapped VMA with those adjacent to it. The caller
+ * must ensure that [start, end) does not overlap any existing VMA.
+ */
+static struct vm_area_struct *vma_merge_new_vma(struct vma_iterator *vmi,
+						struct vm_area_struct *prev,
+						struct vm_area_struct *vma,
+						unsigned long start,
+						unsigned long end,
+						pgoff_t pgoff)
+{
+	return vma_merge(vmi, vma->vm_mm, prev, start, end, vma->vm_flags,
+			 vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
+			 vma->vm_userfaultfd_ctx, anon_vma_name(vma));
+}
+
 /*
  * do_vmi_align_munmap() - munmap the aligned region from @start to @end.
  * @vmi: The vma iterator
@@ -2837,10 +2853,9 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 		 * vma again as we may succeed this time.
 		 */
 		if (unlikely(vm_flags != vma->vm_flags && prev)) {
-			merge = vma_merge(&vmi, mm, prev, vma->vm_start,
-				    vma->vm_end, vma->vm_flags, NULL,
-				    vma->vm_file, vma->vm_pgoff, NULL,
-				    NULL_VM_UFFD_CTX, NULL);
+			merge = vma_merge_new_vma(&vmi, prev, vma,
+						  vma->vm_start, vma->vm_end,
+						  pgoff);
 			if (merge) {
 				/*
 				 * ->mmap() can change vma->vm_file and fput
@@ -3382,9 +3397,7 @@  struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 	if (new_vma && new_vma->vm_start < addr + len)
 		return NULL;	/* should never get here */
 
-	new_vma = vma_merge(&vmi, mm, prev, addr, addr + len, vma->vm_flags,
-			    vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
-			    vma->vm_userfaultfd_ctx, anon_vma_name(vma));
+	new_vma = vma_merge_new_vma(&vmi, prev, vma, addr, addr + len, pgoff);
 	if (new_vma) {
 		/*
 		 * Source vma may have been merged into new_vma