Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\))
Subject: Re: [PATCH 01/13] mm: Update ptep_get_lockless()s comment
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <CAHk-=wjX_P78xoNcGDTjhkgffs-Bhzcwp-mdsE1maeF57Sh0MA@mail.gmail.com>
Date:   Sun, 30 Oct 2022 12:34:51 -0700
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Jann Horn <jannh@google.com>,
        John Hubbard <jhubbard@nvidia.com>, X86 ML <x86@kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        jroedel@suse.de, ubizjak@gmail.com,
        Alistair Popple <apopple@nvidia.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <44A8D373-24CA-4777-AFC8-DB48F0DC4FAE@gmail.com>
References: <20221022111403.531902164@infradead.org>
 <20221022114424.515572025@infradead.org>
 <2c800ed1-d17a-def4-39e1-09281ee78d05@nvidia.com>
 <Y1ZGNwUEdm15W6Xz@hirez.programming.kicks-ass.net>
 <CAG48ez3fG=WnvbiE5mJaD66_gJj_hohnV8CqBG9eNdjd7pJW3A@mail.gmail.com>
 <Y1fsYwshJ93FT21r@hirez.programming.kicks-ass.net>
 <CAG48ez3VE+3dVdUMK+Pg_942gR+h_TCcSaFxGwCbNfh3W+mfOA@mail.gmail.com>
 <Y1f7YvKuwOl1XEwU@hirez.programming.kicks-ass.net>
 <CAG48ez05gBiB2w7bL_3O_OUoBWazmHnRwMiuni_wQyXBUcaxbQ@mail.gmail.com>
 <Y1oub9MvqwGBlHkq@hirez.programming.kicks-ass.net>
 <CAHk-=wihPdOtXgJ32pLB6Xd-UvnaZW9YvOOjM24JGYRjNHeykA@mail.gmail.com>
 <6C548A9A-3AF3-4EC1-B1E5-47A7FFBEB761@gmail.com>
 <CAHk-=wh8oi0qQtYDFTfm7d1s5C8mG7ig=NfzGWt4zbjXMzcdqQ@mail.gmail.com>
 <F9E42822-DA1D-4192-8410-3BAE42E9E4A9@gmail.com>
 <B88D3073-440A-41C7-95F4-895D3F657EF2@gmail.com>
 <CAHk-=wgzT1QsSCF-zN+eS06WGVTBg4sf=6oTMg95+AEq7QrSCQ@mail.gmail.com>
 <47678198-C502-47E1-B7C8-8A12352CDA95@gmail.com>
 <CAHk-=wjzngbbwHw4nAsqo_RpyOtUDk5G+Wus=O0w0A6goHvBWA@mail.gmail.com>
 <CAHk-=wijU_YHSZq5N7vYK+qHPX0aPkaePaGOyWk4aqMvvSXxJA@mail.gmail.com>
 <140B437E-B994-45B7-8DAC-E9B66885BEEF@gmail.com>
 <CAHk-=wjX_P78xoNcGDTjhkgffs-Bhzcwp-mdsE1maeF57Sh0MA@mail.gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Precedence: bulk

On Oct 30, 2022, at 11:19 AM, Linus Torvalds =
<torvalds@linux-foundation.org> wrote:

> And page_remove_rmap() could *almost* be called later, but it does
> have code that also depends on the page table lock, although it looks
> like realistically that's just because it "knows" that means that
> preemption is disabled, so it uses non-atomic statistics update.
>=20
> I say "knows" in quotes, because that's what the comment says, but it
> turns out that __mod_node_page_state() has to deal with CONFIG_RT
> anyway and does that
>=20
>        preempt_disable_nested();
>        ...
>        preempt_enable_nested();
>=20
> thing.
>=20
> And then it wants to see the vma, although that's actually only to see
> if it's 'mlock'ed, so we could just squirrel that away.
>=20
> So we *could* move page_remove_rmap() later into the TLB flush region,
> but then we would have lost the page table lock anyway, so then
> folio_mkclean() can come in regardless.
>=20
> So that doesn't even help.

Well, if you combine it with the per-page-table stale TLB detection
mechanism that I proposed, I think this could work.

Reminder (feel free to skip): you would have per-mm =E2=80=9Ccompleted
TLB-generation=E2=80=9D in addition to the current one, which would be =
renamed to
=E2=80=9Cpending TLB-generation=E2=80=9D. Whenever you update the =
page-tables in a manner
that might require a TLB flush, you would increase the =E2=80=9Cpending
TLB-generation=E2=80=9D and save the pending TLB-generation in the =
page-table=E2=80=99s
page-struct. All of that is done once under the page-table lock. When =
you
finish a TLB-flush, you update the =E2=80=9Ccompleted TLB-generation=E2=80=
=9D.

Then on page_vma_mkclean_one(), you would check if the page-table=E2=80=99=
s
TLB-generation is greater than the completed TLB-generation, which would
indicate that TLB entries for PTEs in this table might be stale. In that
case you would just flush the TLB. [ Of course you can instead just =
flush if
mm_tlb_flush_pending(), but nobody likes this mechanism that has a very
coarse granularity, and therefore can lead to many unnecessary TLB =
flushes.
]

Indeed, there would be potentially some overhead in extreme cases, since
mm's TLB-generation since its cache is already highly-contended in =
extreme
cases. But I think it worth it to have simple logic that allows to =
reason
about correctness.

My intuition is that although you appear to be right that we can just =
mark
this case as =E2=80=9Cextreme case nobody cares about=E2=80=9D, it might =
have now or in the
future some other implications that are hard to predict and prevent.