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

[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