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