Message-ID: <aNvjDsBuw3hqwy31@tiehlicka>
Date: Tue, 30 Sep 2025 16:02:54 +0200
From: Michal Hocko <mhocko@...e.com>
To: Mauricio Faria de Oliveira <mfo@...lia.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 Fri 26-09-25 13:47:15, Mauricio Faria de Oliveira wrote:
> 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).

Sure, I see. The main problem with the option file is that it is
inherently suited for a single consumer which is a hard assumption to
make at this stage. So I think it is worth having a separate 2 files
which provide the missing functionality.

Thanks!
-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists