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 5/5] mm/mprotect: do not flush on permission promotion
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <YXaRHE0IrJD2lXeg@hirez.programming.kicks-ass.net>
Date:   Mon, 25 Oct 2021 09:27:08 -0700
Cc:     Linux-MM <linux-mm@kvack.org>, LKML <linux-kernel@vger.kernel.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Peter Xu <peterx@redhat.com>,
        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: <94412B55-0E16-4BC1-982F-37C082A0BABD@gmail.com>
References: <20211021122112.592634-1-namit@vmware.com>
 <20211021122112.592634-6-namit@vmware.com>
 <YXaRHE0IrJD2lXeg@hirez.programming.kicks-ass.net>
To:     Peter Zijlstra <peterz@infradead.org>
Precedence: bulk


> On Oct 25, 2021, at 4:12 AM, Peter Zijlstra <peterz@infradead.org> =
wrote:
>=20
> On Thu, Oct 21, 2021 at 05:21:12AM -0700, Nadav Amit wrote:
>> +/*
>> + * pte_may_need_flush() checks whether permissions were demoted and =
require a
>> + * flush. It should only be used for userspace PTEs.
>> + */
>> +static inline bool pte_may_need_flush(pte_t oldpte, pte_t newpte)
>> +{
>> +	/* new is non-present: need only if old is present */
>> +	if (!pte_present(newpte))
>> +		return pte_present(oldpte);
>> +
>> +	/* old is not present: no need for flush */
>> +	if (!pte_present(oldpte))
>> +		return false;
>=20
> Would it not be clearer to write the above like:
>=20
> 	/* !PRESENT -> * ; no need for flush */
> 	if (!pte_present(oldpte))
> 		return false;
>=20
> 	/* PRESENT -> !PRESENT ; needs flush */
> 	if (!pte_present(newpte))
> 		return true;
>=20
> ?

I will change the comment to yours. Thanks.

>=20
>=20
>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>> index 0f5c87af5c60..6179c82ea72d 100644
>> --- a/mm/mprotect.c
>> +++ b/mm/mprotect.c
>> @@ -141,7 +141,8 @@ static unsigned long change_pte_range(struct =
mmu_gather *tlb,
>> 				ptent =3D pte_mkwrite(ptent);
>> 			}
>> 			ptep_modify_prot_commit(vma, addr, pte, oldpte, =
ptent);
>> -			tlb_flush_pte_range(tlb, addr, PAGE_SIZE);
>> +			if (pte_may_need_flush(oldpte, ptent))
>> +				tlb_flush_pte_range(tlb, addr, =
PAGE_SIZE);
>> 			pages++;
>> 		} else if (is_swap_pte(oldpte)) {
>> 			swp_entry_t entry =3D pte_to_swp_entry(oldpte);
>=20
> One question on naming, "may_need" sounds a bit washy to me, either it
> does or it does not. I suppose you're trying to convey the fact that =
we
> ought to err towards too many TLBi rather than too few, but that's
> always true.
>=20
> That is, would "needs" not be a better name?

The =E2=80=9Cmay=E2=80=9D is indeed intended to be clear that the =
function can error
towards too many TLB flushes (of any kind). For instance, in a change
from (!dirty|write)->(!write), no flush is needed in theory. I was too
chicken to add it, at least for now.

I can change the name and indicate in the comment instead though.=