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
