Message-ID: <20230807014229.GB69206@system.software.com>
Date:   Mon, 7 Aug 2023 10:42:29 +0900
From:   Byungchul Park <byungchul@...com>
To:     Nadav Amit <namit@...are.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>,
        "kernel_team@...ynix.com" <kernel_team@...ynix.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "ying.huang@...el.com" <ying.huang@...el.com>,
        "xhao@...ux.alibaba.com" <xhao@...ux.alibaba.com>,
        "mgorman@...hsingularity.net" <mgorman@...hsingularity.net>,
        Hugh Dickins <hughd@...gle.com>,
        Matthew Wilcox <willy@...radead.org>,
        David Hildenbrand <david@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [RFC 2/2] mm: Defer TLB flush by keeping both src and dst folios
 at migration

On Fri, Aug 04, 2023 at 05:32:30PM +0000, Nadav Amit wrote:
> > On Aug 3, 2023, at 11:18 PM, Byungchul Park <byungchul@...com> wrote:
> > 
> > Implementation of CONFIG_MIGRC that stands for 'Migration Read Copy'.
> > 
> > We always face the migration overhead at either promotion or demotion,
> > while working with tiered memory e.g. CXL memory and found out TLB
> > shootdown is a quite big one that is needed to get rid of if possible.
> > 
> > Fortunately, TLB flush can be defered or even skipped if both source and
> > destination of folios during migration are kept until all TLB flushes
> > required will have been done, of course, only if the target PTE entries
> > have read only permission, more precisely speaking, don't have write
> > permission. Otherwise, no doubt the folio might get messed up.
> > 
> > To achieve that:
> > 
> >   1. For the folios that have only non-writable TLB entries, prevent
> >      TLB flush by keeping both source and destination of folios during
> >      migration, which will be handled later at a better time.
> > 
> >   2. When any non-writable TLB entry changes to writable e.g. through
> >      fault handler, give up CONFIG_MIGRC mechanism so as to perform
> >      TLB flush required right away.
> > 
> >   3. TLB flushes can be skipped if all TLB flushes required to free the
> >      duplicated folios have been done by any reason, which doesn't have
> >      to be done from migrations.
> > 
> >   4. Adjust watermark check routine, __zone_watermark_ok(), with the
> >      number of duplicated folios because those folios can be freed
> >      and obtained right away through appropreate TLB flushes.
> > 
> >   5. Perform TLB flushes and free the duplicated folios pending the
> >      flushes if page allocation routine is in trouble due to memory
> >      pressure, even more aggresively for high order allocation.
> 
> So I think that what you want to do may be possible, but I think it worth
> checking once an RFC that can be reviewed is posted. The complexity and
> overheads would then need to be evaluated.
> 
> The patch in its current form, I am afraid, is very very hard to review.
> It is way too big and is missing comments. Having CONFIG_MIGRC makes no

Sorry for that. I will split this patch set into more pieces and try to
add sufficient comments, and then will repost it in the next spin.

> sense (I guess it is intended to be a “chicken-bit”). Variable and

Exactly.

> function names are not informative. The memory barriers are handle

Yeah, 'naming' is the hardest one to do. Lemme try to rename those.

> improperly (please check again the smp_mb__after_atomic() rules).

Thank you. I meant to put a smp_mb() along with atomic_read() after
that, between reading migrc_gen and TLB flush. I will check it more.

> Actually, when it comes to concurrency, there are many things I did not
> understand from a glance at the code when it comes to concurrency: the
> use of llist_add when (I think?) the llist is not shared (I think?); the

A llist isolated for handling TLB flush and freeing folios is a stack
variable so it's not shared. However, there is another type of llist
that is a global llist so as to be shared, that is for collecting
all the requests in the system.

> use of WRITE_ONCE() for synchronization; migrc_gen scheme (and BTW, since
> such a counter might overflow it should be atomic64).

Sure. It would overflow but it's not a big problem because we can
compare between the generation numbers with '(int)(a - b) < 0' trick
unless more than the MAX_INT/2 numbers of requests happen at the same
time. I assumed it's barely gonna happen.

> But much more importantly, going up one level, there are several issues
> that should be addressed/considered/discussed:
> 
> a. It seems to me that when a new PTE is established (e.g., following
>    an mmap()), and there are pending deferred flushes, a full TLB flush
>    would also be required. So your point (2) would need to be extended.

It has nothing to do with tiering migration. So I think It would work as
it was with the original code, say, it would perform TLB flush needed
for the PTE change. I don't think CONFIG_MIGRC needs to consider the
case. Could you explain what would be a probelm in this case in more
detail?

> b. When a reference to the page is taken in other means (get_user_pages()),
>    a TLB flush might also be needed.

All TLB flush would be performed as it was, except TLB flushes at
tiering migration where CONFIG_MIGRC works. I might miss things.. Please
explain in more detail what you think is a problem.

> c. If we start deferring TLB flushes for a long time, and throughout that
>    time many events (TLB flush, page-faults, etc.) might require a *full*
>    TLB flush, that might have negative impact.

I let it work as it was, except tiering migration. It'd help me get you
if you describe in more detail. I might need to make this patch set more
readable first tho. However, I'd like to understand what you are
concerning exactly so that I can answer like either 'you don't have to
worry about that because blur blur' or 'Oh my I will fix it thank you'.

> d. The interactions with other mechanisms that inspect the PTE to make
>    decisions and might not take into account the fact a TLB flush was not
>    done need to be considered. The interaction with mmu_gather has been
>    taken for, but there is a question of whether something here might
>    break it.  

I also think I need to be more careful when it comes to mmu_gather
things. My opinion on this is, it'd be okay becasue CONFIG_MIGRC only
works with tiering migration and let other pathes where TLB flush is
required go as it was. Howerver, it definitely should be considered if
those routines that require TLB flush during tiering migration. Am I
missing something?

> Now there are many things in the patch that need to be addressed and are
> unacceptable in their current form (e.g., migrc_try_flush() flushing
> potentially twice the same cores), but reviewing this patch in its

No. It doens't flush twice cuz it keeps whether each CPU has been TLB
flushed that was requested, or not ;)

> current form is too tedious for me.

I will reform it.

> [ BTW: for future versions, consider cc'ing Peter Zijlstra, Andy
>   Lutomirski and Dave Hansen. ]

I will.

Appreciate all your comments!

	Byungchul

Powered by blists - more mailing lists