Date:   Mon, 20 Mar 2023 14:58:22 +0000
From:   Lorenzo Stoakes <lstoakes@gmail.com>
To:     Vernon Yang <vernon2gm@gmail.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>,
        Matthew Wilcox <willy@infradead.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Liam R . Howlett" <Liam.Howlett@oracle.com>,
        maple-tree@lists.infradead.org
Subject: Re: [PATCH 2/4] mm/mmap/vma_merge: set next to NULL if not applicable
Message-ID: <ZBh0jl2+IVOSB0SH@murray>
References: <cover.1679137163.git.lstoakes@gmail.com>
 <4d717269303d8a6fe1d837968c252eeb6ff1d7e5.1679137163.git.lstoakes@gmail.com>
 <ZBhpPFmAOKyohN0M@vernon-pc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZBhpPFmAOKyohN0M@vernon-pc>
Precedence: bulk

On Mon, Mar 20, 2023 at 10:25:11PM +0800, Vernon Yang wrote:
> On Sat, Mar 18, 2023 at 11:13:19AM +0000, Lorenzo Stoakes wrote:
> > We are only interested in next if end == next->vm_start (in which case we
> > check to see if we can set merge_next), so perform this check alongside
> > checking whether curr should be set.
> >
> > This groups all of the simple range checks together and establishes the
> > invariant that, if prev, curr or next are non-NULL then their positions are
> > as expected.
> >
> > Additionally, use the abstract 'vma' object to look up the possible curr or
> > next VMA in order to avoid any confusion as to what these variables
> > represent - now curr and next are assigned once and only once.
>
> Hi Lorenzo,
>
> Due to the "vma" variable is used as an intermediate member, I feels a bit
> confusing, so cleanup this patch as below.

Hi Vernon, if you read the commit message you'll see what you're undoing
here is exactly what I have done on purpose. The point is to assign each of
curr and next once and only once after we've determined which of the two we
are assigning to.

You also delete some of the explanation which I added explicitly to make
the logic clearer and adjust _punctionation_ neither of which I feel are
positive changes.

The point is that existing logic treated either mid or curr as temporary
variables that might get reassigned.

By using a temporary value and explaining what we're doing, you can see
_why_ we are assigning it.

>
> If this cleanup patch is issue, please let me know, and then ignore it
> directly, thanks.

So I'm afraid I am not in favour of your change, though thank you for your
interest!

I am happy to get feedback on the change, though I'd suggest commenting on
anything you have issues with rather than attempting to rework my change as
if we start getting patches within patches it's going to end like inception
:)

Cheers, Lorenzo

