On Oct 25, 2021, at 4:12 AM, Peter Zijlstra [off-list ref] wrote:

On Thu, Oct 21, 2021 at 05:21:12AM -0700, Nadav Amit wrote:

quoted

+/*
+ * 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;

Would it not be clearer to write the above like:

	/* !PRESENT -> * ; no need for flush */
	if (!pte_present(oldpte))
		return false;

	/* PRESENT -> !PRESENT ; needs flush */
	if (!pte_present(newpte))
		return true;

?

I will change the comment to yours. Thanks.

quoted

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 = 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 = pte_to_swp_entry(oldpte);

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.

That is, would "needs" not be a better name?

The “may” 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.