All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lorenzo Stoakes <lstoakes@gmail.com>
To: "Liam R. Howlett" <Liam.Howlett@oracle.com>,
	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>,
	maple-tree@lists.infradead.org, Vernon Yang <vernon2gm@gmail.com>
Subject: Re: [PATCH v2 2/4] mm/mmap/vma_merge: set next to NULL if not applicable
Date: Wed, 22 Mar 2023 06:23:43 +0000	[thread overview]
Message-ID: <467b6158-5c91-4c95-b05b-ea9331f7f563@lucifer.local> (raw)
In-Reply-To: <20230322014232.4iixbwn5urfo6t2y@revolver>

On Tue, Mar 21, 2023 at 09:42:32PM -0400, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lstoakes@gmail.com> [230321 16:51]:
> > 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.
> >
> > This has no functional impact.
> >
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> > ---
> >  mm/mmap.c | 25 ++++++++++++-------------
> >  1 file changed, 12 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index c9834364ac98..6361baf75601 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -930,15 +930,15 @@ 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;
> > +	/* Does the input range span an existing VMA? (cases 5 - 8) */
> > +	curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end);
> >
> > -	/* In cases 1 - 4 there's no CCCC vma */
> > -	if (curr && end <= curr->vm_start)
> > -		curr = NULL;
> > +	if (curr && end == curr->vm_end)
> > +		/* Is there is a VMA immediately adjacent (cases 6 - 8)? */
> > +		next = vma_lookup(mm, curr->vm_end);
>
> Since end == curr->vm_end, this lookup is the same as below so these two
> statements can be combined.

I can't believe I didn't see this :) wood for the trees + a great example as to
the benefit of code review.

>
> I also think you may need to initialize next to null since it may not be
> set for the 'cannot merge' case.

You're right, will fix.

>
> Something like:
> 	if ((!curr) ||		    /* case 1-4 */
> 	     (end == curr->vm_end)) /* Case 6-8, adjacent vma */
> 		next = vma_lookup(mm, end);
> 	else
> 		next = NULL
>

This is really turning out to be quite nicely succinct now I think! Will do.

>
> > +	else if (!curr)
> > +		/* Is there a VMA next to a hole (case 1 - 3) or prev (4)? */
> > +		next = vma_lookup(mm, end);
>
> Nit, can we have braces for comments that make the if/else look like
> it's unguarded?
>

Ack will fix in respin.

> >
> >  	/* verify some invariant that must be enforced by the caller */
> >  	VM_WARN_ON(prev && addr <= prev->vm_start);
> > @@ -959,11 +959,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
> >

  reply	other threads:[~2023-03-22  6:26 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-21 20:45 [PATCH v2 0/4] further cleanup of vma_merge() Lorenzo Stoakes
2023-03-21 20:45 ` [PATCH v2 1/4] mm/mmap/vma_merge: further improve prev/next VMA naming Lorenzo Stoakes
2023-03-21 20:45 ` [PATCH v2 2/4] mm/mmap/vma_merge: set next to NULL if not applicable Lorenzo Stoakes
2023-03-22  1:42   ` Liam R. Howlett
2023-03-22  6:23     ` Lorenzo Stoakes [this message]
2023-03-21 20:45 ` [PATCH v2 3/4] mm/mmap/vma_merge: extend invariants, avoid invalid res, vma Lorenzo Stoakes
2023-03-22  9:16   ` Vlastimil Babka
     [not found]   ` <CGME20230323170046eucas1p2483ab0fcc3d6bc56d4b6d09143bbadda@eucas1p2.samsung.com>
2023-03-23 17:00     ` Marek Szyprowski
2023-03-23 17:08       ` Lorenzo Stoakes
2023-03-23 17:10         ` Lorenzo Stoakes
2023-03-21 20:45 ` [PATCH v2 4/4] mm/mmap/vma_merge: init cleanup, be explicit about the non-mergeable case Lorenzo Stoakes
2023-03-22  2:08   ` Liam R. Howlett
2023-03-22  6:07     ` Lorenzo Stoakes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=467b6158-5c91-4c95-b05b-ea9331f7f563@lucifer.local \
    --to=lstoakes@gmail.com \
    --cc=Liam.Howlett@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=david@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=maple-tree@lists.infradead.org \
    --cc=vbabka@suse.cz \
    --cc=vernon2gm@gmail.com \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.