Re: [PATCH v2 5/5] mm/mprotect: do not flush on permission promotion
From: Peter Zijlstra <peterz@infradead.org>
Date: 2021-10-25 11:12:42
Also in:
lkml
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;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; ?
quoted hunk ↗ jump to hunk
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?