Re: [PATCH v3 1/4] mm/page_isolation: make page isolation a standalone bit.

[Johannes Weiner <hannes@cmpxchg.org>]

On Thu, May 08, 2025 at 03:17:05PM -0400, Zi Yan wrote:
> 
> >>> @@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
> >>>  		     migratetype < MIGRATE_PCPTYPES))
> >>>  		migratetype = MIGRATE_UNMOVABLE;
> >>>
> >>> -	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
> >>> +#ifdef CONFIG_MEMORY_ISOLATION
> >>> +	if (migratetype == MIGRATE_ISOLATE)
> >>> +		set_pageblock_isolate(page);
> >>
> >> Are there paths actually doing this after the second patch?
> >>
> >> There are many instances that want to *read* the migratetype or
> >> MIGRATE_ISOLATE, but only isolation code should be manipulating that
> >> bit through the dedicated set/toggle_pageblock_isolate API.
> >>
> >> If there isn't one, it might be good to enforce this with a VM_WARN
> >> instead.
> >
> > I checked all set_pageblock_migratetype() callers and do not see
> > one using it for pageblock isolation. Let me replace the code
> > with a VM_WARN and add a comment to tell users to use dedicated
> > pageblock isolation APIs.
> >
> 
> Actually, move_freepages_block_isolate() calls __move_freepages_block()
> to move free pages to MIGRATE_ISOLATE pageblock and
> set_pageblock_migratetype() is used inside __move_freepages_block().
> So the branch has to stay. Will use the suggestion below.

Ah, good catch. But looking at the callers, it's:

move_freepages_block()
move_freepages_block_isolate()
try_to_claim_block()

The last one would benefit from having the set_pageblock_migratetype()
there explicitly, as this is what this function is supposed to do. It
also should never set the isolation bit.

move_freepages_block_isolate() has two set_pageblock_migratetype()
calls already. And after the series, it should only manipulate the
isolate bit, not change the actual migratetype anymore, right?

Maybe it makes the most sense to move it into the three callers?

And then fortify set_pageblock_migratetype() after all.