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=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.60.0.1.1\))
Subject: Re: [RESEND PATCH v3 4/5] mm/mprotect: do not flush on permission
 promotion
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <20220311190749.338281-5-namit@vmware.com>
Date:   Fri, 11 Mar 2022 14:45:57 -0800
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.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: 7bit
Message-Id: <0FFB7FA7-4F08-47EB-8AAE-400867E96DE4@gmail.com>
References: <20220311190749.338281-1-namit@vmware.com>
 <20220311190749.338281-5-namit@vmware.com>
To:     linux-mm@kvack.org
Precedence: bulk


> On Mar 11, 2022, at 11:07 AM, Nadav Amit <nadav.amit@gmail.com> wrote:
> 
> From: Nadav Amit <namit@vmware.com>
> 
> Currently, using mprotect() to unprotect a memory region or uffd to
> unprotect a memory region causes a TLB flush. At least on x86, as
> protection is promoted, no TLB flush is needed.
> 
> Add an arch-specific pte_may_need_flush() which tells whether a TLB
> flush is needed based on the old PTE and the new one. Implement an x86
> pte_may_need_flush().
> 
> For x86, besides the simple logic that PTE protection promotion or
> changes of software bits does require a flush, also add logic that
> considers the dirty-bit. If the dirty-bit is clear and write-protect is
> set, no TLB flush is needed, as x86 updates the dirty-bit atomically
> on write, and if the bit is clear, the PTE is reread.
> 
> 

[snip]

> + */
> +static inline bool pte_needs_flush(pte_t oldpte, pte_t newpte)
> +{
> +	/* !PRESENT -> * ; no need for flush */
> +	if (!pte_present(oldpte))
> +		return false;
> +
> +	/* PRESENT -> !PRESENT ; needs flush */
> +	if (!pte_present(newpte))
> +		return true;
> +
> +	/* PFN changed ; needs flush */
> +	if (pte_pfn(oldpte) != pte_pfn(newpte))
> +		return true;
> +
> +	return pte_flags_need_flush(pte_flags(oldpte), pte_flags(newpte),
> +				    _PAGE_ACCESSED);
> +}


Looking again at this patch, I think the PRESENT -> !PRESENT is
not needed:

1. It might trigger unnecessary flushes for PROT_NONE (or NUMA)
2. Real PRESENT -> !PRESENT is already checked by
   pte_flags_need_flush().


Let me know if I am missing something. Otherwise I will change it
for v4.