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

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

[Johannes Berg]

On Sun, 2025-07-27 at 14:29 +0800, Tiwei Bie wrote:
> 
> +++ b/arch/um/include/asm/smp.h
> @@ -2,6 +2,27 @@
>  #ifndef __UM_SMP_H
>  #define __UM_SMP_H
>  
> -#define hard_smp_processor_id()		0
> +#if IS_ENABLED(CONFIG_SMP)
> +
> +#include <linux/bitops.h>
> +#include <asm/current.h>
> +#include <linux/cpumask.h>
> +#include <shared/smp.h>
> +
> +#define raw_smp_processor_id() uml_curr_cpu()
> +
> +void arch_smp_send_reschedule(int cpu);
> +
> +void arch_send_call_function_single_ipi(int cpu);
> +
> +void arch_send_call_function_ipi_mask(const struct cpumask *mask);
> +
> +static inline void smp_cpus_done(unsigned int maxcpus) { }
> +
> +#else /* !CONFIG_SMP */
> +
> +#define raw_smp_processor_id() 0

This seems a bit odd to me, linux/smp.h also defines
raw_smp_processor_id() to 0 the same way, unconditionally.

It almost seems to me we should define raw_smp_processor_id() only for
SMP? But then also __smp_processor_id()? Maybe not?

linux-arch folks, do you have any comments?

> --- /dev/null
> +++ 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?

> +void enter_turnstile(struct mm_id *mm_id);
> +void exit_turnstile(struct mm_id *mm_id);

We could add __acquires(turnstile) and __releases(turnstile) or
something, to have sparse check that it's locked/unlocked correctly, but
not sure it's worth it.

> +int disable_kmalloc[NR_CPUS] = { 0 };

nit: you can remove the "0".

