Re: [PATCH v4] mm: introduce a new page type for page pool in page type

[David Hildenbrand (Arm)]

On 5/13/26 14:18, Byungchul Park wrote:
> On Wed, May 13, 2026 at 11:00:51AM +0200, Dragos Tatulea wrote:
>> On 24.02.26 06:13, Byungchul Park wrote:
>>> Currently, the condition 'page->pp_magic == PP_SIGNATURE' is used to
>>> determine if a page belongs to a page pool.  However, with the planned
>>> removal of @pp_magic, we should instead leverage the page_type in struct
>>> page, such as PGTY_netpp, for this purpose.
>>>
>>> Introduce and use the page type APIs e.g. PageNetpp(), __SetPageNetpp(),
>>> and __ClearPageNetpp() instead, and remove the existing APIs accessing
>>> @pp_magic e.g. page_pool_page_is_pp(), netmem_or_pp_magic(), and
>>> netmem_clear_pp_magic().
>>>
>>> Plus, add @page_type to struct net_iov at the same offset as struct page
>>> so as to use the page_type APIs for struct net_iov as well.  While at it,
>>> reorder @type and @owner in struct net_iov to avoid a hole and
>>> increasing the struct size.
>>>
>>> This work was inspired by the following link:
>>>
>>>   https://lore.kernel.org/all/582f41c0-2742-4400-9c81-0d46bf4e8314@xxxxxxxxx/
>>>
>>> While at it, move the sanity check for page pool to on the free path.
>>>
>>> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
>>> Co-developed-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
>>> Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
>>> Signed-off-by: Byungchul Park <byungchul@xxxxxx>
>>> Acked-by: David Hildenbrand <david@xxxxxxxxxx>
>>> Acked-by: Zi Yan <ziy@xxxxxxxxxx>
>>> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
>>> Reviewed-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>>> ---
>>
>> Seems like this patch broke tcp_mmap because
>> validate_page_before_insert() returns -EINVAL due
>> to a page having a type. Here's the full flow:
>>
>> getsockopt(TCP_ZEROCOPY_RECEIVE) returns -EINVAL because of the
>> below flow in the kernel:
>>
>> tcp_zerocopy_receive()
>> -> tcp_zerocopy_vm_insert_batch()
>>   -> vm_insert_pages()
>>     -> insert_pages()
>>       -> insert_page_in_batch_locked()
>>         -> validate_page_before_insert() returns -EINVAL
>>            because page_has_type(page) is now true.
>>
>> The patch below fixes the issue. But is this a valid fix?
> 
> Hi,
> 
> The problem comes from the fact that page_type and _mapcount are
> union'ed but there is a case where these two information should be kept
> at the same time.
> 
> Why don't we allow these two information can be kept in the 4 bytes at
> the same time until Zi Yan's work on _mapcount and page_type will be
> done, instead of taking a step back?
> 
> It can be more optimized but I suggest the approach I just mentioned:
> ---
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 64dc44832808..e5ec204866dc 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -185,8 +185,7 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>  {
>  	int mapcount = atomic_read(&page->_mapcount) + 1;
>  
> -	if (page_mapcount_is_type(mapcount))
> -		mapcount = 0;
> +	mapcount = page_mapcount_clear_type(mapcount);
>  	if (folio_test_large(folio))
>  		mapcount += folio_entire_mapcount(folio);
>  
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8260e28205e9..f45064796313 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1865,8 +1865,7 @@ static inline int folio_mapcount(const struct folio *folio)
>  
>  	if (likely(!folio_test_large(folio))) {
>  		mapcount = atomic_read(&folio->_mapcount) + 1;
> -		if (page_mapcount_is_type(mapcount))
> -			mapcount = 0;
> +		mapcount = page_mapcount_clear_type(mapcount);
>  		return mapcount;
>  	}
>  	return folio_large_mapcount(folio);
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 0e03d816e8b9..f3b0d1fa262d 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -934,9 +934,9 @@ static inline bool page_type_has_type(int page_type)
>  }
>  
>  /* This takes a mapcount which is one more than page->_mapcount */
> -static inline bool page_mapcount_is_type(unsigned int mapcount)
> +static inline unsigned int page_mapcount_clear_type(unsigned int mapcount)
>  {
> -	return page_type_has_type(mapcount - 1);
> +	return (unsigned int)(((int)(mapcount << 8)) >> 8);
>  }
>  
>  static inline bool page_has_type(const struct page *page)
> @@ -953,16 +953,20 @@ static __always_inline void __folio_set_##fname(struct folio *folio)	\
>  {									\
>  	if (folio_test_##fname(folio))					\
>  		return;							\
> -	VM_BUG_ON_FOLIO(data_race(folio->page.page_type) != UINT_MAX,	\
> +	VM_BUG_ON_FOLIO(page_type_has_type(data_race(folio->page.page_type)), \
>  			folio);						\
> -	folio->page.page_type = (unsigned int)PGTY_##lname << 24;	\
> +	folio->page.page_type &= ~(PGTY_mapcount_underflow << 24);	\
> +	folio->page.page_type |= (unsigned int)PGTY_##lname << 24;	\
>  }									\
>  static __always_inline void __folio_clear_##fname(struct folio *folio)	\
>  {									\
> -	if (folio->page.page_type == UINT_MAX)				\
> +	int mapcount;							\
> +									\
> +	if (!page_type_has_type(folio->page.page_type))			\
>  		return;							\
>  	VM_BUG_ON_FOLIO(!folio_test_##fname(folio), folio);		\
> -	folio->page.page_type = UINT_MAX;				\
> +	mapcount = atomic_read(&folio->page._mapcount);			\
> +	folio->page.page_type = page_mapcount_clear_type(mapcount);	\
>  }
>  
>  #define PAGE_TYPE_OPS(uname, lname, fname)				\
> @@ -975,15 +979,20 @@ static __always_inline void __SetPage##uname(struct page *page)		\
>  {									\
>  	if (Page##uname(page))						\
>  		return;							\
> -	VM_BUG_ON_PAGE(data_race(page->page_type) != UINT_MAX, page);	\
> -	page->page_type = (unsigned int)PGTY_##lname << 24;		\
> +	VM_BUG_ON_PAGE(page_type_has_type(data_race(page->page_type)),	\
> +				page);					\
> +	page->page_type &= ~(PGTY_mapcount_underflow << 24);		\
> +	page->page_type |= (unsigned int)PGTY_##lname << 24;		\
>  }									\
>  static __always_inline void __ClearPage##uname(struct page *page)	\
>  {									\
> -	if (page->page_type == UINT_MAX)				\
> +	int mapcount;							\
> +									\
> +	if (!page_type_has_type(page->page_type))			\
>  		return;							\
>  	VM_BUG_ON_PAGE(!Page##uname(page), page);			\
> -	page->page_type = UINT_MAX;					\
> +	mapcount = atomic_read(&page->_mapcount);			\
> +	page->page_type = page_mapcount_clear_type(mapcount);		\
>  }
>  
>  /*
> diff --git a/mm/debug.c b/mm/debug.c
> index 77fa8fe1d641..9a932ded09d4 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -74,8 +74,7 @@ static void __dump_folio(const struct folio *folio, const struct page *page,
>  	int mapcount = atomic_read(&page->_mapcount) + 1;
>  	char *type = "";
>  
> -	if (page_mapcount_is_type(mapcount))
> -		mapcount = 0;
> +	mapcount = page_mapcount_clear_type(mapcount);
>  
>  	pr_warn("page: refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
>  			folio_ref_count(folio), mapcount, mapping,
> ---
> 
> Thoughts?

God no.

-- 
Cheers,

David