Message-ID: <d9af42d5cb9d427632087c5f51e50501@igalia.com>
Date: Fri, 26 Sep 2025 13:47:15 -0300
From: Mauricio Faria de Oliveira <mfo@...lia.com>
To: Michal Hocko <mhocko@...e.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Vlastimil Babka
 <vbabka@...e.cz>, Oscar Salvador <osalvador@...e.de>, Suren Baghdasaryan
 <surenb@...gle.com>, Brendan Jackman <jackmanb@...gle.com>, Johannes Weiner
 <hannes@...xchg.org>, Zi Yan <ziy@...dia.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, kernel-dev@...lia.com
Subject: Re: [PATCH 0/3] mm/page_owner: add options 'print_handle' and
 'print_stack' for 'show_stacks'

On 2025-09-26 03:55, Michal Hocko wrote:
> On Thu 25-09-25 16:38:46, Mauricio Faria de Oliveira wrote:
>> On 2025-09-25 13:08, Michal Hocko wrote:
> [...]
>> > Could you elaborate some more on why the performance really matters here?
>> 
>> Sure.
>> 
>> One reason is optimizing data processing.
>> 
>> Currently, the step to obtain the key of a strack trace (e.g., hashing)
>> incurs
>> a considerable work (done for all stack traces, on every sample) that
>> actually
>> is duplicated work (the same result for each stack trace, on every
>> sample).
> 
> OK, that was not really clear to me but the above seems to suggest that
> by hashing you really mean hashing in the userspace when trying to
> create a key so that you can watch memory consumption trends per stack
> trace (hash in this case) without post processing.

Yes.

> Stating that more explicitly in the changelog along with an example on
> how you are using this would be really helpful. 

Sure. Thanks for pointing that out, and making the effort to understand.

> When the interface was originally introduced the primary usecase was to
> examine biggest memory consumers - e.g. when memory counters do not add
> up to counters that track most common users (e.g. userspace memory, slab
> caches etc.). In those case you need to see those stack traces as those
> are giving you the most valuable information.
> 
> I can see you are coming from a different direction and you want to
> collect data repeatedly and watch for trends rather than analyzing a
> particular situation. This seems like a useful usecase in itself.

Precisely. I can make that more explicit in the changelog as well.

> My main question is whether this should squashed into the existing file
> with a rather strange semantic of controling the file content depending
> on a different file content. Instead, would it make more sense to add
> two more files, one to display your requested key:value data and another
> to resolve key -> stack trace?

I see your point. Either way works for me, honestly.
Let me justify the current way, but it's certainly OK to change it, if
that is preferred.

The use of option files has precedents in page_owner itself
(count_threshould) and ftrace (/sys/kernel/debug/trace/options/*).

The use of output files needs more code/complexity for a similar result,
AFAICT (I actually started it this way, but changed it to minimize
changes). 
The reason is debugfs_create_bool() is more specialized/simpler to
handle than debugfs_create_file().

It ends up with a similar pattern in a common "__stack_print()" to avoid
duplicate code (conditions on parameters to configure the output), and
it adds:
- 2 ops structs per file (file_operations and seq_operations, as in
'show_stacks'), for plumbing different behaviors down to different
functions, to call the common function with different parameters.
- It should be possible to reduce it with private fields (from
debugfs_create_file(data) to seq_file.private), however, since
seq_file.private is used (iterator in stack_start|next()), this needs
more code: a new struct for the private field (to store the current
iterator and add the new parameters).

So, I went for the (IMHO) simpler and smaller implementation with option
files instead of output files.

Please let me know which way is preferred, and I'll send v2 with that
(in addition to the changelog suggestions).

Thanks,

-- 
Mauricio

Powered by blists - more mailing lists