Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Wed, 11 Oct 2023 07:58:20 +0100
From:   Lorenzo Stoakes <lstoakes@gmail.com>
To:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>
Cc:     "=Liam R . Howlett" <Liam.Howlett@oracle.com>,
        Vlastimil Babka <vbabka@suse.cz>, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v3 4/5] mm: abstract merge for new VMAs into
 vma_merge_new_vma()
Message-ID: <e2c8fb0e-e5dc-454e-aad7-93a85fefaa9a@lucifer.local>
References: <cover.1696929425.git.lstoakes@gmail.com>
 <fe658ae961de1206f1557001f4d41d6e931d3919.1696929425.git.lstoakes@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fe658ae961de1206f1557001f4d41d6e931d3919.1696929425.git.lstoakes@gmail.com>
Precedence: bulk

On Tue, Oct 10, 2023 at 07:23:07PM +0100, Lorenzo Stoakes wrote:
> 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.

Andrew - based on feedback from Liam on the v2 version of this patch, could
we change this commit message to:-


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.

The VMA merge call in mmap_region() occurs only for file-backed mappings,
where each of the parameters previously specified as NULL are defaulted to
NULL in vma_init() (called by vm_area_alloc()).

This matches the previous behaviour of specifying NULL for a number of
fields, however note that prior to this call we pass the VMA to the file
system driver via call_mmap(), which may in theory adjust fields that we
pass in to vma_merge_new_vma().

Therefore we actually resolve an oversight here by allowing for the fact
that the driver may have done this.


>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  mm/mmap.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>

Thanks!

> diff --git a/mm/mmap.c b/mm/mmap.c
> index a516f2412f79..db3842601a88 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2485,6 +2485,22 @@ struct vm_area_struct *vma_modify(struct vma_iterator *vmi,
>  	return vma;
>  }
>
> +/*
> + * 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
> @@ -2840,10 +2856,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
> @@ -3385,9 +3400,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
>