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

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

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