Re: [PATCH 9/9] um: Add initial SMP support

[Tiwei Bie <tiwei.bie@linux.dev>]

On Mon, 28 Jul 2025 18:27:53 +0200, Johannes Berg wrote:
> On Tue, 2025-07-29 at 00:04 +0800, Tiwei Bie wrote:
> > > > +++ b/arch/um/include/asm/spinlock.h
> > > > @@ -0,0 +1,8 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +#ifndef __ASM_UM_SPINLOCK_H
> > > > +#define __ASM_UM_SPINLOCK_H
> > > > +
> > > > +#include <asm/processor.h>
> > > > +#include <asm-generic/spinlock.h>
> > > > +
> > > > +#endif /* __ASM_UM_SPINLOCK_H */
> > > 
> > > Do we need this file? Maybe asm-generic should be including the right
> > > things it needs?
> > 
> > I added this file to include asm/processor.h; otherwise, there would be
> > a lot of compilation errors. Other architectures seem to do the same:
> > 
> > $ grep -r asm/processor.h arch/ | grep asm/spinlock.h
> > arch/arm/include/asm/spinlock.h:#include <asm/processor.h>
> > arch/alpha/include/asm/spinlock.h:#include <asm/processor.h>
> > arch/arc/include/asm/spinlock.h:#include <asm/processor.h>
> > arch/hexagon/include/asm/spinlock.h:#include <asm/processor.h>
> > arch/parisc/include/asm/spinlock.h:#include <asm/processor.h>
> > arch/x86/include/asm/spinlock.h:#include <asm/processor.h>
> > arch/s390/include/asm/spinlock.h:#include <asm/processor.h>
> > arch/mips/include/asm/spinlock.h:#include <asm/processor.h>
> > arch/loongarch/include/asm/spinlock.h:#include <asm/processor.h>
> 
> Except for loongarch they all do something else too though. Feels to me
> um (and loongarch) really shouldn't need that file.

Sorry for the confusion. My point is that since other architectures
also do this, it seems common practice to include asm/processor.h in
asm/spinlock.h when necessary.

The reason we need to include asm/processor.h in asm/spinlock.h on UML
is because:

ticket_spin_lock() (which is an inline function indirectly provided by
asm-generic/spinlock.h) relies on atomic_cond_read_acquire(), which
is defined as smp_cond_load_acquire().

On UML, smp_cond_load_acquire() is provided by asm-generic/barrier.h,
and it relies on smp_cond_load_relaxed(), which is also provided by
asm-generic/barrier.h on UML. And smp_cond_load_relaxed() is a macro
that relies on cpu_relax(), which is provided by asm/processor.h.

If we don't include asm/processor.h in asm/spinlock.h, ticket_spin_lock()
will fail to build:

./include/asm-generic/ticket_spinlock.h: In function ‘ticket_spin_lock’:
./include/asm-generic/barrier.h:253:17: error: implicit declaration of function ‘cpu_relax’ [-Werror=implicit-function-declaration]
  253 |                 cpu_relax();                                    \
      |                 ^~~~~~~~~
./include/asm-generic/barrier.h:270:16: note: in expansion of macro ‘smp_cond_load_relaxed’
  270 |         _val = smp_cond_load_relaxed(ptr, cond_expr);           \
      |                ^~~~~~~~~~~~~~~~~~~~~
./include/linux/atomic.h:28:40: note: in expansion of macro ‘smp_cond_load_acquire’
   28 | #define atomic_cond_read_acquire(v, c) smp_cond_load_acquire(&(v)->counter, (c))
      |                                        ^~~~~~~~~~~~~~~~~~~~~
./include/asm-generic/ticket_spinlock.h:49:9: note: in expansion of macro ‘atomic_cond_read_acquire’
   49 |         atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~

I can add a comment for it like this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sparc/include/asm/spinlock_32.h?h=v6.16#n14

Regards,
Tiwei