On Fri, Aug 04, 2023 at 05:32:30PM +0000, Nadav Amit wrote: > > 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 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
