From: Guo Ren <[email protected]>
Palmer:
Peter sent an RFC out about a year ago
<https://lore.kernel.org/lkml/[email protected]/>,
but after a spirited discussion it looks like we lost track of things.
IIRC there was broad consensus on this being the way to go, but there
was a lot of discussion so I wasn't sure. Given that it's been a year,
I figured it'd be best to just send this out again formatted a bit more
explicitly as a patch.
This has had almost no testing (just a build test on RISC-V defconfig),
but I wanted to send it out largely as-is because I didn't have a SOB
from Peter on the code. I had sent around something sort of similar in
spirit, but this looks completely re-written. Just to play it safe I
wanted to send out almost exactly as it was posted. I'd probably rename
this tspinlock and tspinlock_types, as the mis-match kind of makes my
eyes go funny, but I don't really care that much. I'll also go through
the other ports and see if there's any more candidates, I seem to
remember there having been more than just OpenRISC but it's been a
while.
I'm in no big rush for this and given the complex HW dependencies I
think it's best to target it for 5.19, that'd give us a full merge
window for folks to test/benchmark it on their systems to make sure it's
OK. RISC-V has a forward progress guarantee so we should be safe, but
these can always trip things up.
Guo:
Update V2 with Arnd's suggestion [1].
[1] https://lore.kernel.org/linux-arch/CAK8P3a0NMPVGVw7===uEOtNnu1hr1GqimMbZT+Kea1CUxRvPmw@mail.gmail.com/raw
Changes in V2:
- Follow Arnd suggestion to make the patch series more generic.
- Add csky in the series.
- Combine RISC-V's two patches into one.
- Modify openrisc's patch to suit the new generic version.
Guo Ren (1):
csky: Move to generic ticket-spinlock
Palmer Dabbelt (1):
RISC-V: Move to ticket-spinlocks & RW locks
Peter Zijlstra (3):
asm-generic: ticket-lock: New generic ticket-based spinlock
asm-generic: qspinlock: Indicate the use of mixed-size atomics
openrisc: Move to ticket-spinlock
arch/csky/include/asm/Kbuild | 3 +-
arch/csky/include/asm/spinlock.h | 89 --------------
arch/csky/include/asm/spinlock_types.h | 27 -----
arch/openrisc/Kconfig | 1 -
arch/openrisc/include/asm/Kbuild | 7 +-
arch/openrisc/include/asm/spinlock.h | 27 -----
arch/openrisc/include/asm/spinlock_types.h | 7 --
arch/riscv/Kconfig | 1 +
arch/riscv/include/asm/Kbuild | 2 +
arch/riscv/include/asm/spinlock.h | 135 ---------------------
arch/riscv/include/asm/spinlock_types.h | 25 ----
include/asm-generic/qspinlock.h | 30 +++++
include/asm-generic/spinlock.h | 11 +-
include/asm-generic/spinlock_types.h | 15 +++
include/asm-generic/ticket-lock-types.h | 11 ++
include/asm-generic/ticket-lock.h | 86 +++++++++++++
16 files changed, 157 insertions(+), 320 deletions(-)
delete mode 100644 arch/csky/include/asm/spinlock.h
delete mode 100644 arch/csky/include/asm/spinlock_types.h
delete mode 100644 arch/openrisc/include/asm/spinlock.h
delete mode 100644 arch/openrisc/include/asm/spinlock_types.h
delete mode 100644 arch/riscv/include/asm/spinlock.h
delete mode 100644 arch/riscv/include/asm/spinlock_types.h
create mode 100644 include/asm-generic/spinlock_types.h
create mode 100644 include/asm-generic/ticket-lock-types.h
create mode 100644 include/asm-generic/ticket-lock.h
--
2.25.1
From: Peter Zijlstra <[email protected]>
The qspinlock implementation depends on having well behaved mixed-size
atomics. This is true on the more widely-used platforms, but these
requirements are somewhat subtle and may not be satisfied by all the
platforms that qspinlock is used on.
Document these requirements, so ports that use qspinlock can more easily
determine if they meet these requirements.
Signed-off-by: Palmer Dabbelt <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Waiman Long <[email protected]>
---
include/asm-generic/qspinlock.h | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index d74b13825501..a7a1296b0b4d 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -2,6 +2,36 @@
/*
* Queued spinlock
*
+ * A 'generic' spinlock implementation that is based on MCS locks. An
+ * architecture that's looking for a 'generic' spinlock, please first consider
+ * ticket-lock.h and only come looking here when you've considered all the
+ * constraints below and can show your hardware does actually perform better
+ * with qspinlock.
+ *
+ *
+ * It relies on atomic_*_release()/atomic_*_acquire() to be RCsc (or no weaker
+ * than RCtso if you're power), where regular code only expects atomic_t to be
+ * RCpc.
+ *
+ * It relies on a far greater (compared to ticket-lock.h) set of atomic
+ * operations to behave well together, please audit them carefully to ensure
+ * they all have forward progress. Many atomic operations may default to
+ * cmpxchg() loops which will not have good forward progress properties on
+ * LL/SC architectures.
+ *
+ * One notable example is atomic_fetch_or_acquire(), which x86 cannot (cheaply)
+ * do. Carefully read the patches that introduced queued_fetch_set_pending_acquire().
+ *
+ * It also heavily relies on mixed size atomic operations, in specific it
+ * requires architectures to have xchg16; something which many LL/SC
+ * architectures need to implement as a 32bit and+or in order to satisfy the
+ * forward progress guarantees mentioned above.
+ *
+ * Further reading on mixed size atomics that might be relevant:
+ *
+ * http://www.cl.cam.ac.uk/~pes20/popl17/mixed-size.pdf
+ *
+ *
* (C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P.
* (C) Copyright 2015 Hewlett-Packard Enterprise Development LP
*
--
2.25.1
From: Peter Zijlstra <[email protected]>
We have no indications that openrisc meets the qspinlock requirements,
so move to ticket-spinlock as that is more likey to be correct.
Remove duplicate arch_spin_relax, arch_read_relax, arch_write_relax
definition.
Signed-off-by: Palmer Dabbelt <[email protected]>
Signed-off-by: Guo Ren <[email protected]>
---
arch/openrisc/Kconfig | 1 -
arch/openrisc/include/asm/Kbuild | 7 ++----
arch/openrisc/include/asm/spinlock.h | 27 ----------------------
arch/openrisc/include/asm/spinlock_types.h | 7 ------
4 files changed, 2 insertions(+), 40 deletions(-)
delete mode 100644 arch/openrisc/include/asm/spinlock.h
delete mode 100644 arch/openrisc/include/asm/spinlock_types.h
diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
index f724b3f1aeed..f5fa226362f6 100644
--- a/arch/openrisc/Kconfig
+++ b/arch/openrisc/Kconfig
@@ -30,7 +30,6 @@ config OPENRISC
select HAVE_DEBUG_STACKOVERFLOW
select OR1K_PIC
select CPU_NO_EFFICIENT_FFS if !OPENRISC_HAVE_INST_FF1
- select ARCH_USE_QUEUED_SPINLOCKS
select ARCH_USE_QUEUED_RWLOCKS
select OMPIC if SMP
select ARCH_WANT_FRAME_POINTERS
diff --git a/arch/openrisc/include/asm/Kbuild b/arch/openrisc/include/asm/Kbuild
index ca5987e11053..1df59e446b46 100644
--- a/arch/openrisc/include/asm/Kbuild
+++ b/arch/openrisc/include/asm/Kbuild
@@ -1,9 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
generic-y += extable.h
generic-y += kvm_para.h
-generic-y += mcs_spinlock.h
-generic-y += qspinlock_types.h
-generic-y += qspinlock.h
-generic-y += qrwlock_types.h
-generic-y += qrwlock.h
+generic-y += spinlock_types.h
+generic-y += spinlock.h
generic-y += user.h
diff --git a/arch/openrisc/include/asm/spinlock.h b/arch/openrisc/include/asm/spinlock.h
deleted file mode 100644
index 264944a71535..000000000000
--- a/arch/openrisc/include/asm/spinlock.h
+++ /dev/null
@@ -1,27 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * OpenRISC Linux
- *
- * Linux architectural port borrowing liberally from similar works of
- * others. All original copyrights apply as per the original source
- * declaration.
- *
- * OpenRISC implementation:
- * Copyright (C) 2003 Matjaz Breskvar <[email protected]>
- * Copyright (C) 2010-2011 Jonas Bonn <[email protected]>
- * et al.
- */
-
-#ifndef __ASM_OPENRISC_SPINLOCK_H
-#define __ASM_OPENRISC_SPINLOCK_H
-
-#include <asm/qspinlock.h>
-
-#include <asm/qrwlock.h>
-
-#define arch_spin_relax(lock) cpu_relax()
-#define arch_read_relax(lock) cpu_relax()
-#define arch_write_relax(lock) cpu_relax()
-
-
-#endif
diff --git a/arch/openrisc/include/asm/spinlock_types.h b/arch/openrisc/include/asm/spinlock_types.h
deleted file mode 100644
index 7c6fb1208c88..000000000000
--- a/arch/openrisc/include/asm/spinlock_types.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#ifndef _ASM_OPENRISC_SPINLOCK_TYPES_H
-#define _ASM_OPENRISC_SPINLOCK_TYPES_H
-
-#include <asm/qspinlock_types.h>
-#include <asm/qrwlock_types.h>
-
-#endif /* _ASM_OPENRISC_SPINLOCK_TYPES_H */
--
2.25.1
From: Peter Zijlstra <[email protected]>
This is a simple, fair spinlock. Specifically it doesn't have all the
subtle memory model dependencies that qspinlock has, which makes it more
suitable for simple systems as it is more likely to be correct.
[Palmer: commit text]
Signed-off-by: Palmer Dabbelt <[email protected]>
--
I have specifically not included Peter's SOB on this, as he sent his
original patch
<https://lore.kernel.org/lkml/[email protected]/>
without one.
---
include/asm-generic/spinlock.h | 11 +++-
include/asm-generic/spinlock_types.h | 15 +++++
include/asm-generic/ticket-lock-types.h | 11 ++++
include/asm-generic/ticket-lock.h | 86 +++++++++++++++++++++++++
4 files changed, 120 insertions(+), 3 deletions(-)
create mode 100644 include/asm-generic/spinlock_types.h
create mode 100644 include/asm-generic/ticket-lock-types.h
create mode 100644 include/asm-generic/ticket-lock.h
diff --git a/include/asm-generic/spinlock.h b/include/asm-generic/spinlock.h
index adaf6acab172..a8e2aa1bcea4 100644
--- a/include/asm-generic/spinlock.h
+++ b/include/asm-generic/spinlock.h
@@ -1,12 +1,17 @@
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef __ASM_GENERIC_SPINLOCK_H
#define __ASM_GENERIC_SPINLOCK_H
+
/*
- * You need to implement asm/spinlock.h for SMP support. The generic
- * version does not handle SMP.
+ * Using ticket-spinlock.h as generic for SMP support.
*/
#ifdef CONFIG_SMP
-#error need an architecture specific asm/spinlock.h
+#include <asm-generic/ticket-lock.h>
+#ifdef CONFIG_QUEUED_RWLOCKS
+#include <asm-generic/qrwlock.h>
+#else
+#error Please select ARCH_USE_QUEUED_RWLOCKS in architecture Kconfig
+#endif
#endif
#endif /* __ASM_GENERIC_SPINLOCK_H */
diff --git a/include/asm-generic/spinlock_types.h b/include/asm-generic/spinlock_types.h
new file mode 100644
index 000000000000..ba8ef4b731ba
--- /dev/null
+++ b/include/asm-generic/spinlock_types.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_GENERIC_SPINLOCK_TYPES_H
+#define __ASM_GENERIC_SPINLOCK_TYPES_H
+
+/*
+ * Using ticket spinlock as generic for SMP support.
+ */
+#ifdef CONFIG_SMP
+#include <asm-generic/ticket-lock-types.h>
+#include <asm-generic/qrwlock_types.h>
+#else
+#error The asm-generic/spinlock_types.h is not for CONFIG_SMP=n
+#endif
+
+#endif /* __ASM_GENERIC_SPINLOCK_TYPES_H */
diff --git a/include/asm-generic/ticket-lock-types.h b/include/asm-generic/ticket-lock-types.h
new file mode 100644
index 000000000000..829759aedda8
--- /dev/null
+++ b/include/asm-generic/ticket-lock-types.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_GENERIC_TICKET_LOCK_TYPES_H
+#define __ASM_GENERIC_TICKET_LOCK_TYPES_H
+
+#include <linux/types.h>
+typedef atomic_t arch_spinlock_t;
+
+#define __ARCH_SPIN_LOCK_UNLOCKED ATOMIC_INIT(0)
+
+#endif /* __ASM_GENERIC_TICKET_LOCK_TYPES_H */
diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
new file mode 100644
index 000000000000..59373de3e32a
--- /dev/null
+++ b/include/asm-generic/ticket-lock.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * 'Generic' ticket-lock implementation.
+ *
+ * It relies on atomic_fetch_add() having well defined forward progress
+ * guarantees under contention. If your architecture cannot provide this, stick
+ * to a test-and-set lock.
+ *
+ * It also relies on atomic_fetch_add() being safe vs smp_store_release() on a
+ * sub-word of the value. This is generally true for anything LL/SC although
+ * you'd be hard pressed to find anything useful in architecture specifications
+ * about this. If your architecture cannot do this you might be better off with
+ * a test-and-set.
+ *
+ * It further assumes atomic_*_release() + atomic_*_acquire() is RCpc and hence
+ * uses atomic_fetch_add() which is SC to create an RCsc lock.
+ *
+ * The implementation uses smp_cond_load_acquire() to spin, so if the
+ * architecture has WFE like instructions to sleep instead of poll for word
+ * modifications be sure to implement that (see ARM64 for example).
+ *
+ */
+
+#ifndef __ASM_GENERIC_TICKET_LOCK_H
+#define __ASM_GENERIC_TICKET_LOCK_H
+
+#include <linux/atomic.h>
+#include <asm-generic/ticket-lock-types.h>
+
+static __always_inline void ticket_lock(arch_spinlock_t *lock)
+{
+ u32 val = atomic_fetch_add(1<<16, lock); /* SC, gives us RCsc */
+ u16 ticket = val >> 16;
+
+ if (ticket == (u16)val)
+ return;
+
+ atomic_cond_read_acquire(lock, ticket == (u16)VAL);
+}
+
+static __always_inline bool ticket_trylock(arch_spinlock_t *lock)
+{
+ u32 old = atomic_read(lock);
+
+ if ((old >> 16) != (old & 0xffff))
+ return false;
+
+ return atomic_try_cmpxchg(lock, &old, old + (1<<16)); /* SC, for RCsc */
+}
+
+static __always_inline void ticket_unlock(arch_spinlock_t *lock)
+{
+ u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
+ u32 val = atomic_read(lock);
+
+ smp_store_release(ptr, (u16)val + 1);
+}
+
+static __always_inline int ticket_is_locked(arch_spinlock_t *lock)
+{
+ u32 val = atomic_read(lock);
+
+ return ((val >> 16) != (val & 0xffff));
+}
+
+static __always_inline int ticket_is_contended(arch_spinlock_t *lock)
+{
+ u32 val = atomic_read(lock);
+
+ return (s16)((val >> 16) - (val & 0xffff)) > 1;
+}
+
+static __always_inline int ticket_value_unlocked(arch_spinlock_t lock)
+{
+ return !ticket_is_locked(&lock);
+}
+
+#define arch_spin_lock(l) ticket_lock(l)
+#define arch_spin_trylock(l) ticket_trylock(l)
+#define arch_spin_unlock(l) ticket_unlock(l)
+#define arch_spin_is_locked(l) ticket_is_locked(l)
+#define arch_spin_is_contended(l) ticket_is_contended(l)
+#define arch_spin_value_unlocked(l) ticket_value_unlocked(l)
+
+#endif /* __ASM_GENERIC_TICKET_LOCK_H */
--
2.25.1
On Sat, Mar 19, 2022 at 4:54 AM <[email protected]> wrote:
> /*
> - * You need to implement asm/spinlock.h for SMP support. The generic
> - * version does not handle SMP.
> + * Using ticket-spinlock.h as generic for SMP support.
> */
> #ifdef CONFIG_SMP
> -#error need an architecture specific asm/spinlock.h
> +#include <asm-generic/ticket-lock.h>
> +#ifdef CONFIG_QUEUED_RWLOCKS
> +#include <asm-generic/qrwlock.h>
> +#else
> +#error Please select ARCH_USE_QUEUED_RWLOCKS in architecture Kconfig
> +#endif
> #endif
There is no need for the !CONFIG_SMP case, as asm/spinlock.h only ever
gets included for SMP builds in the first place. This was already a mistake
in the existing code, but your change would be the time to fix it.
I would also drop the !CONFIG_QUEUED_RWLOCKS case, just include
it unconditionally. If any architecture wants the ticket spinlock in
combination with a custom rwlock, they can simply include the
asm-generic/ticket-lock.h from their asm/spinlock.h, but more
likely any architecture that can use the ticket spinlock will also
want the qrwlock anyway.
Arnd
Hello,
There is a problem with this patch on Big Endian machines, see below.
On Sat, Mar 19, 2022 at 11:54:53AM +0800, [email protected] wrote:
> From: Peter Zijlstra <[email protected]>
>
> This is a simple, fair spinlock. Specifically it doesn't have all the
> subtle memory model dependencies that qspinlock has, which makes it more
> suitable for simple systems as it is more likely to be correct.
>
> [Palmer: commit text]
> Signed-off-by: Palmer Dabbelt <[email protected]>
>
> --
>
> I have specifically not included Peter's SOB on this, as he sent his
> original patch
> <https://lore.kernel.org/lkml/[email protected]/>
> without one.
> ---
> include/asm-generic/spinlock.h | 11 +++-
> include/asm-generic/spinlock_types.h | 15 +++++
> include/asm-generic/ticket-lock-types.h | 11 ++++
> include/asm-generic/ticket-lock.h | 86 +++++++++++++++++++++++++
> 4 files changed, 120 insertions(+), 3 deletions(-)
> create mode 100644 include/asm-generic/spinlock_types.h
> create mode 100644 include/asm-generic/ticket-lock-types.h
> create mode 100644 include/asm-generic/ticket-lock.h
>
> diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
> new file mode 100644
> index 000000000000..59373de3e32a
> --- /dev/null
> +++ b/include/asm-generic/ticket-lock.h
...
> +static __always_inline void ticket_unlock(arch_spinlock_t *lock)
> +{
> + u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
As mentioned, this patch series breaks SMP on OpenRISC. I traced it to this
line. The above `__is_defined(__BIG_ENDIAN)` does not return 1 as expected
even on BIG_ENDIAN machines. This works:
diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
index 59373de3e32a..52b5dc9ffdba 100644
--- a/include/asm-generic/ticket-lock.h
+++ b/include/asm-generic/ticket-lock.h
@@ -26,6 +26,7 @@
#define __ASM_GENERIC_TICKET_LOCK_H
#include <linux/atomic.h>
+#include <linux/kconfig.h>
#include <asm-generic/ticket-lock-types.h>
static __always_inline void ticket_lock(arch_spinlock_t *lock)
@@ -51,7 +52,7 @@ static __always_inline bool ticket_trylock(arch_spinlock_t *lock)
static __always_inline void ticket_unlock(arch_spinlock_t *lock)
{
- u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
+ u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
u32 val = atomic_read(lock);
smp_store_release(ptr, (u16)val + 1);
> + u32 val = atomic_read(lock);
> +
> + smp_store_release(ptr, (u16)val + 1);
> +}
> +
Hi
On Sun, Mar 20, 2022 at 12:05 PM Guo Ren <[email protected]> wrote:
>
> Hi openrisc guys,
>
> > kernel/signal.c:2625:49: sparse: expected struct sighand_struct *sighand
> > kernel/signal.c:2625:49: sparse: got struct sighand_struct [noderef] __rcu *sighand
>
> Some warning here, Is that all right? I don't think it is because of
> changing arch_spinlock_t from struct qspinlock to atomic_t.
>
> On Sun, Mar 20, 2022 at 8:07 AM kernel test robot <[email protected]> wrote:
> >
> > Hi,
> >
> > I love your patch! Perhaps something to improve:
> >
> > [auto build test WARNING on arnd-asm-generic/master]
> > [also build test WARNING on tip/locking/core openrisc/for-next linus/master v5.17-rc8 next-20220318]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> >
> > url: https://github.com/0day-ci/linux/commits/guoren-kernel-org/Generic-Ticket-Spinlocks/20220319-115644
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master
> > config: openrisc-randconfig-s032-20220319 (https://download.01.org/0day-ci/archive/20220320/[email protected]/config)
> > compiler: or1k-linux-gcc (GCC) 11.2.0
> > reproduce:
> > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # apt-get install sparse
> > # sparse version: v0.6.4-dirty
> > # https://github.com/0day-ci/linux/commit/4e66dc8c71c62011bcb287f66bf5c5363920cd91
> > git remote add linux-review https://github.com/0day-ci/linux
> > git fetch --no-tags linux-review guoren-kernel-org/Generic-Ticket-Spinlocks/20220319-115644
> > git checkout 4e66dc8c71c62011bcb287f66bf5c5363920cd91
> > # save the config file to linux build tree
> > mkdir build_dir
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=openrisc SHELL=/bin/bash
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <[email protected]>
> >
> >
> > sparse warnings: (new ones prefixed by >>)
> > kernel/signal.c: note: in included file (through include/uapi/asm-generic/signal.h, include/asm-generic/signal.h, arch/openrisc/include/generated/uapi/asm/signal.h, ...):
> > include/uapi/asm-generic/signal-defs.h:83:29: sparse: sparse: multiple address spaces given
> > kernel/signal.c:195:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
> > kernel/signal.c:195:31: sparse: expected struct spinlock [usertype] *lock
> > kernel/signal.c:195:31: sparse: got struct spinlock [noderef] __rcu *
> > kernel/signal.c:198:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
> > kernel/signal.c:198:33: sparse: expected struct spinlock [usertype] *lock
> > kernel/signal.c:198:33: sparse: got struct spinlock [noderef] __rcu *
> > kernel/signal.c:480:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
> > kernel/signal.c:480:9: sparse: expected struct spinlock [usertype] *lock
> > kernel/signal.c:480:9: sparse: got struct spinlock [noderef] __rcu *
> > kernel/signal.c:484:34: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
> > kernel/signal.c:484:34: sparse: expected struct spinlock [usertype] *lock
> > kernel/signal.c:484:34: sparse: got struct spinlock [noderef] __rcu *
> > kernel/signal.c:517:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
> > kernel/signal.c:517:9: sparse: expected struct spinlock [usertype] *lock
> > kernel/signal.c:517:9: sparse: got struct spinlock [noderef] __rcu *
> > kernel/signal.c:520:36: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
> > kernel/signal.c:520:36: sparse: expected struct spinlock [usertype] *lock
> > kernel/signal.c:520:36: sparse: got struct spinlock [noderef] __rcu *
> > kernel/signal.c:542:53: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected struct k_sigaction *ka @@ got struct k_sigaction [noderef] __rcu * @@
> > kernel/signal.c:542:53: sparse: expected struct k_sigaction *ka
> > kernel/signal.c:542:53: sparse: got struct k_sigaction [noderef] __rcu *
> > include/uapi/asm-generic/signal-defs.h:83:29: sparse: sparse: multiple address spaces given
> > kernel/signal.c:698:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
> > kernel/signal.c:698:33: sparse: expected struct spinlock [usertype] *lock
> > kernel/signal.c:698:33: sparse: got struct spinlock [noderef] __rcu *
> > kernel/signal.c:700:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
> > kernel/signal.c:700:31: sparse: expected struct spinlock [usertype] *lock
> > kernel/signal.c:700:31: sparse: got struct spinlock [noderef] __rcu *
> > >> kernel/signal.c:887:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct atomic_t [usertype] *lock @@ got struct atomic_t [noderef] __rcu * @@
> > kernel/signal.c:887:9: sparse: expected struct atomic_t [usertype] *lock
> > kernel/signal.c:887:9: sparse: got struct atomic_t [noderef] __rcu *
This one is being reported as a new warning. I can see this warning
even when testing with x86_64 which uses qspinlock:
kernel/signal.c:542:53: got struct k_sigaction [noderef] __rcu *
./include/uapi/asm-generic/signal-defs.h:83:29: error: multiple
address spaces given
kernel/signal.c:698:33: warning: incorrect type in argument 1
(different address spaces)
kernel/signal.c:698:33: expected struct spinlock [usertype] *lock
kernel/signal.c:698:33: got struct spinlock [noderef] __rcu *
kernel/signal.c:700:31: warning: incorrect type in argument 1
(different address spaces)
kernel/signal.c:700:31: expected struct spinlock [usertype] *lock
kernel/signal.c:700:31: got struct spinlock [noderef] __rcu *
kernel/signal.c:887:9: warning: incorrect type in argument 1
(different address spaces)
kernel/signal.c:887:9: expected struct qspinlock *lock
kernel/signal.c:887:9: got struct qspinlock [noderef] __rcu *
kernel/signal.c:1082:9: warning: incorrect type in argument 1
(different address spaces)
kernel/signal.c:1082:9: expected struct qspinlock *lock
It looks like these are treated as *new* by the kbuild robot because
they changed from qspinlock to atomic_t, which changes the match
pattern.
I looked into what it takes to fix it but it seems such a change would
be very intrusive. I think we can leave as is for now.
-Stafford
On 3/21/22 23:10, Stafford Horne wrote:
> Hello,
>
> There is a problem with this patch on Big Endian machines, see below.
>
> On Sat, Mar 19, 2022 at 11:54:53AM +0800, [email protected] wrote:
>> From: Peter Zijlstra <[email protected]>
>>
>> This is a simple, fair spinlock. Specifically it doesn't have all the
>> subtle memory model dependencies that qspinlock has, which makes it more
>> suitable for simple systems as it is more likely to be correct.
>>
>> [Palmer: commit text]
>> Signed-off-by: Palmer Dabbelt <[email protected]>
>>
>> --
>>
>> I have specifically not included Peter's SOB on this, as he sent his
>> original patch
>> <https://lore.kernel.org/lkml/[email protected]/>
>> without one.
>> ---
>> include/asm-generic/spinlock.h | 11 +++-
>> include/asm-generic/spinlock_types.h | 15 +++++
>> include/asm-generic/ticket-lock-types.h | 11 ++++
>> include/asm-generic/ticket-lock.h | 86 +++++++++++++++++++++++++
>> 4 files changed, 120 insertions(+), 3 deletions(-)
>> create mode 100644 include/asm-generic/spinlock_types.h
>> create mode 100644 include/asm-generic/ticket-lock-types.h
>> create mode 100644 include/asm-generic/ticket-lock.h
>>
>> diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
>> new file mode 100644
>> index 000000000000..59373de3e32a
>> --- /dev/null
>> +++ b/include/asm-generic/ticket-lock.h
> ...
>
>> +static __always_inline void ticket_unlock(arch_spinlock_t *lock)
>> +{
>> + u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
> As mentioned, this patch series breaks SMP on OpenRISC. I traced it to this
> line. The above `__is_defined(__BIG_ENDIAN)` does not return 1 as expected
> even on BIG_ENDIAN machines. This works:
>
>
> diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
> index 59373de3e32a..52b5dc9ffdba 100644
> --- a/include/asm-generic/ticket-lock.h
> +++ b/include/asm-generic/ticket-lock.h
> @@ -26,6 +26,7 @@
> #define __ASM_GENERIC_TICKET_LOCK_H
>
> #include <linux/atomic.h>
> +#include <linux/kconfig.h>
> #include <asm-generic/ticket-lock-types.h>
>
> static __always_inline void ticket_lock(arch_spinlock_t *lock)
> @@ -51,7 +52,7 @@ static __always_inline bool ticket_trylock(arch_spinlock_t *lock)
>
> static __always_inline void ticket_unlock(arch_spinlock_t *lock)
> {
> - u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
> + u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
> u32 val = atomic_read(lock);
>
> smp_store_release(ptr, (u16)val + 1);
>
>
>> + u32 val = atomic_read(lock);
>> +
>> + smp_store_release(ptr, (u16)val + 1);
>> +}
>> +
__BIG_ENDIAN is defined in <linux/kconfig.h>. I believe that if you
include <linux/kconfig.h>, the second hunk is not really needed and vice
versa.
Cheers,
Longman
On Tue, Mar 22, 2022 at 11:54:37AM -0400, Waiman Long wrote:
> On 3/21/22 23:10, Stafford Horne wrote:
> > Hello,
> >
> > There is a problem with this patch on Big Endian machines, see below.
> >
> > On Sat, Mar 19, 2022 at 11:54:53AM +0800, [email protected] wrote:
> > > From: Peter Zijlstra <[email protected]>
> > >
> > > This is a simple, fair spinlock. Specifically it doesn't have all the
> > > subtle memory model dependencies that qspinlock has, which makes it more
> > > suitable for simple systems as it is more likely to be correct.
> > >
> > > [Palmer: commit text]
> > > Signed-off-by: Palmer Dabbelt <[email protected]>
> > >
> > > --
> > >
> > > I have specifically not included Peter's SOB on this, as he sent his
> > > original patch
> > > <https://lore.kernel.org/lkml/[email protected]/>
> > > without one.
> > > ---
> > > include/asm-generic/spinlock.h | 11 +++-
> > > include/asm-generic/spinlock_types.h | 15 +++++
> > > include/asm-generic/ticket-lock-types.h | 11 ++++
> > > include/asm-generic/ticket-lock.h | 86 +++++++++++++++++++++++++
> > > 4 files changed, 120 insertions(+), 3 deletions(-)
> > > create mode 100644 include/asm-generic/spinlock_types.h
> > > create mode 100644 include/asm-generic/ticket-lock-types.h
> > > create mode 100644 include/asm-generic/ticket-lock.h
> > >
> > > diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
> > > new file mode 100644
> > > index 000000000000..59373de3e32a
> > > --- /dev/null
> > > +++ b/include/asm-generic/ticket-lock.h
> > ...
> >
> > > +static __always_inline void ticket_unlock(arch_spinlock_t *lock)
> > > +{
> > > + u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
> > As mentioned, this patch series breaks SMP on OpenRISC. I traced it to this
> > line. The above `__is_defined(__BIG_ENDIAN)` does not return 1 as expected
> > even on BIG_ENDIAN machines. This works:
> >
> >
> > diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
> > index 59373de3e32a..52b5dc9ffdba 100644
> > --- a/include/asm-generic/ticket-lock.h
> > +++ b/include/asm-generic/ticket-lock.h
> > @@ -26,6 +26,7 @@
> > #define __ASM_GENERIC_TICKET_LOCK_H
> > #include <linux/atomic.h>
> > +#include <linux/kconfig.h>
> > #include <asm-generic/ticket-lock-types.h>
> > static __always_inline void ticket_lock(arch_spinlock_t *lock)
> > @@ -51,7 +52,7 @@ static __always_inline bool ticket_trylock(arch_spinlock_t *lock)
> > static __always_inline void ticket_unlock(arch_spinlock_t *lock)
> > {
> > - u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
> > + u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
> > u32 val = atomic_read(lock);
> > smp_store_release(ptr, (u16)val + 1);
> >
> >
> > > + u32 val = atomic_read(lock);
> > > +
> > > + smp_store_release(ptr, (u16)val + 1);
> > > +}
> > > +
>
> __BIG_ENDIAN is defined in <linux/kconfig.h>. I believe that if you include
> <linux/kconfig.h>, the second hunk is not really needed and vice versa.
I thought so too, but it doesn't seem to work. I think __is_defined is not
doing what we think in this context. It looks like __is_defined works when a
macro is defined as 1, in this case we have __BIG_ENDIAN 4321.
With just the first hunk, we can see we still get 0 for the lock offset as per
below:
diff --git a/include/asm-generic/ticket-lock.h
b/include/asm-generic/ticket-lock.h
index 59373de3e32a..769561fb6997 100644
--- a/include/asm-generic/ticket-lock.h
+++ b/include/asm-generic/ticket-lock.h
@@ -26,6 +26,7 @@
#define __ASM_GENERIC_TICKET_LOCK_H
#include <linux/atomic.h>
+#include <linux/kconfig.h>
#include <asm-generic/ticket-lock-types.h>
static __always_inline void ticket_lock(arch_spinlock_t *lock)
--
make ARCH=openrisc simple_smp_defconfig
make ARCH=openrisc CROSS_COMPILE=or1k-linux- kernel/locking/spinlock.i
grep -C3 'lock +' kernel/locking/spinlock.i
static inline __attribute__((__gnu_inline__)) __attribute__((__unused__)) __attribute__((__no_instrument_function__)) __attribute__((__always_inline__)) void
ticket_unlock(arch_spinlock_t *lock)
{
u16 *ptr = (u16 *)lock + 0;
u32 val = atomic_read(lock);
-Stafford
On 3/22/22 17:06, Stafford Horne wrote:
> On Tue, Mar 22, 2022 at 11:54:37AM -0400, Waiman Long wrote:
>> On 3/21/22 23:10, Stafford Horne wrote:
>>> Hello,
>>>
>>> There is a problem with this patch on Big Endian machines, see below.
>>>
>>> On Sat, Mar 19, 2022 at 11:54:53AM +0800, [email protected] wrote:
>>>> From: Peter Zijlstra <[email protected]>
>>>>
>>>> This is a simple, fair spinlock. Specifically it doesn't have all the
>>>> subtle memory model dependencies that qspinlock has, which makes it more
>>>> suitable for simple systems as it is more likely to be correct.
>>>>
>>>> [Palmer: commit text]
>>>> Signed-off-by: Palmer Dabbelt <[email protected]>
>>>>
>>>> --
>>>>
>>>> I have specifically not included Peter's SOB on this, as he sent his
>>>> original patch
>>>> <https://lore.kernel.org/lkml/[email protected]/>
>>>> without one.
>>>> ---
>>>> include/asm-generic/spinlock.h | 11 +++-
>>>> include/asm-generic/spinlock_types.h | 15 +++++
>>>> include/asm-generic/ticket-lock-types.h | 11 ++++
>>>> include/asm-generic/ticket-lock.h | 86 +++++++++++++++++++++++++
>>>> 4 files changed, 120 insertions(+), 3 deletions(-)
>>>> create mode 100644 include/asm-generic/spinlock_types.h
>>>> create mode 100644 include/asm-generic/ticket-lock-types.h
>>>> create mode 100644 include/asm-generic/ticket-lock.h
>>>>
>>>> diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
>>>> new file mode 100644
>>>> index 000000000000..59373de3e32a
>>>> --- /dev/null
>>>> +++ b/include/asm-generic/ticket-lock.h
>>> ...
>>>
>>>> +static __always_inline void ticket_unlock(arch_spinlock_t *lock)
>>>> +{
>>>> + u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
>>> As mentioned, this patch series breaks SMP on OpenRISC. I traced it to this
>>> line. The above `__is_defined(__BIG_ENDIAN)` does not return 1 as expected
>>> even on BIG_ENDIAN machines. This works:
>>>
>>>
>>> diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h
>>> index 59373de3e32a..52b5dc9ffdba 100644
>>> --- a/include/asm-generic/ticket-lock.h
>>> +++ b/include/asm-generic/ticket-lock.h
>>> @@ -26,6 +26,7 @@
>>> #define __ASM_GENERIC_TICKET_LOCK_H
>>> #include <linux/atomic.h>
>>> +#include <linux/kconfig.h>
>>> #include <asm-generic/ticket-lock-types.h>
>>> static __always_inline void ticket_lock(arch_spinlock_t *lock)
>>> @@ -51,7 +52,7 @@ static __always_inline bool ticket_trylock(arch_spinlock_t *lock)
>>> static __always_inline void ticket_unlock(arch_spinlock_t *lock)
>>> {
>>> - u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN);
>>> + u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
>>> u32 val = atomic_read(lock);
>>> smp_store_release(ptr, (u16)val + 1);
>>>
>>>
>>>> + u32 val = atomic_read(lock);
>>>> +
>>>> + smp_store_release(ptr, (u16)val + 1);
>>>> +}
>>>> +
>> __BIG_ENDIAN is defined in <linux/kconfig.h>. I believe that if you include
>> <linux/kconfig.h>, the second hunk is not really needed and vice versa.
> I thought so too, but it doesn't seem to work. I think __is_defined is not
> doing what we think in this context. It looks like __is_defined works when a
> macro is defined as 1, in this case we have __BIG_ENDIAN 4321.
You are right. __is_defined() only for 1 or not 1. So it can't be used
for __BIG_ENDIAN.
I was not aware of that. Anyway, the <linux/kconfig.h> include is not
really needed then.
Cheers,
Longman