Re: [PATCH v11 07/14] mm: multi-gen LRU: exploit locality in rmap

[Barry Song]

On Fri, Jun 17, 2022 at 9:56 AM Yu Zhao <yuzhao@xxxxxxxxxx> wrote:
>
> On Wed, Jun 8, 2022 at 4:46 PM Barry Song <21cnbao@xxxxxxxxx> wrote:
> >
> > On Thu, Jun 9, 2022 at 3:52 AM Linus Torvalds
> > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Jun 7, 2022 at 5:43 PM Barry Song <21cnbao@xxxxxxxxx> wrote:
> > > >
> > > > Given we used to have a flush for clear pte young in LRU, right now we are
> > > > moving to nop in almost all cases for the flush unless the address becomes
> > > > young exactly after look_around and before ptep_clear_flush_young_notify.
> > > > It means we are actually dropping flush. So the question is,  were we
> > > > overcautious? we actually don't need the flush at all even without mglru?
> > >
> > > We stopped flushing the TLB on A bit clears on x86 back in 2014.
> > >
> > > See commit b13b1d2d8692 ("x86/mm: In the PTE swapout page reclaim case
> > > clear the accessed bit instead of flushing the TLB").
> >
> > This is true for x86, RISC-V, powerpc and S390. but it is not true for
> > most platforms.
> >
> > There was an attempt to do the same thing in arm64:
> > https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1793830.html
> > but arm64 still sent a nosync tlbi and depent on a deferred to dsb :
> > https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1794484.html
>
> Barry, you've already answered your own question.
>
> Without commit 07509e10dcc7 arm64: pgtable: Fix pte_accessible():
>    #define pte_accessible(mm, pte)        \
>   -       (mm_tlb_flush_pending(mm) ? pte_present(pte) : pte_valid_young(pte))
>   +       (mm_tlb_flush_pending(mm) ? pte_present(pte) : pte_valid(pte))
>
> You missed all TLB flushes for PTEs that have gone through
> ptep_test_and_clear_young() on the reclaim path. But most of the time,
> you got away with it, only occasional app crashes:
> https://lore.kernel.org/r/CAGsJ_4w6JjuG4rn2P=d974wBOUtXUUnaZKnx+-G6a8_mSROa+Q@xxxxxxxxxxxxxx/
>
> Why?

Yes. On the arm64 platform, ptep_test_and_clear_young() without flush
can cause random
App to crash.
ptep_test_and_clear_young() + flush won't have this kind of crashes though.
But after applying commit 07509e10dcc7 arm64: pgtable: Fix
pte_accessible(), on arm64,
ptep_test_and_clear_young() without flush won't cause App to crash.

ptep_test_and_clear_young(), with flush, without commit 07509e10dcc7:   OK
ptep_test_and_clear_young(), without flush, with commit 07509e10dcc7:   OK
ptep_test_and_clear_young(), without flush, without commit 07509e10dcc7:   CRASH

So is it possible that other platforms have similar problems with
arm64 while commit
07509e10dcc7 isn't there? but anyway, we depend on those platforms which can
really use mglru to expose this kind of potential bugs.

BTW, do you think it is safe to totally remove the flush code even for
the original
LRU? I don't see fundamental difference between MGLRU and LRU on this
"flush" thing. Since MGLRU doesn't need flush, why does LRU need it? flush
is very expensive, if we do think this flush is unnecessary, will we remove
it for the original LRU as well?

BTW, look_around is a great idea. Our experiments also show some great
decrease on the cpu consumption of page_referenced().

Thanks
Barry