On Wed, 4 May 2022 23:44:29 -0700 Minchan Kim <minchan@xxxxxxxxxx> 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 putting CMA pages longterm pinning possible on
> pin_user_pages APIs so CMA allocation fails.
>
> 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 type. Thus, we don't
> need zone->lock but just checks the migratype in either of
> (MIGRATE_ISOLATE and MIGRATE_CMA).
>
> Adding the MIGRATE_ISOLATE check in is_pinnable_page could cause
> rejecting of pinning the page on MIGRATE_ISOLATE pageblock even
> thouth it's neither CMA nor movable zone if the page is temporarily
"though"
> unmovable. However, the migration failure is general issue, not
> only come from MIGRATE_ISOLATE and the MIGRATE_ISOLATE is also
> transient state like other temporal refcount holding of pages.
>
> ...
>
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1625,8 +1625,18 @@ 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.
> + */
> + volatile int mt = get_pageblock_migratetype(page);
> +
> + if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)
> + return false;
> +#endif
Open-coded use of `volatile' draws unwelcome attention.
What are we trying to do here? Prevent the compiler from rerunning all
of get_pageblock_migratetype() (really __get_pfnblock_flags_mask())
twice? That would be pretty dumb of it?
Would a suitably-commented something like
int __mt = get_pageblock_migratetype(page);
int mt = __READ_ONCE(__mt);
express this better?
> +
> + return !(is_zone_movable_page(page) || is_zero_pfn(page_to_pfn(page)));
> }
> #else
> static inline bool is_pinnable_page(struct page *page)
