Re: [PATCH v11 1/4] mm/page_owner: add print_mode filter

[zhen.ni]

在 2026/7/3 16:51, Vlastimil Babka (SUSE) 写道:
> On 7/3/26 10:14, zhen.ni wrote:
> > 在 2026/6/29 17:30, Vlastimil Babka (SUSE) 写道:
> > > On 6/29/26 04:59, Ye Liu wrote:
> > > > 在 2026/6/25 12:30, Zhen Ni 写道:
> > > > Hi , Zhen
> > > > The spinlock in struct page_owner_filter_state is unnecessary and adds significant overhead in the read path.
> > > >                                                                                                       
> > > > 1. Per-fd isolation: the state is allocated per open() and stored in file->private_data.
> > > > There is no cross-fd contention possible.
> > > > 2. Hot path cost: the lock is taken for every single page in read_page_owner() and
> > > > print_page_owner(). A single read can traverse millions of pages, each paying
> > > > spin_lock_irqsave/irqrestore — including interrupt disable — just to read a mode
> > > > enum or check a nodemask. This is measurable overhead for no real benefit.
> > > > 3. No practical race: nobody writes filter config to an fd while simultaneously reading from it.
> > > >                                                                                                       
> > > > Suggest dropping the lock entirely.
> > > >                                                                                                       
> > > > Just my take though — happy to follow whatever the other reviewers prefer here.
> > >
> > > I agree. If someone is writing (updating filter) and reading (getting
> > > page_owner output) at the same time from multiple threads, they might get
> > > inconsistent results but that's getting what you ask for. Importantly it
> > > can't cause any crash, AFAICS.
> >
> > Hi Vlastimil, Ye,
> >
> > Thanks for the review. I understand your concerns about the spinlock
> > overhead in the read path.
> >
> > The spinlock does have its use case: it prevents race conditions when
> > multiple threads share the same file descriptor and call read() and
> > write() concurrently. While we recommend users use the page_owner_filter
> > tool, we cannot exclude the possibility that some users might directly
> > share the fd across threads.
>
> Yeah and they will then get inconsistent results even with the spinlock, we
> don't have to prevent that.
>
> > That said, I'm open to discussion on whether we need the spinlock. As
> > Vlastimil noted, the issue isn't severe enough to cause crashes. My v8
> > version didn't have the spinlock - I added it in response to review
> > feedback.
>
> It was in response to sashiko. Those suggestions are sometimes useful,
> sometimes not. It's good to consider them, but critically, and not implement
> blindly.
>
> > So the question is really whether we want to protect multi-threaded fd
>
> We don't.
>
> > sharing or not. Because, the overhead is small in non-contended cases
> > (single-threaded usage) since there are no competing lock holders.
>
> The atomic operations are still expensive and unnecessary.
>
> Thanks.
>
> > Thanks,
> > Zhen
I'll remove the spinlock in v12 soon.

Thanks,
Zhen