Re: [PATCH RFC v2 03/11] mm/page_owner: preserve accounting with countable stack depot records

[Marco Elver <elver@google.com>]

On Tue, Sep 08, 2026 at 02:13PM +0100, Caleb Kan wrote:
> From: Caleb Kan <ckan@cloudflare.com>
> 
> page_owner keeps stable struct stack_record pointers in its stack list.
> It uses each record's count with a one-count bias to track live base pages
> and reads the stored entries directly. A later patch adds a trie backend,
> which provides neither a flat record layout nor an independent count field.
> 
> Add STACK_DEPOT_FLAG_COUNTABLE to keep these records on the hash backend
> and deduplicate them separately from all non-countable records. Store the
> discriminator in a new u16 flags field and narrow size to u16. This keeps
> the record header size unchanged while covering the configured maximum of
> 256 frames. Do not otherwise split hash-table deduplication: ordinary and
> GET saves can continue to share records. Make COUNTABLE mutually exclusive
> with GET because the two flags assign incompatible meanings to the record
> count. Reject countable records in stack_depot_put() and require COUNTABLE
> in __stack_depot_get_stack_record().
> 
> Mark both page_owner save sites countable so its existing accounting and
> reporting continue to use stable hash records. Extend the KUnit coverage to
> verify direct-record access and isolation between countable and
> non-countable records.
> 
> Signed-off-by: Caleb Kan <ckan@cloudflare.com>
> ---
>  include/linux/stackdepot.h   | 28 +++++++++++++++-------
>  lib/Kconfig.debug            |  3 ++-
>  lib/stackdepot.c             | 20 +++++++++++++++-
>  lib/tests/stackdepot_kunit.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
>  mm/page_owner.c              |  6 +++--
>  5 files changed, 100 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index 734529767c8a..7ff67c70d727 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -53,7 +53,8 @@ union handle_parts {
>  struct stack_record {
>  	struct list_head hash_list;	/* Links in the hash table */
>  	u32 hash;			/* Hash in hash table */
> -	u32 size;			/* Number of stored frames */
> +	u16 size;			/* Number of stored frames */
> +	u16 flags;
>  	union handle_parts handle;	/* Constant after initialization */
>  	refcount_t count;
>  	union {
> @@ -84,8 +85,9 @@ typedef u32 depot_flags_t;
>   */
>  #define STACK_DEPOT_FLAG_CAN_ALLOC	((depot_flags_t)0x0001)
>  #define STACK_DEPOT_FLAG_GET		((depot_flags_t)0x0002)
> +#define STACK_DEPOT_FLAG_COUNTABLE	((depot_flags_t)0x0004)
>  
> -#define STACK_DEPOT_FLAGS_NUM	2
> +#define STACK_DEPOT_FLAGS_NUM	3
>  #define STACK_DEPOT_FLAGS_MASK	((depot_flags_t)((1 << STACK_DEPOT_FLAGS_NUM) - 1))
>  
>  /*
> @@ -144,6 +146,11 @@ static inline int stack_depot_early_init(void)	{ return 0; }
>   * Users of this flag must also call stack_depot_put() when keeping the stack
>   * trace is no longer required to avoid overflowing the refcount.
>   *
> + * If STACK_DEPOT_FLAG_COUNTABLE is set in @depot_flags, stack depot stores the
> + * stack in hash-backed storage for callers that need direct stack_record count
> + * access. This flag does not imply %STACK_DEPOT_FLAG_CAN_ALLOC and is mutually
> + * exclusive with %STACK_DEPOT_FLAG_GET.
> + *
>   * If the provided stack trace comes from the interrupt context, only the part
>   * up to the interrupt entry is saved.
>   *
> @@ -178,11 +185,12 @@ depot_stack_handle_t stack_depot_save(unsigned long *entries,
>  				      unsigned int nr_entries, gfp_t alloc_flags);
>  
>  /**
> - * __stack_depot_get_stack_record - Get a pointer to a stack_record struct
> + * __stack_depot_get_stack_record - Get a hash-backed stack record
>   *
>   * @handle: Stack depot handle
>   *
> - * This function is only for internal purposes.
> + * This function is only for internal purposes. @handle must have been saved
> + * with %STACK_DEPOT_FLAG_COUNTABLE.
>   *
>   * Return: Returns a pointer to a stack_record struct
>   */
> @@ -260,10 +268,14 @@ int stack_depot_snprint(depot_stack_handle_t handle, char *buf, size_t size,
>   *
>   * @handle:	Stack depot handle returned from stack_depot_save()
>   *
> - * The stack trace is evicted from stack depot once all references to it have
> - * been dropped (once the number of stack_depot_evict() calls matches the
> - * number of stack_depot_save_flags() calls with STACK_DEPOT_FLAG_GET set for
> - * this stack trace).
> + * Drop a reference acquired by stack_depot_save_flags() with
> + * %STACK_DEPOT_FLAG_GET. Calling this for a handle saved without
> + * %STACK_DEPOT_FLAG_GET is invalid; persistent handles are owned by stack depot
> + * for the lifetime of the system.
> + *
> + * The stack trace is evicted once the number of stack_depot_put() calls matches
> + * the number of successful stack_depot_save_flags() calls with
> + * %STACK_DEPOT_FLAG_GET for this stack trace.
>   */
>  void stack_depot_put(depot_stack_handle_t handle);
>  
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index fcd74edfd93a..3a78c67b6b36 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2792,7 +2792,8 @@ config STACKDEPOT_KUNIT_TEST
>  	default KUNIT_ALL_TESTS
>  	help
>  	  Enable this option to test stack depot API behavior at boot.
> -	  This test is built in, so KUNIT must also be built in.
> +	  This test is built in because it exercises internal, non-exported
> +	  stack depot helpers, so KUNIT must also be built in.
>  
>  	  KUnit tests run during boot and output the results to the debug log
>  	  in TAP format (https://testanything.org/). Only useful for kernel
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 4da7279d9f83..66c5e8594566 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -94,6 +94,7 @@ static const char *const counter_names[] = {
>  	[DEPOT_COUNTER_PERSIST_BYTES]	= "persistent_bytes",
>  };
>  static_assert(ARRAY_SIZE(counter_names) == DEPOT_COUNTER_COUNT);
> +static_assert(CONFIG_STACKDEPOT_MAX_FRAMES <= U16_MAX);
>  
>  static int __init disable_stack_depot(char *str)
>  {
> @@ -467,6 +468,7 @@ depot_alloc_stack(unsigned long *entries, unsigned int nr_entries, u32 hash, dep
>  	/* Save the stack trace. */
>  	stack->hash = hash;
>  	stack->size = nr_entries;
> +	stack->flags = flags & STACK_DEPOT_FLAG_COUNTABLE;
>  	/* stack->handle is already filled in by depot_pop_free_pool(). */
>  	memcpy(stack->entries, entries, flex_array_size(stack, entries, nr_entries));
>  
> @@ -609,6 +611,9 @@ static inline struct stack_record *find_stack(struct list_head *bucket,
>  	list_for_each_entry_rcu(stack, bucket, hash_list) {
>  		if (stack->hash != hash || stack->size != size)
>  			continue;
> +		/* Page owner countable records have a distinct count lifetime. */
> +		if ((stack->flags ^ flags) & STACK_DEPOT_FLAG_COUNTABLE)
> +			continue;

This splits hash deduplication unconditionally even when
stackdepot.trie_enabled=0 (the default), and even for page_owner's
free_handle traces (__reset_page_owner()), which never touch
stack_record->count. Prior to this patch, persistent hash records had
refcount_set(&stack->count, REFCOUNT_SATURATED) and page_owner safely
overwrote it with its 1-biased page count while still sharing records
with non-GET callers.

The interface looks error-prone to me.