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

From: Minchan Kim <minchan@kernel.org>
To: Will Deacon <will@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	linux-kernel@vger.kernel.org, kernel-team@android.com,
	Catalin Marinas <catalin.marinas@arm.com>,
	Yu Zhao <yuzhao@google.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 4/6] mm: proc: Invalidate TLB after clearing soft-dirty page state
Date: Wed, 25 Nov 2020 14:51:25 -0800	[thread overview]
Message-ID: <20201125225125.GE1484898@google.com> (raw)
In-Reply-To: <20201123184113.GD11688@willie-the-truck>

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:
> > On Fri, Nov 20, 2020 at 04:00:23PM +0100, Peter Zijlstra wrote:
> > > On Fri, Nov 20, 2020 at 02:35:55PM +0000, Will Deacon wrote:
> > > > 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.
> > > > 
> > > 
> > > > @@ -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*.

> 
> > 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).