Message-ID: <857d21da-5de2-fa3e-b1ce-41cc1cfb0191@nvidia.com>
Date:   Tue, 10 May 2022 15:56:37 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Minchan Kim <minchan@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
CC:     linux-mm <linux-mm@...ck.org>, LKML <linux-kernel@...r.kernel.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        John Dias <joaodias@...gle.com>,
        "David Hildenbrand" <david@...hat.com>
Subject: Re: [PATCH v4] mm: fix is_pinnable_page against on cma page

On 5/10/22 14:17, Minchan Kim wrote:
> Pages on CMA area could have MIGRATE_ISOLATE as well as MIGRATE_CMA
> so current is_pinnable_page could miss CMA pages which has MIGRATE_
> ISOLATE. It ends up pinning CMA pages as longterm at pin_user_pages
> APIs so CMA allocation keep failed until the pin is released.
> 
> 
>       CPU 0                                   CPU 1 - Task B
> 
> cma_alloc
> alloc_contig_range
> 					pin_user_pages_fast(FOLL_LONGTERM)
> change pageblock as MIGRATE_ISOLATE
> 					internal_get_user_pages_fast
>                                          lockless_pages_from_mm
>                                          gup_pte_range
>                                          try_grab_folio
>                                          is_pinnable_page
>                                            return true;
>                                          So, pinned the page successfully.
> page migration failure with pinned page
> 					..
>                                          .. After 30 sec
> 					unpin_user_page(page)
> 
> CMA allocation succeeded after 30 sec.

Hi Minchan,

Thanks for spelling out how this works, that really speeds up the
review and helps others quickly learn what is going on with the code.

For my own information, mainly: where is CMA blocking, so that
it waits (apparently) for the during of the pin, before proceeding?
(Or is the caller retrying?)

I noticed a few minor points but was too slow to reply, notes below:

> 
> The CMA allocation path protects the migration type change race
> using zone->lock but what GUP path need to know is just whether the
> page is on CMA area or not rather than exact migration type.
> Thus, we don't need zone->lock but just checks migration type in
> either of (MIGRATE_ISOLATE and MIGRATE_CMA).
> 
> Adding the MIGRATE_ISOLATE check in is_pinnable_page could cause
> rejecting of pinning pages on MIGRATE_ISOLATE pageblocks even
> though it's neither CMA nor movable zone if the page is temporarily
> unmovable. However, such a migration failure by unexpected temporal
> refcount holding is general issue, not only come from MIGRATE_ISOLATE
> and the MIGRATE_ISOLATE is also transient state like other temporal
> elevated refcount problem.
> 
> Cc: David Hildenbrand <david@...hat.com>
> Signed-off-by: Minchan Kim <minchan@...nel.org>
> ---
> * from v3 - https://lore.kernel.org/all/20220509153430.4125710-1-minchan@kernel.org/
>    * Fix typo and adding more description - akpm
> 
> * from v2 - https://lore.kernel.org/all/20220505064429.2818496-1-minchan@kernel.org/
>    * Use __READ_ONCE instead of volatile - akpm
> 
> * from v1 - https://lore.kernel.org/all/20220502173558.2510641-1-minchan@kernel.org/
>    * fix build warning - lkp
>    * fix refetching issue of migration type
>    * add side effect on !ZONE_MOVABLE and !MIGRATE_CMA in description - david
> 
>   include/linux/mm.h | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6acca5cecbc5..cbf79eb790e0 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1625,8 +1625,19 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
>   #ifdef CONFIG_MIGRATION
>   static inline bool is_pinnable_page(struct page *page)
>   {
> -	return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
> -		is_zero_pfn(page_to_pfn(page));
> +#ifdef CONFIG_CMA
> +	/*
> +	 * use volatile to use local variable mt instead of
> +	 * refetching mt value.
> +	 */

This comment is stale and should therefore be deleted.

> +	int __mt = get_pageblock_migratetype(page);
> +	int mt = __READ_ONCE(__mt);

Although I saw the email discussion about this in v2, that discussion
didn't go far enough. It started with "don't use volatile", and went
on to "try __READ_ONCE() instead", but it should have continued on
to "you don't need this at all".

Because you don't. There is nothing you are racing with, and adding
__READ_ONCE() in order to avoid a completely not-going-to-happen
compiler re-invocation of a significant code block is just very wrong.

So let's just let it go entirely. :)

> +
> +	if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)

MIGRATE_ISOLATE is not always defined, and must therefore be protected
with a check on CONFIG_MEMORY_ISOLATION...oh never mind, I see in
mm/Kconfig:

config CMA
	...
	select MEMORY_ISOLATION

...so that's OK. What a tangled web, I wonder if enum migratetype
really needs to be sliced up like that, but that's a separate topic.

> +		return false;
> +#endif
> +
> +	return !(is_zone_movable_page(page) || is_zero_pfn(page_to_pfn(page)));

And actually this area is getting rather nested with the various ifdefs,
and it is probably time to refactor them a bit, considering the above
point about MIGRATE_ISOLATE. I had something in mind (which is why I
delayed my feedback), along the lines of merging _ISOLATE and _CMA and
the ifdefs. But it's just a fine point and not critical of course, just
a thought.

>   }
>   #else
>   static inline bool is_pinnable_page(struct page *page)


thanks,
-- 
John Hubbard
NVIDIA

Powered by blists - more mailing lists