Re: [PATCH v3 2/4] mm/page_owner: add print_mode filter

[zhen.ni]

在 2026/4/29 08:57, SeongJae Park 写道:
> On Tue, 28 Apr 2026 15:11:10 +0800 Zhen Ni <zhen.ni@xxxxxxxxxxxx> wrote:
>
> > Add print_mode functionality to reduce page_owner output size by
> > printing only the stack handle instead of the full stack trace.
> >
> > Example output with print_mode enabled:
> >    Page allocated via order 0, mask 0x42800(GFP_NOWAIT|__GFP_COMP),
> >    pid 1, tgid 1 (systemd), ts 349667370 ns
> >    PFN 0xa00a2 type Unmovable Block 1280 type Unmovable
> >    Flags 0x33fffe0000004124(referenced|lru|active|private|node=3|zone=0|
> > lastcpupid=0x1ffff)
> >    handle: 17432583
> >    Charged to memcg /
>
> Looks nice to me.  I have just trivial cosmetic comments below.
>
> > Print mode significantly reduces output size while preserving all
> > other page allocation information. The correspondence between handles
> > and stack traces can be obtained through the show_stacks_handles interface.
>
> I understand the mode was introduced for the stack handle mode, but it exists
> for both full stack and stack handle mode?  The wording makes me assume the
> mode is only enabled vs disabled (boolean).  It is true that there are only two
> modes, but I feel like this commit message might better written.

The commit message could be clearer.

> > Link: https://lore.kernel.org/linux-mm/20260417154638.22370-3-zhen.ni@xxxxxxxxxxxx/
>
> Seems this is a link to v1 of this patch.  I think adding the context or moving
> this to the changelog [1] with the brief context explanation would be nice.
Good suggestion.

> > Suggested-by: Zi Yan <ziy@xxxxxxxxxx>
> > Signed-off-by: Zhen Ni <zhen.ni@xxxxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> > - Renamed from 'compact mode' to 'print_mode' for better clarity
> > - Use enum values (0=full_stack, 1=stack_handle) instead of boolean
> > - Update debugfs filename from 'compact' to 'print_mode'
> >
> > Changes in v3:
> > - No code changes
> > ---
> >   mm/page_owner.c | 28 +++++++++++++++++++++++++++-
> >   1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/page_owner.c b/mm/page_owner.c
> > index 5884d883837e..6d87b6948cfa 100644
> > --- a/mm/page_owner.c
> > +++ b/mm/page_owner.c
> > @@ -590,7 +590,13 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
> >   			migratetype_names[pageblock_mt],
> >   			&page->flags);
> >   
> > -	ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0);
> > +	/* Print mode: full stack or stack handle */
>
> I think this comment should be useful if it was compact_mode.  But because now
> it is called print_mode and values have good names that self explaining what
> the mode is, I'm not sure if this comment is really needed.

I'll remove the redundant comment

> > +	if (READ_ONCE(owner_filter.print_mode) == PAGE_OWNER_PRINT_STACK_HANDLE) {
> > +		ret += scnprintf(kbuf + ret, count - ret,
> > +				"handle: %d\n", handle);
> > +	} else {
> > +		ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0);
> > +	}
>
> Braces are not needed [2] here?
I'll remove the unnecessary braces

Thanks for the detailed review
> >   	if (ret >= count)
> >   		goto err;
> >   
> > @@ -985,6 +991,24 @@ static int page_owner_threshold_set(void *data, u64 val)
> >   DEFINE_SIMPLE_ATTRIBUTE(page_owner_threshold_fops, &page_owner_threshold_get,
> >   			&page_owner_threshold_set, "%llu");
> >   
> > +static int page_owner_print_mode_get(void *data, u64 *val)
> > +{
> > +	*val = READ_ONCE(owner_filter.print_mode);
> > +	return 0;
> > +}
> > +
> > +static int page_owner_print_mode_set(void *data, u64 val)
> > +{
> > +	if (val > PAGE_OWNER_PRINT_STACK_HANDLE)
> > +		return -EINVAL;
> > +	WRITE_ONCE(owner_filter.print_mode, val);
> > +	return 0;
> > +}
> > +
> > +DEFINE_SIMPLE_ATTRIBUTE(page_owner_print_mode_fops,
> > +			&page_owner_print_mode_get,
> > +			&page_owner_print_mode_set, "%lld");
> > +
> >   
> >   static int __init pageowner_init(void)
> >   {
> > @@ -998,6 +1022,8 @@ static int __init pageowner_init(void)
> >   	debugfs_create_file("page_owner", 0400, NULL, NULL, &page_owner_fops);
> >   
> >   	filter_dir = debugfs_create_dir("page_owner_filter", NULL);
> > +	debugfs_create_file("print_mode", 0600, filter_dir, NULL,
> > +			    &page_owner_print_mode_fops);
> >   
> >   	dir = debugfs_create_dir("page_owner_stacks", NULL);
> >   	debugfs_create_file("show_stacks", 0400, dir,
> > --
> > 2.20.1
>
> [1] https://docs.kernel.org/process/submitting-patches.html#commentary
> [2] https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces
>
>
> Thanks,
> SJ
Best regards,
Zhen Ni