Re: [PATCH v7 2/4] mm,page_owner: Implement the tracking of the stacks count

[Marco Elver <elver@google.com>]

On Fri, 9 Feb 2024 at 22:42, Marco Elver <elver@google.com> wrote:
>
> 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);

Do you need to inc the first allocation? Should there be an "else"
here instead of always doing refcount_inc()?

> >                  refcount_inc(&stack->count);
> >          }
> >  }
>
> That looks reasonable.