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

[Brendan Jackman <jackmanb@google.com>]

Hi Zi,

I hope you don't mind me jumping in on a late revision like this...

On Fri May 9, 2025 at 8:01 PM UTC, Zi Yan wrote:
> During page isolation, the original migratetype is overwritten, since
> MIGRATE_* are enums and stored in pageblock bitmaps. Change
> MIGRATE_ISOLATE to be stored a standalone bit, PB_migrate_isolate, like
> PB_migrate_skip, so that migratetype is not lost during pageblock
> isolation. pageblock bits needs to be word aligned, so expand
> the number of pageblock bits from 4 to 8 and make PB_migrate_isolate bit 7.

Forgive my ignorance but can you help me confirm if I'm following this -
Do you just mean that NR_PAGEBLOCK_BITS must divide the word size? Or is
there something else going on here?

> +#ifdef CONFIG_MEMORY_ISOLATION
> +	PB_migrate_isolate = 7, /* If set the block is isolated */
> +			/* set it to 7 to make pageblock bit word aligned */
> +#endif

I feel I'm always just asking for commentary so please feel free to
complain if this is annoying. But I think it would be worth adding the 
context from the commit message into the code here (or somewhere else),
e.g:

/*
 * Page isolation is represented with a separate bit, so that the other
 * bits can store the migratetype that the block had before it was
 * isolated.
 */

Just adding in that detail about the intent will help readers a lot IMO.

>  
> +unsigned long get_pageblock_migratetype(const struct page *page)
> +{
> +	unsigned long flags;
> +
> +	flags = get_pfnblock_flags_mask(page, page_to_pfn(page),
> +			MIGRATETYPE_MASK);
> +#ifdef CONFIG_MEMORY_ISOLATION
> +	if (flags & PB_migrate_isolate_bit)
> +		return MIGRATE_ISOLATE;
> +#endif
> +	return flags;
> +}

Can we just do get_pageblock_migratetype(page, page_to_pfn(page)) here?

>  static __always_inline int get_pfnblock_migratetype(const struct page *page,
>  					unsigned long pfn)
>  {
> -	return get_pfnblock_flags_mask(page, pfn, MIGRATETYPE_MASK);
> +	unsigned long flags;
> +
> +	flags = get_pfnblock_flags_mask(page, pfn,
> +			MIGRATETYPE_MASK);
> +#ifdef CONFIG_MEMORY_ISOLATION
> +	if (flags & PB_migrate_isolate_bit)
> +		return MIGRATE_ISOLATE;
> +#endif
> +	return flags;
>  }