On 7/24/20 1:22 PM, Linus Torvalds wrote:
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.
Thanks a lot for looking into this. It does make sense to me.
A follow-up question about your comment in the previous email "The
notion of "this is a retry, so let's do nothing" is fundamentally
wrong.", do you mean it is not safe? Or since we have pte_same check, we
should just rely on it to skip unnecessary TLB flush?
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.
I'm not quite familiar with other architectures, my wild guess is
FAULT_FLAG_WRITE is a cheap way to tell us if this is a .text page or
not. The original commit which introduced the check said so as well:
commit 1a44e149084d772a1bcf4cdbdde8a013a8a1cfde
Author: Andrea Arcangeli <andrea@xxxxxxx>
Date: Sat Oct 29 18:16:48 2005 -0700
[PATCH] .text page fault SMP scalability optimization
We had a problem on ppc64 where with more than 4 threads a large system
wouldn't scale well while faulting in the .text (most of the time
was spent
in the kernel despite it was an userland compute intensive app). The
reason is the useless overwrite of the same pte from all cpu.
I fixed it this way (verified on an older kernel but the forward
port is
almost identical). This will benefit all archs not just ppc64.
Signed-off-by: Andrea Arcangeli <andrea@xxxxxxx>
Cc: Hugh Dickins <hugh@xxxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxx>
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);
