Received-SPF: pass (google.com: domain of linux-kernel+bounces-139315-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 10 Apr 2024 21:26:45 +0100
From: Matthew Wilcox <willy@infradead.org>
To: Peter Xu <peterx@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Lokesh Gidra <lokeshgidra@google.com>,
	"Liam R . Howlett" <Liam.Howlett@oracle.com>,
	Alistair Popple <apopple@nvidia.com>
Subject: Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks
Message-ID: <Zhb2BWntckP3ZhDc@casper.infradead.org>
References: <20240410170621.2011171-1-peterx@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240410170621.2011171-1-peterx@redhat.com>

On Wed, Apr 10, 2024 at 01:06:21PM -0400, Peter Xu wrote:
> anon_vma is a tricky object in the context of per-vma lock, because it's
> racy to modify it in that context and mmap lock is needed if it's not
> stable yet.

I object to this commit message.  First, it's not a "sanity check".  It's
a check to see if we already have an anon VMA.  Second, it's not "racy
to modify it" at all.  The problem is that we need to look at other
VMAs, for which we do not hold the lock.

> So the trivial side effect of such patch is:
> 
>   - We may do slightly better on the first WRITE of a private file mapping,
>   because we can retry earlier (in lock_vma_under_rcu(), rather than
>   vmf_anon_prepare() later).
> 
>   - We may always use mmap lock for the initial READs on a private file
>   mappings, while before this patch it _can_ (only when no WRITE ever
>   happened... but it doesn't make much sense for a MAP_PRIVATE..) do the
>   read fault with per-vma lock.

But that's a super common path!  Look at 'cat /proc/self/maps'.  All
your program text (including libraries) is mapped PRIVATE, and never
written to (except by ptrace, I guess).

NAK this patch.