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

["zhen.ni" <zhen.ni@easystack.cn>]

在 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