> +int smp_sigio_handler(struct uml_pt_regs *regs)
> +{
> +	int cpu = raw_smp_processor_id();
> +
> +	IPI_handler(cpu, regs);
> +	if (cpu != 0)
> +		return 1;
> +	return 0;

nit: "return cpu != 0;" perhaps

> +__uml_setup("ncpus=", uml_ncpus_setup,
> +"ncpus=<# of desired CPUs>\n"
> +"    This tells UML how many virtual processors to start. The maximum\n"
> +"    number of supported virtual processors can be obtained by querying\n"
> +"    the CONFIG_NR_CPUS option using --showconfig.\n\n"


I feel like probably this should at least for now be mutually exclusive
with time-travel= parameters, at least if it's not 1? Or perhaps only
with time-travel=ext?

The timer code is in another patch, will look at that also. I guess
until then it's more of a gut feeling on "how would this work" :)

johannes

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

[Randy Dunlap]

On 7/28/25 3:47 AM, Johannes Berg wrote:
>> +int smp_sigio_handler(struct uml_pt_regs *regs)
>> +{
>> +	int cpu = raw_smp_processor_id();
>> +
>> +	IPI_handler(cpu, regs);
>> +	if (cpu != 0)
>> +		return 1;
>> +	return 0;
> nit: "return cpu != 0;" perhaps

	return !!cpu;

-- 
~Randy

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

[Tiwei Bie]

On Mon, 28 Jul 2025 12:47:08 +0200, Johannes Berg wrote:
> On Sun, 2025-07-27 at 14:29 +0800, Tiwei Bie wrote:
> > 
> > +++ b/arch/um/include/asm/smp.h
> > @@ -2,6 +2,27 @@
> >  #ifndef __UM_SMP_H
> >  #define __UM_SMP_H
> >  
> > -#define hard_smp_processor_id()		0
> > +#if IS_ENABLED(CONFIG_SMP)
> > +
> > +#include <linux/bitops.h>
> > +#include <asm/current.h>
> > +#include <linux/cpumask.h>
> > +#include <shared/smp.h>
> > +
> > +#define raw_smp_processor_id() uml_curr_cpu()
> > +
> > +void arch_smp_send_reschedule(int cpu);
> > +
> > +void arch_send_call_function_single_ipi(int cpu);
> > +
> > +void arch_send_call_function_ipi_mask(const struct cpumask *mask);
> > +
> > +static inline void smp_cpus_done(unsigned int maxcpus) { }
> > +
> > +#else /* !CONFIG_SMP */
> > +
> > +#define raw_smp_processor_id() 0
> 
> This seems a bit odd to me, linux/smp.h also defines
> raw_smp_processor_id() to 0 the same way, unconditionally.
> 
> It almost seems to me we should define raw_smp_processor_id() only for
> SMP? But then also __smp_processor_id()? Maybe not?

I think you're right. I should't define raw_smp_processor_id() for non-SMP.

> 
> linux-arch folks, do you have any comments?
> 
> > --- /dev/null
> > +++ 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>

> 
> > +void enter_turnstile(struct mm_id *mm_id);
> > +void exit_turnstile(struct mm_id *mm_id);
> 
> We could add __acquires(turnstile) and __releases(turnstile) or
> something, to have sparse check that it's locked/unlocked correctly, but
> not sure it's worth it.

Will do.

> 
> > +int disable_kmalloc[NR_CPUS] = { 0 };
> 
> nit: you can remove the "0".

Will fix all the nits in the next version.

> 
> > +int smp_sigio_handler(struct uml_pt_regs *regs)
> > +{
> > +	int cpu = raw_smp_processor_id();
> > +
> > +	IPI_handler(cpu, regs);
> > +	if (cpu != 0)
> > +		return 1;
> > +	return 0;
> 
> nit: "return cpu != 0;" perhaps
> 
> > +__uml_setup("ncpus=", uml_ncpus_setup,
> > +"ncpus=<# of desired CPUs>\n"
> > +"    This tells UML how many virtual processors to start. The maximum\n"
> > +"    number of supported virtual processors can be obtained by querying\n"
> > +"    the CONFIG_NR_CPUS option using --showconfig.\n\n"
> 
> 
> I feel like probably this should at least for now be mutually exclusive
> with time-travel= parameters, at least if it's not 1? Or perhaps only
> with time-travel=ext?

Currently, the UML_TIME_TRAVEL_SUPPORT option depends on !SMP:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/um/Kconfig?h=v6.16#n218

so they can't be enabled at the same time during build.

> 
> The timer code is in another patch, will look at that also. I guess
> until then it's more of a gut feeling on "how would this work" :)

Thanks for the review! :)

Regards,
Tiwei

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

[Johannes Berg]

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.

> > I feel like probably this should at least for now be mutually exclusive
> > with time-travel= parameters, at least if it's not 1? Or perhaps only
> > with time-travel=ext?
> 
> Currently, the UML_TIME_TRAVEL_SUPPORT option depends on !SMP:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/um/Kconfig?h=v6.16#n218
> 
> so they can't be enabled at the same time during build.

Oops, sorry, missed that. Good. I didn't see anything particularly wrong
in the time code, but I'm sure it won't work there :)

johannes

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

[Tiwei Bie]

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

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

[Johannes Berg]

On Tue, 2025-07-29 at 23:06 +0800, Tiwei Bie wrote:
> 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.

Yeah, I understand.

> 
> 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().

Right, but that's not the architecture's "fault".

It seems to me that either spinlock.h should include asm/processor.h for
it, or (at least, but I think less appropriate) asm-generic/spinlock.h
should be doing this.

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

In general though, there ought to be some definition of which header
file(s) is/are expected to provide smp_cond_load_acquire() and/or
atomic_cond_read_acquire(). And that header file/those header files
should be included by the files that use the functions/macros.


IOW, I think you've stumbled across an inconsistency in the generic
files, and hence we should fix that, rather than having each
architecture paper over it.


johannes

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

[Tiwei Bie]

On Tue, 29 Jul 2025 17:37:24 +0200, Johannes Berg wrote:
> On Tue, 2025-07-29 at 23:06 +0800, Tiwei Bie wrote:
> > 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.
> 
> Yeah, I understand.
> 
> > 
> > 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().
> 
> Right, but that's not the architecture's "fault".
> 
> It seems to me that either spinlock.h should include asm/processor.h for
> it,

+1

> or (at least, but I think less appropriate) asm-generic/spinlock.h
> should be doing this.
> 
> > 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.
> 
> In general though, there ought to be some definition of which header
> file(s) is/are expected to provide smp_cond_load_acquire() and/or
> atomic_cond_read_acquire(). And that header file/those header files
> should be included by the files that use the functions/macros.
> 
> 
> IOW, I think you've stumbled across an inconsistency in the generic
> files, and hence we should fix that, rather than having each
> architecture paper over it.

That does make sense. I will prepare a patch for that. Thanks!

Regards,
Tiwei

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

[Tiwei Bie]

On Wed, 30 Jul 2025 12:18:38 +0800, Tiwei Bie wrote:
> On Tue, 29 Jul 2025 17:37:24 +0200, Johannes Berg wrote:
[...]
> > 
> > IOW, I think you've stumbled across an inconsistency in the generic
> > files, and hence we should fix that, rather than having each
> > architecture paper over it.
> 
> That does make sense. I will prepare a patch for that. Thanks!

Hmm, this issue might be a bit tricky to resolve.. The root cause
is that smp_cond_load_relaxed() provided by asm/barrier.h relies
on cpu_relax() [1][2], but the corresponding header isn't included.
The reason why it's not included might be that asm/processor.h
includes too many dependencies, which prevents barrier.h from
including it. I haven't come up with an ideal way to address it
yet. Fortunately, smp_cond_load_relaxed() is a macro, so cpu_relax()
is needed only when smp_cond_load_relaxed() is invoked inside a
function (i.e., when it's expanded during preprocessing).

As for this series, I realized that I should implement spinlock in
$SUBARCH (i.e., in a $SUBARCH native way, which won't require the
above workaround anymore), similar to how atomic and barrier are
implemented. I'll take that approach in the next version.

Thanks again for the review!

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/asm-generic/barrier.h?h=v6.16#n253
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/um/asm/barrier.h?h=v6.16#n27

Regards,
Tiwei