Let's allow for not clearing a page type before freeing a page to the
buddy.

We'll focus on having a 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).

Signed-off-by: David Hildenbrand <david@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;
+
 	if (is_check_pages_enabled()) {
 		if (free_page_is_bad(page))
 			bad++;
-- 
2.49.0

On 18 Jun 2025, at 13:39, David Hildenbrand wrote:

> Let's allow for not clearing a page type before freeing a page to the
> buddy.
>
> We'll focus on having a 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).
>
> Signed-off-by: David Hildenbrand <david@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;
> +
>  	if (is_check_pages_enabled()) {
>  		if (free_page_is_bad(page))
>  			bad++;

Should we be pedantic to only do this for PageOffline and PageZsmalloc
and warn for the rest page types?

Something like:

if (unlikely(page_has_type(page))) {
	if (PageOffline(page) || PageZsmalloc(page))
		page->page_type = UINT_MAX;
	else
		VM_WARN_ONCE_PAGE(1, page);
}

Best Regards,
Yan, Zi

On 18.06.25 20:43, Zi Yan wrote:
> On 18 Jun 2025, at 13:39, David Hildenbrand wrote:
> 
>> Let's allow for not clearing a page type before freeing a page to the
>> buddy.
>>
>> We'll focus on having a 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).
>>
>> Signed-off-by: David Hildenbrand <david@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;
>> +
>>   	if (is_check_pages_enabled()) {
>>   		if (free_page_is_bad(page))
>>   			bad++;
> 
> Should we be pedantic to only do this for PageOffline and PageZsmalloc
> and warn for the rest page types?

I think we should just allow any page types. Limiting it to specific 
types sounds like some use-after-free check that probably shouldn't be 
handled that way.

-- 
Cheers,

David / dhildenb

On 18 Jun 2025, at 13:39, David Hildenbrand wrote:

> Let's allow for not clearing a page type before freeing a page to the
> buddy.
>
> We'll focus on having a 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).
>
> Signed-off-by: David Hildenbrand <david@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;
> +
>  	if (is_check_pages_enabled()) {
>  		if (free_page_is_bad(page))
>  			bad++;
> -- 
> 2.49.0

How does this preserve page type? Isn’t page->page_type = UINT_MAX clearing
page_type?

Best Regards,
Yan, Zi

On 18 Jun 2025, at 14:04, Zi Yan wrote:

> On 18 Jun 2025, at 13:39, David Hildenbrand wrote:
>
>> Let's allow for not clearing a page type before freeing a page to the
>> buddy.
>>
>> We'll focus on having a 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).
>>
>> Signed-off-by: David Hildenbrand <david@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;
>> +
>>  	if (is_check_pages_enabled()) {
>>  		if (free_page_is_bad(page))
>>  			bad++;
>> -- 
>> 2.49.0
>
> How does this preserve page type? Isn’t page->page_type = UINT_MAX clearing
> page_type?

OK, next patch explains it. free_pages_prepare() clears page_type,
so that caller does not need to.

I think the message is better to be

mm/page_alloc: clear page_type at page free time

page_type is no longer needed to be cleared before a page is freed, as
page free code does that.

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).

Best Regards,
Yan, Zi

On 18.06.25 20:08, Zi Yan wrote:
> On 18 Jun 2025, at 14:04, Zi Yan wrote:
> 
>> On 18 Jun 2025, at 13:39, David Hildenbrand wrote:
>>
>>> Let's allow for not clearing a page type before freeing a page to the
>>> buddy.
>>>
>>> We'll focus on having a 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).
>>>
>>> Signed-off-by: David Hildenbrand <david@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;
>>> +
>>>   	if (is_check_pages_enabled()) {
>>>   		if (free_page_is_bad(page))
>>>   			bad++;
>>> -- 
>>> 2.49.0
>>
>> How does this preserve page type? Isn’t page->page_type = UINT_MAX clearing
>> page_type?
> 
> OK, next patch explains it. free_pages_prepare() clears page_type,
> so that caller does not need to.
> 
> I think the message is better to be
> 
> mm/page_alloc: clear page_type at page free time
> 
> page_type is no longer needed to be cleared before a page is freed, as
> page free code does that.
> 
> 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).


I'll change it to

     mm/page_alloc: let page freeing clear any set page type
     
     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).
     



-- 
Cheers,

David / dhildenb

On Mon, Jun 23, 2025 at 05:26:05PM +0200, David Hildenbrand wrote:
> On 18.06.25 20:08, Zi Yan wrote:
> > On 18 Jun 2025, at 14:04, Zi Yan wrote:
> > 
> > > On 18 Jun 2025, at 13:39, David Hildenbrand wrote:
> > > 
> > > > Let's allow for not clearing a page type before freeing a page to the
> > > > buddy.
> > > > 
> > > > We'll focus on having a 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).
> > > > 
> > > > Signed-off-by: David Hildenbrand <david@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;
> > > > +
> > > >   	if (is_check_pages_enabled()) {
> > > >   		if (free_page_is_bad(page))
> > > >   			bad++;
> > > > -- 
> > > > 2.49.0
> > > 
> > > How does this preserve page type? Isn’t page->page_type = UINT_MAX clearing
> > > page_type?
> > 
> > OK, next patch explains it. free_pages_prepare() clears page_type,
> > so that caller does not need to.
> > 
> > I think the message is better to be
> > 
> > mm/page_alloc: clear page_type at page free time
> > 
> > page_type is no longer needed to be cleared before a page is freed, as
> > page free code does that.
> > 
> > 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).
> 
> 
> I'll change it to
> 
>     mm/page_alloc: let page freeing clear any set page type
>     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).

Acked-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry / Hyeonggon

On 23 Jun 2025, at 11:26, David Hildenbrand wrote:

> On 18.06.25 20:08, Zi Yan wrote:
>> On 18 Jun 2025, at 14:04, Zi Yan wrote:
>>
>>> On 18 Jun 2025, at 13:39, David Hildenbrand wrote:
>>>
>>>> Let's allow for not clearing a page type before freeing a page to the
>>>> buddy.
>>>>
>>>> We'll focus on having a 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).
>>>>
>>>> Signed-off-by: David Hildenbrand <david@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;
>>>> +
>>>>   	if (is_check_pages_enabled()) {
>>>>   		if (free_page_is_bad(page))
>>>>   			bad++;
>>>> -- 
>>>> 2.49.0
>>>
>>> How does this preserve page type? Isn’t page->page_type = UINT_MAX clearing
>>> page_type?
>>
>> OK, next patch explains it. free_pages_prepare() clears page_type,
>> so that caller does not need to.
>>
>> I think the message is better to be
>>
>> mm/page_alloc: clear page_type at page free time
>>
>> page_type is no longer needed to be cleared before a page is freed, as
>> page free code does that.
>>
>> 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).
>
>
> I'll change it to
>
>     mm/page_alloc: let page freeing clear any set page type
>         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).

Thanks.

Reviewed-by: Zi Yan <ziy@nvidia.com>

--
Best Regards,
Yan, Zi

On Wed, Jun 18, 2025 at 02:04:18PM -0400, Zi Yan wrote:
> > Let's allow for not clearing a page type before freeing a page to the
> > buddy.
> >
> > We'll focus on having a 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).

> > +	if (unlikely(page_has_type(page)))
> > +		page->page_type = UINT_MAX;
> > +
> >  	if (is_check_pages_enabled()) {
> >  		if (free_page_is_bad(page))
> >  			bad++;
> > -- 
> > 2.49.0
> 
> How does this preserve page type? Isn’t page->page_type = UINT_MAX clearing
> page_type?

The point is that the _caller_ used to have to clear the page type.
This patch allows the caller to free the page without clearing
the page type first.

On 18 Jun 2025, at 14:06, Matthew Wilcox wrote:

> On Wed, Jun 18, 2025 at 02:04:18PM -0400, Zi Yan wrote:
>>> Let's allow for not clearing a page type before freeing a page to the
>>> buddy.
>>>
>>> We'll focus on having a 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).
>
>>> +	if (unlikely(page_has_type(page)))
>>> +		page->page_type = UINT_MAX;
>>> +
>>>  	if (is_check_pages_enabled()) {
>>>  		if (free_page_is_bad(page))
>>>  			bad++;
>>> -- 
>>> 2.49.0
>>
>> How does this preserve page type? Isn’t page->page_type = UINT_MAX clearing
>> page_type?
>
> The point is that the _caller_ used to have to clear the page type.
> This patch allows the caller to free the page without clearing
> the page type first.

Yep, find that out when I read the next patch.

I think the change is fine, but the commit message is unclear.

Best Regards,
Yan, Zi