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

[Oscar Salvador <osalvador@suse.de>]

On Mon, Sep 05, 2022 at 02:57:50PM +0200, Marco Elver wrote:
> On Mon, Sep 05, 2022 at 05:10AM +0200, Oscar Salvador wrote:
> [...]
> > +int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
> 
> Can you add kernel-doc comment what this does (and also update
> accordingly in 3/3 when you add 'threshold').

Yes, I guess a kernel-doc comment is due.

> From what I see it prints *all* stacks that have a non-zero count.
> Correct?

That's right.

> If so, should this be called stack_depot_print_all_count() (having
> stack(s) in the name twice doesn't make it more obvious what it does)?
> Then in the follow-up patch you add the 'threshold' arg.

I guess so. The only reason I went with the actual name is that for me
"stack_depot" was kinda the name of the module/library, and
so I wanted to make crystal clear what were we printing.

But I'm ok with renaming it if it's already self-explanatory

> > +{
> > +	int i = *pos, ret = 0;
> > +	struct stack_record **stacks, *stack;
> > +	static struct stack_record *last = NULL;
> > +	unsigned long stack_table_entries = stack_hash_mask + 1;
> > +
> > +	/* Continue from the last stack if we have one */
> > +	if (last) {
> > +		stack = last->next;
> 
> This is dead code?

No, more below.

> Either I'm missing something really obvious, but I was able to simplify
> the above function to just this (untested!):
> 
> 	int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
> 	{
> 		const unsigned long stack_table_entries = stack_hash_mask + 1;
> 
> 		/* Iterate over all tables for valid stacks. */
> 		for (; *pos < stack_table_entries; (*pos)++) {
> 			for (struct stack_record *stack = stack_table[*pos]; stack; stack = stack->next) {
> 				if (!stack->size || stack->size < 0 || stack->size > size ||
> 				    stack->handle.valid != 1 || refcount_read(&stack->count) < 1)
> 					continue;
> 
> 				return stack_trace_snprint(buf, size, stack->entries, stack->size, 0) +
> 				       scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
> 						 refcount_read(&stack->count));
> 			}
> 		}
> 
> 		return 0;

Yes, this will not work.

You have stack_table[] which is an array for struct stacks, and each struct
stack has a pointer to its next stack which walks from the beginning fo a specific
table till the end. e.g:

stack_table[0] = {stack1, stack2, stack3, ...} (each linked by ->next)
stack_table[1] = {stack1, stack2, stack3, ...} (each linked by ->next)
..
stack_table[stack_table_entries - 1] = {stack1, stack2, stack3, ...} (each linked by ->next)

*pos holds the index of stack_table[], while "last" holds the last stack within
the table we were processing.

So, when we find a valid stack to print, set "last" to that stack, and *pos to the index
of stack_table.
So, when we call stack_depot_print_stacks_threshold() again, we set "stack" to "last"->next,
and we are ready to keep looking with:

 for (; stack; stack = stack->next) {
    ...
    check if stack is valid
 }

Should not we find any more valid stacks in that stack_table, we need to check in
the next table, so we do::

    i++; (note that i was set to *pos at the beginning of the function)
	*pos = i;
	last = NULL; 
    goto new_table

and now are ready to do:

new_table:
		stacks = &stack_table[i];
		stack = (struct stack_record *)stacks;


Does this clarify it a little bit?

About using static vs non-static.
In the v1, I was using a parameter which contained last_stack:

https://patchwork.kernel.org/project/linux-mm/patch/20220901044249.4624-3-osalvador@suse.de/

Not sure if that's better? Thoughts?


-- 
Oscar Salvador
SUSE Labs