On Fri, Jul 24, 2020 at 12:27 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> It *may* make sense to say "ok, don't bother flushing the TLB if this
> is a retry, because we already did that originally". MAYBE.
That sounds wrong to me too.
Maybe a *BIG* and understandable comment about why the
FAULT_FLAG_TRIED case shouldn't actually do anything at all.
But it does smell like the real issue is that the "else" case for
ptep_set_access_flags() is simply wrong.
Or maybe something else. But this thing from the changelog really
raises my hackles:
"But it seems not necessary to modify those
bits again for #3 since they should be already set by the first page fault
try"
since we just verified that we know _exactly_ what the pte is:
if (unlikely(!pte_same(*vmf->pte, entry))) {
update_mmu_tlb(vmf->vma, vmf->address, vmf->pte);
goto unlock;
}
so there is no "should be already set" case. We have 100% information
about what the current state is.
And if we don't make any changes, then that's exactly when
ptep_set_access_flags() returns zero.
So the real question is "why do we need the
flush_tlb_fix_spurious_fault() thing".
We could say that we never need it at all for FAULT_FLAG_RETRY. That
makes a lot of sense to me.
So a patch that does something like the appended (intentionally
whitespace-damaged) seems sensible.
But note the XYZ in that commit. When do we actually have stale TLB
entries? Do we actually do the lazy "avoid TLB flushing when loosening
the rules" anywhere?
I think that "when it's a write fault" is actually bogus. I could
imagine that code pages could get the same issue. So the
"FAULT_FLAG_RETRY" part of the check makes perfect sense to me, but
the legacy "FAULT_FLAG_WRITE" case I'd actually want to document more.
On x86, we never care about lazy faults. Page faulting will always
update the TLB.
On other architectures, I can see spurious faults happening either due
to lazy reasons, or simply because another core is modifying the page
table right now (ie the concurrent fault thing), but hasn't actually
flushed yet.
Can somebody flesh out the comment about the
"spurious_protection_fault()" thing? Because something like this I
wouldn't mind, but I'd like that comment to explain the
FAULT_FLAG_WRITE part too.
Linus
---
diff --git a/mm/memory.c b/mm/memory.c
index 3ecad55103ad..9994c98d88c3 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4163,6 +4163,26 @@ static vm_fault_t wp_huge_pud(struct vm_fault
*vmf, pud_t orig_pud)
return VM_FAULT_FALLBACK;
}
+/*
+ * If ptep_set_access_flags() returns zero, that means that
+ * it made no changes. Why did we get a fault?
+ *
+ * It might be a spurious protection fault because we at
+ * some point lazily didn't flush a TLB when we only loosened
+ * the protection rules. But it might also be because a
+ * concurrent fault on another CPU had already marked things
+ * young, and our young/dirty changes didn't change anything.
+ *
+ * The lazy TLB optimization only happens when we make things
+ * writable. See XYZ.
+ */
+static inline bool spurious_protection_fault(unsigned int flags)
+{
+ if (flags & FAULT_FLAG_RETRY)
+ return false;
+ return flags & FAULT_FLAG_WRITE;
+}
+
/*
* These routines also need to handle stuff like marking pages dirty
* and/or accessed for architectures that don't do it in hardware (most
@@ -4247,15 +4267,8 @@ static vm_fault_t handle_pte_fault(struct
vm_fault *vmf)
if (ptep_set_access_flags(vmf->vma, vmf->address, vmf->pte, entry,
vmf->flags & FAULT_FLAG_WRITE)) {
update_mmu_cache(vmf->vma, vmf->address, vmf->pte);
- } else {
- /*
- * This is needed only for protection faults but the
arch code
- * is not yet telling us if this is a protection
fault or not.
- * This still avoids useless tlb flushes for .text
page faults
- * with threads.
- */
- if (vmf->flags & FAULT_FLAG_WRITE)
- flush_tlb_fix_spurious_fault(vmf->vma,
vmf->address);
+ } else if (spurious_protection_fault(vmf->flags)) {
+ flush_tlb_fix_spurious_fault(vmf->vma, vmf->address);
}
unlock:
pte_unmap_unlock(vmf->pte, vmf->ptl);
