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

[Joshua Hahn <joshua.hahnjy@gmail.com>]

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.

> ---
> 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? 

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:

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

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