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

[Zi Yan <ziy@nvidia.com>]

On 19 May 2025, at 12:42, David Hildenbrand wrote:

>>>> +#ifdef CONFIG_MEMORY_ISOLATION
>>>> +	if (flags & PB_migrate_isolate_bit)
>>>> +		return MIGRATE_ISOLATE;
>>>> +#endif
>>>
>>> If you call get_pfnblock_flags_mask() with MIGRATETYPE_MASK, how could you ever get PB_migrate_isolate_bit?
>>
>> MIGRATETYPE_MASK is ((BIT(PB_migratetype_bits) - 1) | PB_migrate_isolate_bit),
>> so it gets PB_migrate_isolate_bit.
>>
>
> Oh ... that's confusing.
>
>>>
>>>
>>> I think what we should do is
>>>
>>> 1) Rename get_pfnblock_flags_mask() to get_pfnblock_flags()
>>>
>>> 2) Remove the mask parameter
>>>
>>> 3) Perform the masking in all callers.
>>
>> get_pfnblock_flags_mask() is also used by get_pageblock_skip() to
>> get PB_migrate_skip. I do not think we want to include PB_migrate_skip
>> in the mask to confuse readers.
>
> The masking will be handled in the caller.
>
> So get_pageblock_skip() would essentially do a
>
> return get_pfnblock_flags() & PB_migrate_skip_bit;
>
> etc.

Got it. Sounds good to me. Will do this.

>
>>
>>>
>>>
>>>
>>> Maybe, we should convert set_pfnblock_flags_mask() to
>>>
>>> void set_clear_pfnblock_flags(struct page *page, unsigned long
>>> 			      set_flags, unsigned long clear_flags);
>>>
>>> And better, splitting it up (or providing helpers)
>>>
>>> set_pfnblock_flags(struct page *page, unsigned long flags);
>>> clear_pfnblock_flags(struct page *page, unsigned long flags);
>>>
>>>
>>> This implies some more code cleanups first that make the code easier to extend.
>>>
>>
>> The same due to PB_migrate_skip.
>>
>> Based on your suggestion, we could make {set,get}_pfnblock_flags_mask()
>> internal APIs by prepending "__". They are only used by the new
>> {get, set, clear}_pfnblock_flags() and {get, set, clear}_pageblock_{skip, isolate}().
>> Then use {get, set, clear}_pfnblock_flags() for all migratetype operations.
>>
>> WDYT?
>
> In general, lgtm. I just hope we can avoid the "_mask" part and just handle it in these functions directly?

Sounds good to me. Will put this and
"#define NR_PAGEBLOCK_BITS (roundup_pow_of_two(__NR_PAGEBLOCK_BITS))"
in a cleanup patch before Patch 1.

>
>>
>>>> +	return flags;
>>>>    }
>>>>     /**
>>>> @@ -402,8 +423,14 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
>>>>    	unsigned long bitidx, word_bitidx;
>>>>    	unsigned long word;
>>>>   +#ifdef CONFIG_MEMORY_ISOLATION
>>>> +	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 8);
>>>> +	/* extra one for MIGRATE_ISOLATE */
>>>> +	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits) + 1);
>>>> +#else
>>>>    	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);
>>>>    	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits));
>>>> +#endif
>>>>     	bitmap = get_pageblock_bitmap(page, pfn);
>>>>    	bitidx = pfn_to_bitidx(page, pfn);
>>>> @@ -426,6 +453,13 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
>>>>    		     migratetype < MIGRATE_PCPTYPES))
>>>>    		migratetype = MIGRATE_UNMOVABLE;
>>>>   +#ifdef CONFIG_MEMORY_ISOLATION
>>>> +	if (migratetype == MIGRATE_ISOLATE) {
>>>> +		set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,
>>>> +				page_to_pfn(page), PB_migrate_isolate_bit);
>>>> +		return;
>>>> +	}
>>>> +#endif
>>>>    	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>>>>    				page_to_pfn(page), MIGRATETYPE_MASK);
>>>>    }
>>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>>> index b2fc5266e3d2..751e21f6d85e 100644
>>>> --- a/mm/page_isolation.c
>>>> +++ b/mm/page_isolation.c
>>>> @@ -15,6 +15,17 @@
>>>>    #define CREATE_TRACE_POINTS
>>>>    #include <trace/events/page_isolation.h>
>>>>   +static inline bool __maybe_unused get_pageblock_isolate(struct page *page)
>>>> +{
>>>> +	return get_pfnblock_flags_mask(page, page_to_pfn(page),
>>>> +			PB_migrate_isolate_bit);
>>>> +}
>>>> +static inline void clear_pageblock_isolate(struct page *page)
>>>> +{
>>>> +	set_pfnblock_flags_mask(page, 0, page_to_pfn(page),
>>>> +			PB_migrate_isolate_bit);
>>>> +}
>>>
>>> Should these reside in include/linux/pageblock-flags.h, just like the
>>> CONFIG_COMPACTION "skip" variants?
>>
>> They are only used inside mm/page_isolation.c, so I would leave them
>> here until other users come out.
>
> get_pageblock_skip() and friends are also only used in mm/compaction.c.
>
> Having these simple wrapper as inline functions in the same header should make it consistent.
>
> ... and avoid tricks like "__maybe_unused" here :)

OK, will do this.

--
Best Regards,
Yan, Zi