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
