Re: [PATCH v4 10/16] mm/page-flags: check page/folio->private instead of PG_private

["David Hildenbrand (Arm)" <david@kernel.org>]

On 9/14/26 04:24, Zi Yan wrote:
> After the changes of the prior commits, page/folio->private != NULL is now
> equivalent to checking PG_private.
> 
> Stop checking PG_private on pages and folios and use page/folio->private
> instead, except swapcache and hugetlb folios, because the former uses a
> field (swp_entry_t swap) overlapping with ->private and the latter sets its
> flags in ->private. Exclude swapcache and hugetlb when the code is meant to
> check PG_private only. PG_swapcache and folio->swap.val cannot be set/clear
> as a whole, so excluding swapcache with folio_test_swapcache() is not
> reliable. Instead, use folio_test_swapbacked(), since PG_swapbacked is
> stable when a folio is added to/removed from swapcache. Add a helper,
> folio_has_attached_private(), for this check.
> 
> folio_expected_ref_count() can be called without folio lock, so annotate
> folio_test_private() with data_race() to avoid triggering race condition
> checks. While at it, annotate folio->mapping too. Add data_race()
> annotation for other lockless callers too.

Shouldn't we just move the data_race() into folio_has_attached_private() and
document there, that it can be called without the folio lock as well? Having the
data_race in multiple callers looks odd.

> 
> folio_set/clear_private() and Set/ClearPagePrivate() become no-ops.
> PG_private is no longer checked at page free time.

You can mention here that we will clean this part up next, to remove these
helpers entirely.

[...]

>  	}
>  
>  	/* One reference per page table mapping. */
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 462e89e055485..08988877331ba 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -577,7 +577,23 @@ FOLIO_FLAG(swapbacked, FOLIO_HEAD_PAGE)
>   * for its own purposes.
>   * - PG_private and PG_private_2 cause release_folio() and co to be invoked
>   */
> -PAGEFLAG(Private, private, PF_ANY)
> +
> +static __always_inline bool folio_test_private(const struct folio *folio)
> +{
> +	return folio->private;
> +}
> +
> +static __always_inline int PagePrivate(const struct page *page)
> +{
> +	return !!page->private;
> +}
> +
> +/* no-ops during transition */
> +static __always_inline void folio_set_private(struct folio *folio) { }
> +static __always_inline void folio_clear_private(struct folio *folio) { }
> +static __always_inline void SetPagePrivate(struct page *page) { }
> +static __always_inline void ClearPagePrivate(struct page *page) { }
> +
>  FOLIO_FLAG(private_2, FOLIO_HEAD_PAGE)
>  
>  /* owner_2 can be set on tail pages for anon memory */
> @@ -1169,7 +1185,7 @@ static __always_inline void __ClearPageAnonExclusive(struct page *page)
>   */
>  #define PAGE_FLAGS_CHECK_AT_FREE				\
>  	(1UL << PG_lru		| 1UL << PG_locked	|	\
> -	 1UL << PG_private	| 1UL << PG_private_2	|	\
> +	 1UL << PG_private_2	|				\
>  	 1UL << PG_writeback	| 1UL << PG_reserved	|	\
>  	 1UL << PG_active 	|				\
>  	 1UL << PG_unevictable	| __PG_MLOCKED | LRU_GEN_MASK)
> @@ -1193,8 +1209,28 @@ static __always_inline void __ClearPageAnonExclusive(struct page *page)
>  	(0xffUL /* order */		| 1UL << PG_has_hwpoisoned |	\
>  	 1UL << PG_large_rmappable	| 1UL << PG_partially_mapped)
>  
> -#define PAGE_FLAGS_PRIVATE				\
> -	(1UL << PG_private | 1UL << PG_private_2)
> +/**
> + * folio_has_attached_private - check if the folio has private data attached
> + * @folio: The folio to check.
> + *
> + * Use this in code that may encounter swapcache or hugetlb folios but only
> + * wants to detect attached private data. Swapcache stores swp_entry_t in
> + * folio->swap, a union with folio->private, and hugetlb stores its own flags
> + * in folio->private; both are excluded.

This is an implementation detail that is irrelevant for callers. Move that above
the actual checks in the code below.

> + *
> + * NOTE: For swapcache, folio->swap.val PG_swapcache are not set as a whole,
> + * so folio_test_swapcache() is not reliable to exclude swapcache.
> + * Use folio_test_swapbacked() instead, since it remains set when a folio is
> + * added to/removed from swapcache.

Dito, a caller of this function doesn't care.

> + *
> + * Return: true if folio->private is set and the folio is neither swapcache
> + * nor hugetlb.

Can we remove these implementation details here instead and talk about "true if
the folio has private data attached" ?

> + */
> +static inline bool folio_has_attached_private(const struct folio *folio)
> +{
> +	return folio_test_private(folio) && !folio_test_swapbacked(folio) &&
> +	       !folio_test_hugetlb(folio);
> +}
Apart from that LGTM.

-- 
Cheers,

David