On Thu, Sep 01, 2022 at 10:24:58AM +0200, Marco Elver wrote:
> On Thu, Sep 01, 2022 at 06:42AM +0200, Oscar Salvador wrote:
> > include/linux/stackdepot.h | 13 ++++++-
> > lib/stackdepot.c | 79 +++++++++++++++++++++++++++++++-------
> > mm/kasan/common.c | 3 +-
>
> +Cc other kasan maintainers
Yeah, sorry about that, I should have CCed you guys.
> > +typedef enum stack_action {
> > + STACK_ACTION_NONE,
> > + STACK_ACTION_INC,
> > +}stack_action_t;
> > +
>
> missing space after '}'. But please no unnecessary typedef, just 'enum
> stack_action' (and spelling out 'enum stack_action' elsewhere) is just
> fine.
Sure, will re-name it.
>
> This is in the global namespace, so I'd call this
> stack_depot_action+STACK_DEPOT_ACTION_*.
>
> However, .._ACTION_INC doesn't really say what's incremented. As an
> analog to stack_depot_dec_count(), perhaps .._ACTION_COUNT?
I guess we can go "STACK_DEPOT_ACTION_COUNT", or "STACK_DEPOT_ACTION_REF_INC",
but the latter seems rather baroque for my taste.
> In general it'd be nicer if there was stack_depot_inc_count() instead of
> this additional argument, but I see that for performance reasons you
> might not like that?
Yes, the first prototypes didn't have this stack_action_t thing,
but that implied that we had to look for the stack twice
in the __set_page_owner() case.
This way we only do that in the __reset_page_owner() case.
So yes, it's a trade-off performance vs LOC.
> > --- a/lib/stackdepot.c
> > +++ b/lib/stackdepot.c
> > @@ -63,6 +63,7 @@ struct stack_record {
> > u32 hash; /* Hash in the hastable */
> > u32 size; /* Number of frames in the stack */
> > union handle_parts handle;
> > + refcount_t count; /* Number of the same repeated stacks */
>
> This will increase stack_record size for every user, even if they don't
> care about the count.
>
> Is there a way to store this out-of-line somewhere?
That would require having some kind of e.g: dynamic struct and allocating
new links to stacks as they were created and increase the refcount there.
But that would be too much of complexity, I think.
As I read in your other thread, we can probably live with that, but
it is worth spelling out in the changelog.
> > +void stack_depot_dec_count(depot_stack_handle_t handle)
> > +{
> > + struct stack_record *stack = NULL;
> > +
> > + stack = stack_depot_getstack(handle);
> > + if (stack) {
> > + /*
> > + * page_owner creates some stacks via create_dummy_stack().
> > + * We are not interested in those, so make sure we only decrement
> > + * "valid" stacks.
> > + */
>
> Comment indent is wrong.
Will fix it.
Thanks for taking the time to review the code Marco!
--
Oscar Salvador
SUSE Labs
