Re: [PATCH 4/6] mm: proc: Invalidate TLB after clearing soft-dirty page state

On Mon, Nov 23, 2020 at 06:41:14PM +0000, Will Deacon wrote:

On Fri, Nov 20, 2020 at 07:55:14AM -0800, Minchan Kim wrote:

quoted

On Fri, Nov 20, 2020 at 04:00:23PM +0100, Peter Zijlstra wrote:

quoted

On Fri, Nov 20, 2020 at 02:35:55PM +0000, Will Deacon wrote:

quoted

Since commit 0758cd830494 ("asm-generic/tlb: avoid potential double flush"),
TLB invalidation is elided in tlb_finish_mmu() if no entries were batched
via the tlb_remove_*() functions. Consequently, the page-table modifications
performed by clear_refs_write() in response to a write to
/proc/<pid>/clear_refs do not perform TLB invalidation. Although this is
fine when simply aging the ptes, in the case of clearing the "soft-dirty"
state we can end up with entries where pte_write() is false, yet a
writable mapping remains in the TLB.

Fix this by calling tlb_remove_tlb_entry() for each entry being
write-protected when cleating soft-dirty.

quoted

@@ -1053,6 +1054,7 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
 		ptent = pte_wrprotect(old_pte);
 		ptent = pte_clear_soft_dirty(ptent);
 		ptep_modify_prot_commit(vma, addr, pte, old_pte, ptent);
+		tlb_remove_tlb_entry(tlb, pte, addr);
 	} else if (is_swap_pte(ptent)) {
 		ptent = pte_swp_clear_soft_dirty(ptent);
 		set_pte_at(vma->vm_mm, addr, pte, ptent);

Oh!

Yesterday when you had me look at this code; I figured the sane thing
to do was to make it look more like mprotect().

Why did you chose to make it work with mmu_gather instead? I'll grant
you that it's probably the smaller patch, but I still think it's weird
to use mmu_gather here.

I agree. The reason why clear_refs_write used the gather API was [1] and
seems like to overkill to me.

I don't see why it's overkill. Prior to that commit, it called
flush_tlb_mm() directly.

The TLB gather was added for increasing tlb flush pending count for
stability bug, not for performance optimiataion(The commit never had
any number to support it and didn't have the logic to handle each pte
with tlb gather) and then it introduced a bug now so I take it as overkill
since it made complication from the beginning *unnecessary*.

quoted

We could just do like [inc|dec]_tlb_flush_pending with flush_tlb_mm at
right before dec_tlb_flush_pending instead of gather.

thought?

I'm not sure why this is better; it's different to the madvise() path, and
will need special logic to avoid the flush in the case where we're just
doing aging.

I thought it's better to fix the bug first as *simple* patch and then
do optimization based on it.
Anyway, following to Yu's comment, we don't need gather API and 
even the flush if we give up the accuarcy(but I want to have it).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel