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
