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

[Zi Yan <ziy@nvidia.com>]

On 13 May 2025, at 7:32, Brendan Jackman wrote:

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

Right on time. :)

>
> 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?

You are right. NR_PAGEBLOCK_BITS must divide the word size. I will fix
the commit log in the next version.

>
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +	PB_migrate_isolate = 7, /* If set the block is isolated */
>> +			/* set it to 7 to make pageblock bit word aligned */
I will fix this comment too.

>> +#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.

Sure. Will add it.

>
>>
>> +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?

Based on my observation, the callers all have page and pfn, so using the
current implementation would save a call to page_to_pfn(). I can add a comment
to this function.

/*
 * Use get_pageblock_migratetype() if caller already has both @page and @pfn
 * to save a call to page_to_pfn().
 */

>
>>  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;
>>  }


Best Regards,
Yan, Zi