Received-SPF: pass (google.com: domain of linux-kernel+bounces-59083-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240208234539.19113-1-osalvador@suse.de> <20240208234539.19113-3-osalvador@suse.de>
In-Reply-To: <20240208234539.19113-3-osalvador@suse.de>
From: Marco Elver <elver@google.com>
Date: Fri, 9 Feb 2024 08:45:00 +0100
Message-ID: <CANpmjNOoYC93dt5hNmWsC2N8-7GuSp2L6Lb7mNOKxTGhreceUg@mail.gmail.com>
Subject: Re: [PATCH v7 2/4] mm,page_owner: Implement the tracking of the
 stacks count
To: Oscar Salvador <osalvador@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org, 
	linux-mm@kvack.org, Michal Hocko <mhocko@suse.com>, Vlastimil Babka <vbabka@suse.cz>, 
	Andrey Konovalov <andreyknvl@gmail.com>, Alexander Potapenko <glider@google.com>
Content-Type: text/plain; charset="UTF-8"

On Fri, 9 Feb 2024 at 00:45, Oscar Salvador <osalvador@suse.de> wrote:
>
> page_owner needs to increment a stack_record refcount when a new allocation
> occurs, and decrement it on a free operation.
> In order to do that, we need to have a way to get a stack_record from a
> handle.
> Implement stack_depot_get_stack() which just does that, and make it public
> so page_owner can use it.
>
> Also implement {inc,dec}_stack_record_count() which increments
> or decrements on respective allocation and free operations, via
> __reset_page_owner() (free operation) and __set_page_owner() (alloc
> operation).
>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>  include/linux/stackdepot.h |  8 ++++++++
>  lib/stackdepot.c           |  8 ++++++++
>  mm/page_owner.c            | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
>
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index d0dcf4aebfb4..ac62de4d4999 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -175,6 +175,14 @@ depot_stack_handle_t stack_depot_save_flags(unsigned long *entries,
>  depot_stack_handle_t stack_depot_save(unsigned long *entries,
>                                       unsigned int nr_entries, gfp_t gfp_flags);
>
> +/**
> + * stack_depo_get_stack - Get a pointer to a stack struct

Typo: "depo" -> depot

I would also write "stack_record struct", because "stack struct" does not exist.

> + * @handle: Stack depot handle
> + *
> + * Return: Returns a pointer to a stack struct
> + */
> +struct stack_record *stack_depot_get_stack(depot_stack_handle_t handle);

I don't know what other usecases there are for this, but I'd want to
make make sure we give users a big hint to avoid unnecessary uses of
this function.

Perhaps we also want to mark it as somewhat internal, e.g. by
prefixing it with __. So I'd call it __stack_depot_get_stack_record().

>  /**
>   * stack_depot_fetch - Fetch a stack trace from stack depot
>   *
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 16c8a1bf0008..197c355601f9 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -681,6 +681,14 @@ depot_stack_handle_t stack_depot_save(unsigned long *entries,
>  }
>  EXPORT_SYMBOL_GPL(stack_depot_save);
>
> +struct stack_record *stack_depot_get_stack(depot_stack_handle_t handle)
> +{
> +       if (!handle)
> +               return NULL;
> +
> +       return depot_fetch_stack(handle);
> +}
> +
>  unsigned int stack_depot_fetch(depot_stack_handle_t handle,
>                                unsigned long **entries)
>  {
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 5634e5d890f8..0adf41702b9d 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -61,6 +61,22 @@ static __init bool need_page_owner(void)
>         return page_owner_enabled;
>  }
>
> +static void inc_stack_record_count(depot_stack_handle_t handle)
> +{
> +       struct stack_record *stack = stack_depot_get_stack(handle);
> +
> +       if (stack)
> +               refcount_inc(&stack->count);
> +}

In the latest stackdepot version in -next, the count is initialized to
REFCOUNT_SATURATED to warn if a non-refcounted entry is suddenly used
as a refcounted one. In your case this is intentional and there is no
risk that the entry will be evicted, so that's ok. But you need to set
the refcount to 1 somewhere here on the initial stack_depot_save().