Re: [RFC PATCH] migrate_pages: Never block waiting for the page lock

[Doug Anderson <dianders@chromium.org>]

Hi,

On Tue, Apr 18, 2023 at 5:34 PM Huang, Ying <ying.huang@intel.com> 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.

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.

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?

-Doug