Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.60.0.1.1\))
Subject: Re: [RESEND PATCH v3 5/5] mm: avoid unnecessary flush on
 change_huge_pmd()
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <e5f84691-3475-1cbd-e46c-163bf594a4bc@intel.com>
Date:   Fri, 11 Mar 2022 12:53:30 -0800
Cc:     Linux-MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Peter Xu <peterx@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Will Deacon <will@kernel.org>, Yu Zhao <yuzhao@google.com>,
        Nick Piggin <npiggin@gmail.com>, x86@kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <A2252098-2EE1-4F8D-A1A2-A49EE8FD18A7@gmail.com>
References: <20220311190749.338281-1-namit@vmware.com>
 <20220311190749.338281-6-namit@vmware.com>
 <e5f84691-3475-1cbd-e46c-163bf594a4bc@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Precedence: bulk


> On Mar 11, 2022, at 12:41 PM, Dave Hansen <dave.hansen@intel.com> =
wrote:
>=20
> On 3/11/22 11:07, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>>=20
>> Calls to change_protection_range() on THP can trigger, at least on =
x86,
>> two TLB flushes for one page: one immediately, when pmdp_invalidate() =
is
>> called by change_huge_pmd(), and then another one later (that can be
>> batched) when change_protection_range() finishes.
>>=20
>> The first TLB flush is only necessary to prevent the dirty bit (and =
with
>> a lesser importance the access bit) from changing while the PTE is
>> modified. However, this is not necessary as the x86 CPUs set the
>> dirty-bit atomically with an additional check that the PTE is (still)
>> present. One caveat is Intel's Knights Landing that has a bug and =
does
>> not do so.
>=20
> First of all, thank you for your diligence here.  This is a super
> obscure issue.  I think I put handling for it in the kernel and I'm =
not
> sure I would have even thought about this angle.
>=20
> That said, I'm not sure this is all necessary.
>=20
> Yes, the Dirty bit can get set unexpectedly in some PTEs.  But, the
> question is whether it is *VALUABLE* and needs to be preserved.  The
> current kernel code pretty much just lets the hardware set the Dirty =
bit
> and then ignores it.  If it were valuable, ignoring it would have been =
a
> bad thing.  We'd be losing data on today's kernels because the =
hardware
> told us about a write that happened but that the kernel ignored.
>=20
> My mental model of what the microcode responsible for the erratum does
> is something along these lines:
>=20
> 	if (write)
> 		pte |=3D _PAGE_DIRTY;
> 	if (!pte_present(pte))
> 		#PF
>=20
> The PTE is marked dirty, but the write never actually executes.  The
> thread that triggered the A/D setting *also* gets a fault.
>=20

This makes perfect sense. I guess I misunderstood or forgot the erratum.
But feel free to recheck. It would allow to remove the KNL check, and
probably the first patch in this series. But I don=E2=80=99t think it =
would
allow to get rid of pmdp_invalidate_ad() since I do not fell comfortable
just to use pmdp_establish() directly: I do not know about other
architectures well enough to say that they have the same atomicity
guarantees when it comes to A/D bits.

> I'll double-check with some Intel folks to make sure I'm not missing
> something.  But, either way, I don't think we should be going to this
> much trouble for the good ol' Xeon Phi.  I doubt there are many still
> around and I *REALLY* doubt they're running new kernels.
>=20
> *If* we need this (and I'm not convinced we do), my first instinct =
would
> be to just do this instead:
>=20
> 	clear_cpu_cap(c, X86_FEATURE_PSE);
>=20
> on KNL systems.  If anyone cares, they know where to find us.

I think that it is not necessary and your understanding of the erratum
is the right one. Let me know if you find it is not the case.

BTW: Thanks for the quick response, and sorry for the time it took me
to send v3.