Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [PATCH v2 2/5] mm: avoid unnecessary flush on change_huge_pmd()
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <4f604380-a52b-660c-af82-541dbd7652e4@intel.com>
Date:   Tue, 26 Oct 2021 13:07:37 -0700
Cc:     Linux-MM <linux-mm@kvack.org>, LKML <linux-kernel@vger.kernel.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        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" <x86@kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <640A6374-A06B-4E20-BF5D-9A21CC85CB12@gmail.com>
References: <20211021122112.592634-1-namit@vmware.com>
 <20211021122112.592634-3-namit@vmware.com>
 <c415820a-aebb-265c-7f47-e048ee429102@intel.com>
 <E38AEB97-DE1B-4C91-A959-132EC24812AE@vmware.com>
 <29E7E8A4-C400-40A5-ACEC-F15C976DDEE0@gmail.com>
 <435f41f2-ffd4-0278-9f26-fbe2c2c7545c@intel.com>
 <8BC74789-FF33-403F-B5D7-19034CAC7EE6@gmail.com>
 <4f604380-a52b-660c-af82-541dbd7652e4@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Precedence: bulk


> On Oct 26, 2021, at 12:40 PM, Dave Hansen <dave.hansen@intel.com> =
wrote:
>=20
> On 10/26/21 12:06 PM, Nadav Amit wrote:
>>=20
>> To make it very clear - consider the following scenario, in which
>> a volatile pointer p is mapped using a certain PTE, which is RW
>> (i.e., *p is writable):
>>=20
>>  CPU0				CPU1
>>  ----				----
>>  x =3D *p
>>  [ PTE cached in TLB;=20
>>    PTE is not dirty ]
>> 				clear_pte(PTE)
>>  *p =3D x
>>  [ needs to set dirty ]
>>=20
>> Note that there is no TLB flush in this scenario. The question
>> is whether the write access to *p would succeed, setting the
>> dirty bit on the clear, non-present entry.
>>=20
>> I was under the impression that the hardware AD-assist would
>> recheck the PTE atomically as it sets the dirty bit. But, as I
>> said, I am not sure anymore whether this is defined architecturally
>> (or at least would work in practice on all CPUs modulo the=20
>> Knights Landing thingy).
>=20
> Practically, at "x=3D*p", he thing that gets cached in the TLB will
> Dirty=3D0.  At the "*p=3Dx", the CPU will decide it needs to do a =
write,
> find the Dirty=3D0 entry and will entirely discard it.  In other =
words, it
> *acts* roughly like this:
>=20
> 	x =3D *p			=09
> 	INVLPG(p)
> 	*p =3D x;
>=20
> Where the INVLPG() and the "*p=3Dx" are atomic.  So, there's no
> _practical_ problem with your scenario.  This specific behavior isn't
> architectural as far as I know, though.
>=20
> Although it's pretty much just academic, as for the architecture, are
> you getting hung up on the difference between the description of =
"Accessed":
>=20
> 	Whenever the processor uses a paging-structure entry as part of
> 	linear-address translation, it sets the accessed flag in that
> 	entry
>=20
> and "Dirty:"
>=20
> 	Whenever there is a write to a linear address, the processor
> 	sets the dirty flag (if it is not already set) in the paging-
> 	structure entry...
>=20
> Accessed says "as part of linear-address translation", which means =
that
> the address must have a translation.  But, the "Dirty" section doesn't
> say that.  It talks about "a write to a linear address" but not =
whether
> there is a linear address *translation* involved.
>=20
> If that's it, we could probably add a bit like:
>=20
> 	In addition to setting the accessed flag, whenever there is a
> 	write...
>=20
> before the dirty rules in the SDM.
>=20
> Or am I being dense and continuing to miss your point? :)

I think this time you got my question right.

I was thrown off by the SDM comment on RW permissions vs dirty that I
mentioned before:

"If software on one logical processor writes to a page while software on
 another logical processor concurrently clears the R/W flag in the
 paging-structure entry that maps the page, execution on some processors =
may
 result in the entry=E2=80=99s dirty flag being set (due to the write on =
the first
 logical processor) and the entry=E2=80=99s R/W flag being clear (due to =
the update
 to the entry on the second logical processor).=E2=80=9D

I did not pay enough attention to these small differences that you =
mentioned
between access and dirty this time (although I did notice them before).

I do not think that the change that you offered to the SDM really =
clarifies
the situation. Setting the access flag is done as part of caching the =
PTE in
the TLB. The SDM change you propose does not clarify the atomicity of =
the
permission/PTE-validity check and dirty-bit setting or the fact the PTE =
is
invalidated if the dirty-bit needs to be set and is cached as clear [I =
do not
presume you would want the latter in the SDM, since it is an =
implementation
detail.]

I just wonder how come the R/W-clearing and the P-clearing cause =
concurrent
dirty bit setting to behave differently. I am not a hardware guy, but I =
would
imagine they would be the same...