Re: [PATCH 0/8] mm/page_owner: Add PID/TGID/COMM and cgroup filtering

[zhen.ni]

在 2026/9/1 15:27, Lorenzo Stoakes (ARM) 写道:
> On Tue, Sep 01, 2026 at 02:34:34PM +0800, zhen.ni wrote:
> > 在 2026/8/29 02:36, Andrew Morton 写道:
> > > On Fri, 28 Aug 2026 08:03:38 +0100 "Lorenzo Stoakes (ARM)" <ljs@xxxxxxxxxx> wrote:
> > >
> > > > On Fri, Aug 28, 2026 at 11:13:31AM +0800, Zhen Ni wrote:
> > > > > This patch series adds process and memory cgroup filtering support to
> > > > > page_owner. Following the previous series that introduced print_mode and
> > > > > NUMA node filters:
> > > > >     https://lore.kernel.org/linux-mm/20260707115411.1714314-1-zhen.ni@xxxxxxxxxxxx/
> > > > >
> > > > > This series adds filtering capabilities to page_owner, allowing users to
> > > > > filter output by specific processes and memory cgroups. Users can now
> > > > > filter page_owner output by PID, TGID, COMM (with wildcard support), and
> > > > > memory cgroup path. This makes page_owner debugging more focused and
> > > > > efficient for tracking memory allocations in specific contexts.
> > > >
> > > > The majority of this cover letter feels like it should have been
> > > > documentation put somewhere :)
> > >
> > > yes please.
> > >
> > > The only thing longer than the cover letter is the Sashiko report ;)
> > >
> > > 	https://sashiko.dev/#/patchset/20260828031339.1270699-1-zhen.ni@xxxxxxxxxxxx
> >
> > Hi Andrew, Lorenzo,
> >
> > Thanks for the review.
> >
> > I have analyzed all the Sashiko report findings. Some will be fixed in
> > the next version, and for the rest I propose not to fix them, with
> > reasons below. If there are no objections I will send v2 accordingly.
> >
> > Will be fixed in the next version:
> >
> > - mm/page_owner.c: drop the kstrdup() copy in parse_pid_t_list(); the
> >    token will be parsed in place. This also fixes a leak on the success
> >    path and a kfree() of an advanced (interior) pointer on the error
> >    path. cmp_int() will replace plain subtraction in cmp_pid_t(), and
> >    pid values exceeding PID_MAX_LIMIT will be rejected.
> > - mm/page_owner.c: PAGE_OWNER will select GLOB so glob_match() is
> >    always linked in; parse_comm_list() will drop its kstrdup() copy the
> >    same way.
> > - mm/page_owner.c: the cgroup path buffer will be allocated once per
> >    read() outside the page_ext RCU read-side critical section instead of
> >    per page inside get_page_memcg_info() (GFP_KERNEL allocations must
> >    not sleep there). The memcg= parsing branch will be guarded by
> >    CONFIG_MEMCG so kernels built without memcg reject the command
> >    instead of silently enabling a filter that never matches.
> > - tools/mm/page_owner_filter.c: user-visible input errors (empty
> >    -p/-t/-c/-g arguments) will print error messages instead of exiting
> >    silently.
> > - Documentation: the wildcard pattern in the -c example will be quoted
> >    to prevent shell glob expansion.
> >
> > Proposed not to fix, by design:
> >
> > 1. Shared-fd concurrent read/write races (READ_ONCE around the pid
> >     passed to bsearch, torn reads of pid/tgid lists, glob_match() racing
> >     comm rewrites, concurrent write() leading to state->memcg_path
> >     double-allocation): multi-threaded sharing of one page_owner fd is
> >     not a designed use of this interface. The filters are per-fd state
> >     meant to be configured once and then read, which is what the
> >     page_owner_filter tool does. Adding locking to the read path would
> >     put overhead into the per-page scan loop for no designed benefit.
> >     This matches the semantics of the original filter introduction.
> >
> > 2. No "clear filter" support (empty pid=/tgid= lists partially clearing
> >     proc filters, empty memcg=/nid= being rejected): write commands are
> >     incremental -- "keep the unmentioned filters" -- and there is no
> >     clear operation by design. To start over, close the fd and open a
> >     fresh one; the page_owner_filter tool already works this way. This
> >     also matches the semantics of the original filter introduction.
> >
> > 3. kcalloc() vs kmalloc_array() for new_comm_list: no consumer of the
> >     list reads past strscpy()'s NUL terminator, so uninitialized bytes
> >     are unreachable.
> >
> > 4. char cgroup_path[512] in validate_cgroup_path(): acknowledged that
> >     the kernel side accepts paths up to PATH_MAX (4096). The userspace
> >     check truncates an over-long path with snprintf() and then fails
> >     access(), so it is rejected, never silently accepted. Bumping the
> >     buffer to PATH_MAX would only serve pathological paths; realistic
> >     cgroup paths are well under 100 bytes, so 512 wastes nothing in
> >     practice.
> >
> > If this plan looks reasonable I will send v2.
>
> Sorry but this kind of 'summary', 'do you agree with the plan' email is not
> acceptable.
>
> You have your feedback, reply to people like a human being directly to them,
> thank you very much.
>
> At this point, based on past experience, I have to ask if you're using an LLM?
> If so please disclose this as per kernel guidelines:
>
> https://docs.kernel.org/process/coding-assistants.html
> https://docs.kernel.org/process/generated-content.html
>
> > Thanks,
> > Zhen Ni
>
> --
> Cheers, Lorenzo
Hi, Lorenzo

I honestly don't understand what is wrong here. I spent two full days
going through every single finding in the Sashiko report one by one,
checking each against the code. In fact I am already working on v2 and
testing the corresponding changes. What I don't understand is what
"this kind of 'summary' email is not acceptable" is supposed to mean.

If you disagree with any specific item, name it and we can discuss it
-- but rejecting the whole thing outright, with just two documentation
links and no specifics, is not something I can act on.

Thanks,
Zhen