On Fri, 9 Feb 2024 at 22:42, Marco Elver <elver@xxxxxxxxxx> wrote:
>
> On Fri, 9 Feb 2024 at 22:37, Oscar Salvador <osalvador@xxxxxxx> 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.
