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

[Marco Elver <elver@google.com>]

On Wed, 7 Sept 2022 at 06:00, Oscar Salvador <osalvador@suse.de> wrote:
>
> On Tue, Sep 06, 2022 at 10:35:00AM +0200, Marco Elver wrote:
> > I think it's clear from the fact we're using the stack depot that any
> > printing will print stacks. To mirror the existing
> > 'stack_depot_print()', I'd go with 'stack_depot_print_all_count()'.
>
> Fair enough, I will rename it then.
>
> > Moderately better, but still not great. Essentially you need 2
> > cursors, but with loff_t you only get 1.
> >
> > I think the loff_t parameter can be used to encode both cursors. In
> > the kernel, loff_t is always 'long long', so it'll always be 64-bit.
> >
> > Let's assume that collisions in the hash table are rare, so the number
> > of stacks per bucket are typically small. Then you can encode the
> > index into the bucket in bits 0-31 and the bucket index in bits 32-63.
> > STACK_HASH_ORDER_MAX is 20, so 32 bits is plenty to encode the index.
>
> I see, I didn't think of it to be honest.
>
> Then, the below (completely untested) should the trick:
>
> <----
>  int stack_depot_print_all_count(char *buf, size_t size, loff_t *pos)
>  {
>          int ret = 0, stack_i, table_i;
>          struct stack_record **stacks, *stack;
>          unsigned long stack_table_entries = stack_hash_mask + 1;
>
>          stack_i = (*pos & 31);
>          table_i = (*pos >> 32);
>  new_table:
>          stacks = &stack_table[table_i];
>          stack = ((struct stack_record *)stacks) + stack_i;

Why are you casting a stack_record** to a stack_record*? stack_table
is already appropriately typed, and there should be no need to cast
things around.

'stacks' is supposed to be the bucket? In which case you need to
dereference it to get the first entry in the bucket: bucket =
stack_table[table_i];

stack_i cannot be used to index into the bucket, because the elements
in it are a linked list and not necessarily adjacent in memory. You
have to traverse the linked list stack_i elements to get to the start:

  for (int i = 0; stack && i < stack_i; stack = stack->next, ++i);

then you can proceed with the below code.

>          for (; stack; stack = stack->next, stack_i++) {
>                  if (!stack->size || stack->size < 0 ||
>                      stack->size > size || stack->handle.valid != 1 ||
>                      refcount_read(&stack->count) < 1)
>                          continue;
>
>                  ret += stack_trace_snprint(buf, size, stack->entries, stack->size, 0);
>                  ret += scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
>                                   refcount_read(&stack->count));
>                  *pos |= stack_i;
>                  *pos |= ((long long)table_i << 32);
>                  return ret;
>          }
>
>          table_i++;
>          /* Keep looking all tables for valid stacks */
>          if (table_i < stack_table_entries)
>                  goto new_table;

While you're at it, could you try to come up with a version that
avoids the goto?

>          return 0;
>  }
> ---->
>
> I will give it a go.
>
> Thanks Marco!
>
>
> --
> Oscar Salvador
> SUSE Labs