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

[Oscar Salvador]

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


-- 
Oscar Salvador
SUSE Labs