linux-kernel - Re: [PATCH 0/3] mm/page_owner: add options 'print_handle' and 'print_stack' for 'show

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <602271e2-86c9-4a63-845a-b84407d3aa51@suse.cz>
Date: Wed, 1 Oct 2025 12:58:50 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Michal Hocko <mhocko@...e.com>,
 Mauricio Faria de Oliveira <mfo@...lia.com>
Cc: 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 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.

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

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.

> Thanks!