| Message ID | 20220505064429.2818496-1-minchan@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm: fix is_pinnable_page against on cma page | expand |
On Wed, 4 May 2022 23:44:29 -0700 Minchan Kim <minchan@kernel.org> 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)
On 07.05.22 21:23, Andrew Morton wrote: > On Wed, 4 May 2022 23:44:29 -0700 Minchan Kim <minchan@kernel.org> 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? Yes, we want READ_ONCE I think. Apart from that LGTM.
diff --git a/include/linux/mm.h b/include/linux/mm.h index 6acca5cecbc5..e77758e2035e 100644 --- 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 + + return !(is_zone_movable_page(page) || is_zero_pfn(page_to_pfn(page))); } #else static inline bool is_pinnable_page(struct page *page)
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 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. Cc: David Hildenbrand <david@redhat.com> Signed-off-by: Minchan Kim <minchan@kernel.org> --- * 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 | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)