Doug Anderson <dianders@xxxxxxxxxxxx> writes: > Hi, > > On Tue, Apr 18, 2023 at 5:34 PM Huang, Ying <ying.huang@xxxxxxxxx> wrote: >> >> >> >> >> TBH, the test case is too extreme for me. >> >> >> > >> >> >> > That's fair. That being said, I guess the point I was trying to make >> >> >> > is that waiting for this lock could take an unbounded amount of time. >> >> >> > Other parts of the system sometimes hold a page lock and then do a >> >> >> > blocking operation. At least in the case of kcompactd there are better >> >> >> > uses of its time than waiting for any given page. >> >> >> > >> >> >> >> And, we have multiple "sync" mode to deal with latency requirement, for >> >> >> >> example, we use MIGRATE_SYNC_LIGHT for compaction to avoid too long >> >> >> >> latency. If you have latency requirement for some users, you may >> >> >> >> consider to add new "sync" mode. >> >> >> > >> >> >> > Sure. kcompactd_do_work() is currently using MIGRATE_SYNC_LIGHT. I >> >> >> > guess my first thought would be to avoid adding a new mode and make >> >> >> > MIGRATE_SYNC_LIGHT not block here. Then anyone that truly needs to >> >> >> > wait for all the pages to be migrated can use the heavier sync modes. >> >> >> > It seems to me like the current users of MIGRATE_SYNC_LIGHT would not >> >> >> > want to block for an unbounded amount of time here. What do you think? >> >> >> >> >> >> It appears that you can just use MIGRATE_ASYNC if you think the correct >> >> >> behavior is "NOT block at all". I found that there are more >> >> >> fine-grained controls on this in compaction code, please take a look at >> >> >> "enum compact_priority" and its comments. >> >> > >> >> > Actually, the more I think about it the more I think the right answer >> >> > is to keep kcompactd as using MIGRATE_SYNC_LIGHT and make >> >> > MIGRATE_SYNC_LIGHT not block on the folio lock. >> >> >> >> Then, what is the difference between MIGRATE_SYNC_LIGHT and >> >> MIGRATE_ASYNC? >> > >> > Aren't there still some differences even if we remove blocking this >> > one lock? ...or maybe your point is that maybe the other differences >> > have similar properties? >> >> Sorry for confusing words. Here, I asked you to list the implementation >> difference between MIGRATE_ASYNC and MIGRATE_SYNC_LIGHT after your >> proposed changes. Which are waited in MIGRATE_SYNC_LIGHT but not in >> MIGRATE_ASYNC? > > Ah, got it! It's not always the easiest to follow all the code paths, > but let's see what I can find. > > I guess to start with, though, I will assert that someone seems to > have believed that there was an important difference between > MIGRATE_ASYNC and MIGRATE_SYNC_LIGHT besides waiting on the lock in > migrate_folio_unmapt() since (as I found in my previous digging) the > "direct reclaim" path never grabs this lock but explicitly sometimes > chooses MIGRATE_ASYNC some times and MIGRATE_SYNC_LIGHT other times. > > OK, so looking at mainline Linux and comparing differences in behavior > between SYNC_LIGHT and ASYNC and thoughts about which one should be > used for kcompactd. Note that I won't go _too_ deep into all the > differences... > > -- > > In nfs.c: > > 1. We will wait for the fscache if SYNC_LIGHT but not ASYNC. No idea > what would be the most ideal for calls from kcompactd. This appears like something like disk writing. > In compaction.c: > > 2. We will update the non-async "compact_cached_migrate_pfn" for > SYNC_LIGHT but not ASYNC since we keep track of sync and async > progress separately. > > 3. compact_lock_irqsave() note contentions for ASYNC but not > SYNC_LIGHT and cause an earlier abort. Seems like kcompactd would want > the SYNC_LIGHT behavior since this isn't about things indefinitely > blocking. > > 4. isolate_migratepages_block() will bail if too_many_isolated() for > ASYNC but not SYNC_LIGHT. My hunch is that kcompactd wants the > SYNC_LIGHT behavior for kcompact. > > 5. If in direct compaction, isolate_migratepages_block() sets > "skip_on_failure" for ASYNC but not SYNC_LIGHT. My hunch is that > kcompactd wants the SYNC_LIGHT behavior for kcompact. > > 6. suitable_migration_source() considers more things suitable > migration sources when SYNC_LIGHT but not (ASYNC+direct_compaction). > Doesn't matter since kcompactd isn't direct compaction and non-direct > compaction is the same. > > 7. fast_isolate_around() does less scanning when SYNC_LIGHT but not > (ASYNC+direct_compaction). Again, it doesn't matter for kcompactd. > > 8. isolate_freepages() uses a different stride with SYNC_LIGHT vs. > ASYNC. My hunch is that kcompactd wants the SYNC_LIGHT behavior for > kcompact. > > In migrate.c: > > 9. buffer_migrate_lock_buffers() will block waiting to lock buffers > with SYNC_LIGHT but not ASYNC. I don't know for sure, but this feels > like something we _wouldn't_ want to block on in kcompactd and instead > should look for easier pickings. IIUC, this is similar as page lock. > 10. migrate_folio_unmap() has the case we've already talked about > > 11. migrate_folio_unmap() does batch flushes for async because it > doesn't need to worry about a class of deadlock. Somewhat recent code > actually ends up running this code first even for sync modes to get > the batch. > > 12. We'll retry a few more times for SYNC_LIGHT than ASYNC. Seems like > the more retries won't really hurt for kcompactd. > > -- > > So from looking at all the above, I'll say that kcompactd should stick > with SYNC_LIGHT and we should fix #10. In other words, like my > original patch except that we keep blocking on the lock in the full > SYNC modes. > > It's possible that we should also change case #9 I listed above. Do > you know if locking buffers is likely to block on something as slow as > page reading/writing? IIUC, this is related to page reading/writing. Buffer head is used by ext2/4 to read/write. Thank you very much for your research. It looks like ASYNC isn't appropriate for kcompactd. >From the comments of SYNC_LIGHT, * MIGRATE_SYNC_LIGHT in the current implementation means to allow blocking * on most operations but not ->writepage as the potential stall time * is too significant To make SYNC_LIGHT block on less operations than before, I guess that you need to prove the stall time can be long with the operation with not-so-extreme test cases. Best Regards, Huang, Ying
