> On Oct 25, 2021, at 4:12 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> 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;
>
> ?
I will change the comment to yours. Thanks.
>
>
>> 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.
