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?
