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

[Zi Yan <ziy@nvidia.com>]

On 14 Sep 2026, at 9:26, David Hildenbrand (Arm) wrote:

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

Not all callers call it without holding a lock. Before the patch series,
it is fine for lockless reads because PagePrivate() is an atomic operation.
But after the patch series, lockless reads can cause data tearing of
folio->private. I think I need to add this to avoid the confusion.

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

Will do.

> [...]
>
>>  	}
>>
>>  	/* 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.

OK.

>
>> + *
>> + * 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.

Got it.

>
>> + *
>> + * 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" ?

OK.

>
>> + */
>> +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.

Thanks.


Best Regards,
Yan, Zi