[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7023C1AF-6C9B-4544-8EC4-0BB790C1E338@vmware.com>
Date: Fri, 4 Aug 2023 17:32:30 +0000
From: Nadav Amit <namit@...are.com>
To: Byungchul Park <byungchul@...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 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
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. ]
Powered by blists - more mailing lists