Received-SPF: pass (google.com: domain of linux-kernel+bounces-142687-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240410170621.2011171-1-peterx@redhat.com> <20240411171319.almhz23xulg4f7op@revolver>
 <ZhhSItiyLYBEdAX3@x1n> <ZhhV3PKgEX9d7_vA@casper.infradead.org>
 <ZhhaRXHKk7w_hKgi@x1n> <Zhhd-A7w1A8JUadM@casper.infradead.org>
 <ZhinCD-PoblxGFm0@casper.infradead.org> <ZhkrY5tkxgAsL1GF@x1n>
In-Reply-To: <ZhkrY5tkxgAsL1GF@x1n>
From: Suren Baghdasaryan <surenb@google.com>
Date: Fri, 12 Apr 2024 06:06:46 -0700
Message-ID: <CAJuCfpG7YkQ2giKiv07TetTn=QHK9x723vnLaTjDCaQjUvAavw@mail.gmail.com>
Subject: Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks
To: Peter Xu <peterx@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>, "Liam R. Howlett" <Liam.Howlett@oracle.com>, 
	linux-kernel@vger.kernel.org, linux-mm@kvack.org, 
	Andrew Morton <akpm@linux-foundation.org>, Lokesh Gidra <lokeshgidra@google.com>, 
	Alistair Popple <apopple@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Apr 12, 2024 at 5:39=E2=80=AFAM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Apr 12, 2024 at 04:14:16AM +0100, Matthew Wilcox wrote:
> > On Thu, Apr 11, 2024 at 11:02:32PM +0100, Matthew Wilcox wrote:
> > > > How many instructions it takes for a late RETRY for WRITEs to priva=
te file
> > > > mappings, fallback to mmap_sem?
> > >
> > > Doesn't matter.  That happens _once_ per VMA, and it's dwarfed by the
> > > cost of allocating and initialising the COWed page.  You're adding
> > > instructions to every single page fault.  I'm not happy that we had t=
o
> > > add extra instructions to the fault path for single-threaded programs=
,
> > > but we at least had the justification that we were improving scalabil=
ity
> > > on large systems.  Your excuse is "it makes the code cleaner".  And
> > > honestly, I don't think it even does that.
> >
> > Suren, what would you think to this?
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 6e2fe960473d..e495adcbe968 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -5821,15 +5821,6 @@ struct vm_area_struct *lock_vma_under_rcu(struct=
 mm_struct *mm,
> >         if (!vma_start_read(vma))
> >                 goto inval;
> >
> > -       /*
> > -        * find_mergeable_anon_vma uses adjacent vmas which are not loc=
ked.
> > -        * This check must happen after vma_start_read(); otherwise, a
> > -        * concurrent mremap() with MREMAP_DONTUNMAP could dissociate t=
he VMA
> > -        * from its anon_vma.
> > -        */
> > -       if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma))
> > -               goto inval_end_read;
> > -
> >         /* Check since vm_start/vm_end might change before we lock the =
VMA */
> >         if (unlikely(address < vma->vm_start || address >=3D vma->vm_en=
d))
> >                 goto inval_end_read;
> >
> > That takes a few insns out of the page fault path (good!) at the cost
> > of one extra trip around the fault handler for the first fault on an
> > anon vma.  It makes the file & anon paths more similar to each other
> > (good!)
> >
> > We'd need some data to be sure it's really a win, but less code is
> > always good.
>
> You at least need two things:
>
>   (1) don't throw away Jann's comment so easily

I agree, if we make this change we should keep this comment and maybe
move it into vmf_anon_prepare()

>
>   (2) have a look on whether anon memory has the fallback yet, at all

Yeah, I think do_anonymous_page() will have to change as I mentioned
in the previous reply.

>
> Maybe someone can already comment in a harsh way on this one, but no, I'm
> not going to be like that.
>
> I still don't understand why you don't like so much to not fallback at al=
l
> if we could, the flags I checked was all in hot cache I think anyway.
>
> And since I'm also enough on how you comment in your previous replies, I'=
ll
> leave the rest comments for others.

FWIW I fully accept the blame for not seeing that private file mapping
read case regression. In retrospect this should have been obvious...
but the hindsight is always 20/20.

>
> --
> Peter Xu
>