Re: [PATCH] mm/page_alloc: simplify and cleanup pcp locking

[Vlastimil Babka <vbabka@suse.cz>]

On 10/15/25 16:51, Joshua Hahn wrote:
> On Wed, 15 Oct 2025 11:36:09 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:
> 
>> The pcp locking relies on pcp_spin_trylock() which has to be used
>> together with pcp_trylock_prepare()/pcp_trylock_finish() to work
>> properly on !SMP !RT configs. This is tedious and error-prone.
>> 
>> We can remove pcp_spin_lock() and underlying pcpu_spin_lock() because we
>> don't use it. Afterwards pcpu_spin_unlock() is only used together with
>> pcp_spin_trylock(). Therefore we can add the UP_flags parameter to them
>> and handle pcp_trylock_prepare()/finish() within them.
>> 
>> Additionally for the configs where pcp_trylock_prepare() is a no-op (SMP
>> || RT) make it pass &UP_flags to a no-op inline function. This ensures
>> typechecking and makes the local variable "used" so we can remove the
>> __maybe_unused attributes.
>> 
>> In my compile testing, bloat-o-meter reported no change on SMP config,
>> so the compiler is capable of optimizing away the no-ops same as before,
>> and we have simplified the code using pcp_spin_trylock().
>> 
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Hello Vlastimil, I hope you are doing well!
> 
> Thank you for this patch. This is a pattern that I found quite cumbersome,
> so this patch really makes the code so much easier to understand and read.

Hi, that's good to hear!
>> ---
>> based on mm-new
>> ---
>>  mm/page_alloc.c | 99 +++++++++++++++++++++++----------------------------------
>>  1 file changed, 40 insertions(+), 59 deletions(-)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 0155a66d7367..2bf707f92d83 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -99,9 +99,12 @@ static DEFINE_MUTEX(pcp_batch_high_lock);
>>  /*
>>   * On SMP, spin_trylock is sufficient protection.
>>   * On PREEMPT_RT, spin_trylock is equivalent on both SMP and UP.
>> + * Pass flags to a no-op inline function to typecheck and silence the unused
>> + * variable warning.
>>   */
>> -#define pcp_trylock_prepare(flags)	do { } while (0)
>> -#define pcp_trylock_finish(flag)	do { } while (0)
>> +static inline void __pcp_trylock_prepare(unsigned long *flags) { }
>> +#define pcp_trylock_prepare(flags) __pcp_trylock_prepare(&(flags))
>> +#define pcp_trylock_finish(flags)	do { } while (0)
>>  #else
> 
> I have one question here. I was a bit unsure why we do the typechecking and
> silencing for the unused variable warning for only pcp_trylock_prepare, but
> not for pcp_trylock_finish. Is it because pcp_trylock_finish will always
> be called after pcp_trylock_prepare, so the flag will have been used at
> that point? 

Exactly.

> I was concerned that there would have been some area where only
> pcp_trylock_finish would have been used, but compiling with W=1 seems to show
> no errors on my end : -) So it looks good to me! Feel free to add:

Yeah we can change that if ever we end up with some code that needs it
(hopefully not).

> Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>

Thanks!

> Thank you! I hope you have a great day!
> Joshua