Re: [PATCH v8 3/5] mm,page_owner: Display all stacks and their count

[Vlastimil Babka]

On 2/13/24 16:33, Oscar Salvador wrote:
> On Tue, Feb 13, 2024 at 03:25:26PM +0100, Vlastimil Babka wrote:
>> On 2/12/24 23:30, Oscar Salvador wrote:
>> > +static int stack_print(struct seq_file *m, void *v)
>> > +{
>> > +	char *buf;
>> > +	int ret = 0;
>> > +	struct stack *stack = v;
>> > +	struct stack_record *stack_record = stack->stack_record;
>> > +
>> > +	if (!stack_record->size || stack_record->size < 0 ||
>> > +	    refcount_read(&stack_record->count) < 2)
>> > +		return 0;
>> > +
>> > +	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> > +
>> > +	ret += stack_trace_snprint(buf, PAGE_SIZE, stack_record->entries,
>> > +				   stack_record->size, 0);
>> > +	if (!ret)
>> > +		goto out;
>> > +
>> > +	scnprintf(buf + ret, PAGE_SIZE - ret, "stack_count: %d\n\n",
>> > +		  refcount_read(&stack_record->count));
>> > +
>> > +	seq_printf(m, buf);
>> > +	seq_puts(m, "\n\n");
>> > +out:
>> > +	kfree(buf);
>> 
>> Seems rather wasteful to do kzalloc/kfree so you can print into that buffer
>> first and then print/copy it again using seq_printf. If you give up on using
>> stack_trace_snprintf() it's not much harder to print the stack directly with
>> a loop of seq_printf. See e.g. slab_debugfs_show().
> 
> Well, I thought about not reinventing the wheel there, but fair enough
> than performing a kmalloc/free op on every print might be suboptimal.
> I will try to do ir with seq_printf alone.

Of course once there's more than one stackdepot user printing into a
seq_file, creating a common seq_file helper analogy of
stack_trace_snprintf() and using it from all places, would also be an option :)

> Thanks
>  
>