Message-ID: <370b7e1d29a382471db93475f4419f22@igalia.com>
Date: Wed, 01 Oct 2025 14:37:40 -0300
From: Mauricio Faria de Oliveira <mfo@...lia.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Michal Hocko <mhocko@...e.com>, Andrew Morton
 <akpm@...ux-foundation.org>, 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-10-01 07:58, Vlastimil Babka wrote:
> On 9/30/25 4:02 PM, Michal Hocko wrote:
>> On Fri 26-09-25 13:47:15, Mauricio Faria de Oliveira wrote:
>>>> 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.
> 
> Agreed, we should prioritize a better userspace API over having simpler
> kernel implementation.

Just to clarify, I agree. I personally considered option files a good
userspace API for this, but later realized the different perspective
above.

> Will count_threshold apply the same to the new file that prints only
> handles? I guess it will?

Yes. The new file that prints only handles should only differ in format,
not behavior.

This is different for the file that translates handles to stacks,
though. For that, count_threshould does not apply, as a previously
reported handle may have less pages and not make it, which would not
allow it to be resolved to a stack trace.

> Also the handles to stack translation file could perhaps support
> "seeking" to a specific handle if you're interested in only a few
> handles. Perhaps not necessary though if you plan to read it all just once.

That sounds cool. For the current usecase, it only needs to read it all
just once, indeed. I'd be happy to revisit this later, if needed.

Let's see how v2 goes; sending it out.

Thanks!

-- 
Mauricio

Powered by blists - more mailing lists