Re: [PATCH 1/3] mm/page_alloc: effectively disable pcp with CONFIG_SMP=n

[Vlastimil Babka]

On 3/3/26 17:09, David Hildenbrand (Arm) wrote:
>> - * With the UP spinlock implementation, when we spin_lock(&pcp->lock) (for i.e.
>> - * a potentially remote cpu drain) and get interrupted by an operation that
>> - * attempts pcp_spin_trylock(), we can't rely on the trylock failure due to UP
>> - * spinlock assumptions making the trylock a no-op. So we have to turn that
>> - * spin_lock() to a spin_lock_irqsave(). This works because on UP there are no
>> - * remote cpu's so we can only be locking the only existing local one.
>> + * On CONFIG_SMP=n the UP implementation of spin_trylock() never fails and thus
>> + * is not compatible with our locking scheme. However we do not need pcp for
>> + * scalability in the first place, so just make all the trylocks fail and take
>> + * the slow path unconditionally.
>>   */
>> +#else
>> +#define pcp_spin_trylock(ptr)		\
>> +		NULL
>> +
>> +#define pcp_spin_unlock(ptr)		\
>> +		BUG_ON(1)
> 
> Did you try turning this into a BUILD_BUG() ?

I considered it, but was afraid it would work, until with some weird
compiler and arch/config it wouldn't.

> I'd assume that the compiler would optimize-out all dead code and
> consequently not trigger the BUILD_BUG.
> 
> if (pcp_spin_trylock()) {
> 	/* dead code */
> 	pcp_spin_unlock()
> }
> 
> IIUC, the trylock+unlock is not really split over multiple functions.

Some of the code is a bit more complex like in free_unref_folios() there's a
loop where early on is a check meant as a result from the previous iteration

                        if (pcp) {
                                pcp_spin_unlock(pcp);

and only later

                        pcp = pcp_spin_trylock(zone->per_cpu_pageset);


Also in free_frozen_page_commit() we enter locked but might unlock. Yes the
function should be eliminated as a whole, but again in free_unref_folios()
this is a bit complex...