Re: [PATCH v5 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counte

[Alexander Potapenko <glider@google.com>]

On Tue, May 16, 2023 at 8:25 PM Oscar Salvador <osalvador@suse.de> wrote:

I am still hesitant about adding this functionality to stackdepot,
because page_owner is the only user of the stack counters that look
orthogonal to the rest of stackdepot.
One indicator of that is the fact that you keep adding dependencies on
page_owner to stackdepot code.

> We might be only interested in knowing about stacks <-> count
> relationship, so instead of having to fiddle with page_owner
> output and screen through pfns, let us add a new file called
> 'page_owner_stacks' that does just that.
> By cating such file, we will get all the stacktraces followed by

"cating"?

> +#ifdef CONFIG_PAGE_OWNER
> +void *stack_start(struct seq_file *m, loff_t *ppos);
> +void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
> +int stack_print(struct seq_file *m, void *v);
> +#endif

Code depending on CONFIG_PAGE_OWNER should not belong here.
It is fine to have generic iterators to traverse the stack depot in
stackdepot.h without #ifdefs.
Perhaps they don't need to implement the whole interface of seq_file.


> @@ -486,6 +487,77 @@ static struct stack_record *stack_depot_getstack(depot_stack_handle_t handle)
>         return stack;
>  }
>
> +#ifdef CONFIG_PAGE_OWNER

Ditto - no CONFIG_PAGE_OWNER, please


> +
> +int stack_print(struct seq_file *m, void *v)
> +{
> +       char *buf;
> +       int ret = 0;
> +       struct stack_record *stack = v;
> +
> +       if (!stack->size || stack->size < 0 ||
> +           stack->size > PAGE_SIZE || stack->handle.valid != 1 ||
> +           refcount_read(&stack->count) < 1)
> +               return 0;
> +
> +       buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +       ret += stack_trace_snprint(buf, PAGE_SIZE, stack->entries, stack->size, 0);
> +       scnprintf(buf + ret, PAGE_SIZE - ret, "stack count: %d\n\n",
> +                 refcount_read(&stack->count));
> +       seq_printf(m, buf);
> +       seq_puts(m, "\n\n");
> +       kfree(buf);
> +
> +       return 0;
> +}

Maybe stack_print() should be in mm/page_owner.c instead?



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg