Received-SPF: pass (google.com: domain of linux-kernel+bounces-60047-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240208234539.19113-1-osalvador@suse.de> <20240208234539.19113-3-osalvador@suse.de>
 <CANpmjNOoYC93dt5hNmWsC2N8-7GuSp2L6Lb7mNOKxTGhreceUg@mail.gmail.com> <ZcabeMCHCkl3Bf6q@localhost.localdomain>
In-Reply-To: <ZcabeMCHCkl3Bf6q@localhost.localdomain>
From: Marco Elver <elver@google.com>
Date: Fri, 9 Feb 2024 22:42:42 +0100
Message-ID: <CANpmjNPM4aKMWAmxWEMQ-Antq0jDFauOya2XHHX5RhnONucgrg@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 22:37, Oscar Salvador <osalvador@suse.de> wrote:
>
> On Fri, Feb 09, 2024 at 08:45:00AM +0100, Marco Elver wrote:
> > > +/**
> > > + * 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.
>
> Fixed.
>
> > > + * @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().
>
> Yes, I went with __stack_depot_get_stack_record(), and I updated its doc
> in stackdepot.h, mentioning that is only for internal purposes.
>
> > > +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().
>
> Well, I went with something like:
>
>  static void inc_stack_record_count(depot_stack_handle_t handle)
>  {
>          struct stack_record *stack = __stack_depot_get_stack_record(handle);
>
>          if (stack) {
>                  /*
>                   * New stack_records that do not use STACK_DEPOT_FLAG_GET start
>                   * with REFCOUNT_SATURATED to catch spurious increments of their
>                   * refcount.
>                   * Since we do not use STACK_DEPOT_FLAG_{GET,PUT} API, let us

There is no FLAG_PUT, only stack_depot_put(). Saying you do not use
the refcount to free any entries should hopefully make it clear that
even if the refcount saturates and you wrap around to 1, nothing
catastrophic will happen.

>                   * set a refcount of 1 ourselves.
>                   */
>                  if (refcount_read(&stack->count) == REFCOUNT_SATURATED)
>                          refcount_set(&stack->count, 1);
>                  refcount_inc(&stack->count);
>          }
>  }

That looks reasonable.