Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim <minchan@kernel.org>]

On Tue, May 03, 2022 at 11:08:25AM -0700, Minchan Kim wrote:
< snip >

Ccing Paul really this time.

Attach original code for Paul.

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6acca5cecbc5..f59bbe3296e3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,10 @@ 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));
+	int mt = get_pageblock_migratetype(page);
+
+	return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
+		mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
 }
 #else
 static inline bool is_pinnable_page(struct page *page)
--
2.36.0.464.gb9c8b46e94-goog


> > >>>
> > >>> A thing to get some attention is whether we need READ_ONCE or not
> > >>> for the local variable mt.
> > >>>
> > >>
> > >> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> > >> there is anything stopping the compiler from re-reading the value. But
> > >> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> > >> something in between.
> > > 
> > > How about this?
> > > 
> > >      CPU A                                                      CPU B
> > > 
> > > is_pinnable_page
> > >   ..
> > >   ..                                                set_pageblock_migratetype(MIGRATE_ISOLATE)
> > >   mt == MIGRATE_CMA
> > >     get_pageblock_miratetype(page)
> > >         returns MIGRATE_ISOLATE
> > >   mt == MIGRATE_ISOLATE                             set_pageblock_migratetype(MIGRATE_CMA)
> > >     get_pageblock_miratetype(page)
> > >         returns MIGRATE_CMA
> > >  
> > > So both conditions fails to detect it.
> > 
> > I think you're right. That's nasty.
> 
> Ccing Paul to borrow expertise. :)
> 
> int mt = get_pageblock_migratetype(page);
> 
> if (mt == MIGRATE_CMA)
>   return true;
> 
> if (mt == MIGRATE_ISOLATE)
>   return true;
> 
> I'd like to keep use the local variable mt's value in folloing
> conditions checks instead of refetching the value from
> get_pageblock_migratetype.
> 
> What's the right way to achieve it?
> 
> Thanks in advance!

Paul, could you give any hint?