[PATCH v1 04/29] mm/page_alloc: let page freeing clear any set page type

[David Hildenbrand]

On 01.07.25 10:27, Lorenzo Stoakes wrote:
> On Tue, Jul 01, 2025 at 10:17:13AM +0200, David Hildenbrand wrote:
>> On 30.06.25 17:27, Lorenzo Stoakes wrote:
>>> On Mon, Jun 30, 2025 at 02:59:45PM +0200, David Hildenbrand wrote:
>>>> Currently, any user of page types must clear that type before freeing
>>>> a page back to the buddy, otherwise we'll run into mapcount related
>>>> sanity checks (because the page type currently overlays the page
>>>> mapcount).
>>>>
>>>> Let's allow for not clearing the page type by page type users by letting
>>>> the buddy handle it instead.
>>>>
>>>> We'll focus on having a page type set on the first page of a larger
>>>> allocation only.
>>>>
>>>> With this change, we can reliably identify typed folios even though
>>>> they might be in the process of getting freed, which will come in handy
>>>> in migration code (at least in the transition phase).
>>>>
>>>> In the future we might want to warn on some page types. Instead of
>>>> having an "allow list", let's rather wait until we know about once that
>>>> should go on such a "disallow list".
>>>
>>> Is the idea here to get this to show up on folio dumps or?
>>
>> As part of the netmem_desc series, there was a discussion about removing the
>> mystical PP checks -- page_pool_page_is_pp() in page_alloc.c and replacing
>> them by a proper page type check.
>>
>> In that case, we would probably want to warn in case we get such a netmem
>> page unexpectedly freed.
>>
>> But, that page type does not exist yet in code, so the sanity check must be
>> added once introduced.
> 
> OK, and I realise that the UINT_MAX thing is a convention for how a reset
> page_type looks anyway now.
> 
>>
>>>
>>>>
>>>> Reviewed-by: Zi Yan <ziy at nvidia.com>
>>>> Acked-by: Harry Yoo <harry.yoo at oracle.com>
>>>> Signed-off-by: David Hildenbrand <david at redhat.com>
>>>> ---
>>>>    mm/page_alloc.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index 858bc17653af9..44e56d31cfeb1 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -1380,6 +1380,9 @@ __always_inline bool free_pages_prepare(struct page *page,
>>>>    			mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
>>>>    		page->mapping = NULL;
>>>>    	}
>>>> +	if (unlikely(page_has_type(page)))
>>>> +		page->page_type = UINT_MAX;
>>>
>>> Feels like this could do with a comment!
>>
>> /* Reset the page_type -> _mapcount to -1 */
> 
> Hm this feels like saying 'the reason we set it to -1 is to set it to -1' :P

Bingo! Guess why I didn't add a comment in the first place :P

> 
> I'd be fine with something simple like
> 
> /* Set page_type to reset value */

"Reset the page_type (which overlays _mapcount)"

?

 > > But... Can't we just put a #define somewhere here to make life 
easy? Like:

Given that Willy will change all that soon, I'm not in favor of doing 
that in this series.

-- 
Cheers,

David / dhildenb