Re: [PATCH] mm/migrate: fix race between lock page and clear PG_Isolated

[Matthew Wilcox <willy@infradead.org>]

On Tue, Mar 15, 2022 at 11:05:15AM +0800, Andrew Yang wrote:
> When memory is tight, system may start to compact memory for large
> continuous memory demands. If one process tries to lock a memory page
> that is being locked and isolated for compaction, it may wait a long time
> or even forever. This is because compaction will perform non-atomic
> PG_Isolated clear while holding page lock, this may overwrite PG_waiters
> set by the process that can't obtain the page lock and add itself to the
> waiting queue to wait for the lock to be unlocked.
> 
> CPU1                            CPU2
> lock_page(page); (successful)
>                                 lock_page(); (failed)
> __ClearPageIsolated(page);      SetPageWaiters(page) (may be overwritten)
> unlock_page(page);
> 
> The solution is to not perform non-atomic operation on page flags while
> holding page lock.
> 
> Signed-off-by: andrew.yang <andrew.yang@mediatek.com>
> ---
>  include/linux/page-flags.h |  2 +-
>  mm/migrate.c               | 12 ++++++------
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 1c3b6e5c8bfd..64a84a9835cb 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -918,7 +918,7 @@ PAGE_TYPE_OPS(Guard, guard)
>  
>  extern bool is_free_buddy_page(struct page *page);
>  
> -__PAGEFLAG(Isolated, isolated, PF_ANY);
> +PAGEFLAG(Isolated, isolated, PF_ANY);

Agreed.  Further, page cannot be a tail page (this is implied by the
get_page_unless_zero() as tailpages have a zero refcount, and it
is assumed by __PageMovable() as page->mapping is undefined for tail
pages).  So this can actually be:

+PAGEFLAG(Isolated, isolated, PF_NO_TAIL);

I considered PF_ONLY_HEAD, but there are a lot more places that _check_
PageIsolated() and I don't want to prove that they're all definitely
working on head pages.