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;
?
> 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?
