Received-SPF: pass (google.com: domain of linux-kernel+bounces-142665-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
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>
In-Reply-To: <ZhinCD-PoblxGFm0@casper.infradead.org>
From: Suren Baghdasaryan <surenb@google.com>
Date: Fri, 12 Apr 2024 05:46:52 -0700
Message-ID: <CAJuCfpHpewmiyMxyPd_A8KSyQ6tB-1wHLV-FJ7KBrpV=RY9F7w@mail.gmail.com>
Subject: Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks
To: Matthew Wilcox <willy@infradead.org>
Cc: Peter Xu <peterx@redhat.com>, "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 Thu, Apr 11, 2024 at 8:14=E2=80=AFPM Matthew Wilcox <willy@infradead.org=
> 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 private=
 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 to
> > add extra instructions to the fault path for single-threaded programs,
> > but we at least had the justification that we were improving scalabilit=
y
> > 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 m=
m_struct *mm,
>         if (!vma_start_read(vma))
>                 goto inval;
>
> -       /*
> -        * find_mergeable_anon_vma uses adjacent vmas which are not locke=
d.
> -        * This check must happen after vma_start_read(); otherwise, a
> -        * concurrent mremap() with MREMAP_DONTUNMAP could dissociate the=
 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 VM=
A */
>         if (unlikely(address < vma->vm_start || address >=3D vma->vm_end)=
)
>                 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!)

I see what you mean. The impact would depend on the workload but in my
earlier tests when developing per-VMA locks there were on average less
than 1% faults which were for anonymous pages and had
vma->anon_vma=3D=3DNULL. I recorded that after using my desktop for a day
or so and running a series of benchmark tests. Again, that number
might be drastically different on some other workloads.

About the code, I'll take a closer look once I'm back from vacation
this weekend but I think you will also have to modify
do_anonymous_page() to use vmf_anon_prepare() instead of
anon_vma_prepare().

>
> We'd need some data to be sure it's really a win, but less code is
> always good.
>
> We could even eagerly initialise vma->anon_vma for anon vmas.  I don't
> know why we don't do that.

You found the answer to that question a long time ago and IIRC it was
because in many cases we end up not needing to set vma->anon_vma at
all. So, this is an optimization to try avoiding extra operations
whenever we can. I'll try to find your comment on this.