Re: [RFC 2/2] mm: Defer TLB flush by keeping both src and dst folios at migration

[Nadav Amit]

> On Aug 3, 2023, at 11:18 PM, Byungchul Park <byungchul@xxxxxx> 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
sense (I guess it is intended to be a “chicken-bit”). Variable and
function names are not informative. The memory barriers are handle
improperly (please check again the smp_mb__after_atomic() rules).

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
use of WRITE_ONCE() for synchronization; migrc_gen scheme (and BTW, since
such a counter might overflow it should be atomic64).

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.

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

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.

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.  

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
current form is too tedious for me.

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