>
> ----
> From 7dac3ed8c1b747c2edf2a6c867c4e6342c7447c3 Mon Sep 17 00:00:00 2001
> From: Vernon Yang <vernon2gm@gmail.com>
> Date: Mon, 20 Mar 2023 21:38:09 +0800
> Subject: [PATCH] mm/mmap/vma_merge: set next to NULL if not applicable-cleanup
>
> Make code logic simpler and more readable.
>
> Signed-off-by: Vernon Yang <vernon2gm@gmail.com>
> ---
>  mm/mmap.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 45bd43225013..78cb96774602 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -921,7 +921,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  	 * If there is a previous VMA specified, find the next, otherwise find
>  	 * the first.
>  	 */
> -	vma = find_vma(mm, prev ? prev->vm_end : 0);
> +	curr = find_vma(mm, prev ? prev->vm_end : 0);
>
>  	/*
>  	 * Does the input range span an existing VMA? If so, we designate this
> @@ -929,21 +929,19 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  	 *
>  	 * Cases 5 - 8.
>  	 */
> -	if (vma && end > vma->vm_start) {
> -		curr = vma;
> -
> +	if (curr && end > curr->vm_start) {
>  		/*
>  		 * If the addr - end range spans this VMA entirely, then we
>  		 * check to see if another VMA follows it.
>  		 *
> -		 * If it is _immediately_ adjacent (checked below), then we
> +		 * If it is immediately adjacent (checked below), then we
>  		 * designate it 'next' (cases 6 - 8).
>  		 */
>  		if (curr->vm_end == end)
> -			vma = find_vma(mm, curr->vm_end);
> +			next = find_vma(mm, curr->vm_end);
>  		else
>  			/* Case 5. */
> -			vma = NULL;
> +			next = NULL;
>  	} else {
>  		/*
>  		 * The addr - end range either spans the end of prev or spans no
> @@ -952,19 +950,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  		 *
>  		 * Cases 1 - 4.
>  		 */
> +		next = curr;
>  		curr = NULL;
>  	}
>
> -	/*
> -	 * We only actually examine the next VMA if it is immediately adjacent
> -	 * to end which sits either at the end of a hole (cases 1 - 3), PPPP
> -	 * (case 4) or CCCC (cases 6 - 8).
> -	 */
> -	if (vma && end == vma->vm_start)
> -		next = vma;
> -	else
> -		next = NULL;
> -
>  	/*
>  	 * By default, we return prev. Cases 3, 4, 8 will instead return next
>  	 * and cases 3, 8 will also update vma to point at next.
> --
> 2.34.1
>
>
> >
> > This has no functional impact.
> >
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> > ---
> >  mm/mmap.c | 61 ++++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 49 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index c9834364ac98..66893fc72e03 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -930,15 +930,53 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> >  	if (vm_flags & VM_SPECIAL)
> >  		return NULL;
> >
> > -	curr = find_vma(mm, prev ? prev->vm_end : 0);
> > -	if (curr && curr->vm_end == end)		/* cases 6, 7, 8 */
> > -		next = find_vma(mm, curr->vm_end);
> > -	else
> > -		next = curr;
> > +	/*
> > +	 * If there is a previous VMA specified, find the next, otherwise find
> > +	 * the first.
> > +	 */
> > +	vma = find_vma(mm, prev ? prev->vm_end : 0);
> > +
> > +	/*
> > +	 * Does the input range span an existing VMA? If so, we designate this
> > +	 * VMA 'curr'. The caller will have ensured that curr->vm_start == addr.
> > +	 *
> > +	 * Cases 5 - 8.
> > +	 */
> > +	if (vma && end > vma->vm_start) {
> > +		curr = vma;
> >
> > -	/* In cases 1 - 4 there's no CCCC vma */
> > -	if (curr && end <= curr->vm_start)
> > +		/*
> > +		 * If the addr - end range spans this VMA entirely, then we
> > +		 * check to see if another VMA follows it.
> > +		 *
> > +		 * If it is _immediately_ adjacent (checked below), then we
> > +		 * designate it 'next' (cases 6 - 8).
> > +		 */
> > +		if (curr->vm_end == end)
> > +			vma = find_vma(mm, curr->vm_end);
> > +		else
> > +			/* Case 5. */
> > +			vma = NULL;
> > +	} else {
> > +		/*
> > +		 * The addr - end range either spans the end of prev or spans no
> > +		 * VMA at all - in either case we dispense with 'curr' and
> > +		 * maintain only 'prev' and (possibly) 'next'.
> > +		 *
> > +		 * Cases 1 - 4.
> > +		 */
> >  		curr = NULL;
> > +	}
> > +
> > +	/*
> > +	 * We only actually examine the next VMA if it is immediately adjacent
> > +	 * to end which sits either at the end of a hole (cases 1 - 3), PPPP
> > +	 * (case 4) or CCCC (cases 6 - 8).
> > +	 */
> > +	if (vma && end == vma->vm_start)
> > +		next = vma;
> > +	else
> > +		next = NULL;
> >
> >  	/* verify some invariant that must be enforced by the caller */
> >  	VM_WARN_ON(prev && addr <= prev->vm_start);
> > @@ -959,11 +997,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> >  		}
> >  	}
> >  	/* Can we merge the successor? */
> > -	if (next && end == next->vm_start &&
> > -			mpol_equal(policy, vma_policy(next)) &&
> > -			can_vma_merge_before(next, vm_flags,
> > -					     anon_vma, file, pgoff+pglen,
> > -					     vm_userfaultfd_ctx, anon_name)) {
> > +	if (next && mpol_equal(policy, vma_policy(next)) &&
> > +	    can_vma_merge_before(next, vm_flags,
> > +				 anon_vma, file, pgoff+pglen,
> > +				 vm_userfaultfd_ctx, anon_name)) {
> >  		merge_next = true;
> >  	}
> >
> >
> > --
> > 2.39.2
> >