| Message ID | c395b02613572131568bc1fd1bc456d20d1a5426.1709325647.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH/RFC] locking/spinlocks: Make __raw_* lock ops static | expand |
On 3/1/24 15:43, Geert Uytterhoeven wrote: > sh/sdk7786_defconfig (CONFIG_GENERIC_LOCKBREAK=y and > CONFIG_DEBUG_LOCK_ALLOC=n): > > kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_spin_lock' [-Wmissing-prototypes] > kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_spin_lock_irqsave' [-Wmissing-prototypes] > kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_spin_lock_irq' [-Wmissing-prototypes] > kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_spin_lock_bh' [-Wmissing-prototypes] > kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_read_lock' [-Wmissing-prototypes] > kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_read_lock_irqsave' [-Wmissing-prototypes] > kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_read_lock_irq' [-Wmissing-prototypes] > kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_read_lock_bh' [-Wmissing-prototypes] > kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_write_lock' [-Wmissing-prototypes] > kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_write_lock_irqsave' [-Wmissing-prototypes] > kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_write_lock_irq' [-Wmissing-prototypes] > kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_write_lock_bh' [-Wmissing-prototypes] > > Fix this by making the __raw_* lock ops static. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Compile-tested only. > > Is SH really the only SMP platform where CONFIG_GENERIC_LOCKBREAK=y? > --- > kernel/locking/spinlock.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c > index 8475a0794f8c5ad2..7009b568e6255d64 100644 > --- a/kernel/locking/spinlock.c > +++ b/kernel/locking/spinlock.c > @@ -65,7 +65,7 @@ EXPORT_PER_CPU_SYMBOL(__mmiowb_state); > * towards that other CPU that it should break the lock ASAP. > */ > #define BUILD_LOCK_OPS(op, locktype) \ > -void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ > +static void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ > { \ > for (;;) { \ > preempt_disable(); \ > @@ -77,7 +77,7 @@ void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ > } \ > } \ > \ > -unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ > +static unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ > { \ > unsigned long flags; \ > \ > @@ -95,12 +95,12 @@ unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ > return flags; \ > } \ > \ > -void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock) \ > +static void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock) \ > { \ > _raw_##op##_lock_irqsave(lock); \ > } \ > \ > -void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock) \ > +static void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock) \ > { \ > unsigned long flags; \ > \ This may not work if CONFIG_GENERIC_LOCKBREAK is defined. We had been talking about taking out CONFIG_GENERIC_LOCKBREAK before. See the thread in [1]. However, we didn't proceed further at that time as we weren't totally sure if there were still some configurations that required CONFIG_GENERIC_LOCKBREAK. [1] https://lore.kernel.org/lkml/20211022120058.1031690-1-arnd@kernel.org/ Anyway, without taking out CONFIG_GENERIC_LOCKBREAK, the proper way to fix this issue is probably to declare the proper function prototypes in include/linux/rwlock_api_smp.h and include/linux/spinlock_api_smp.h when CONFIG_GENERIC_LOCKBREAK is defined. Cheers, Longman
Hi Waiman, CC s390 On Sun, Mar 3, 2024 at 5:25 AM Waiman Long <longman@redhat.com> wrote: > On 3/1/24 15:43, Geert Uytterhoeven wrote: > > sh/sdk7786_defconfig (CONFIG_GENERIC_LOCKBREAK=y and > > CONFIG_DEBUG_LOCK_ALLOC=n): > > > > kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_spin_lock' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_spin_lock_irqsave' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_spin_lock_irq' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_spin_lock_bh' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_read_lock' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_read_lock_irqsave' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_read_lock_irq' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_read_lock_bh' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_write_lock' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_write_lock_irqsave' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_write_lock_irq' [-Wmissing-prototypes] > > kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_write_lock_bh' [-Wmissing-prototypes] > > > > Fix this by making the __raw_* lock ops static. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > Compile-tested only. > > > > Is SH really the only SMP platform where CONFIG_GENERIC_LOCKBREAK=y? > > --- > > kernel/locking/spinlock.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c > > index 8475a0794f8c5ad2..7009b568e6255d64 100644 > > --- a/kernel/locking/spinlock.c > > +++ b/kernel/locking/spinlock.c > > @@ -65,7 +65,7 @@ EXPORT_PER_CPU_SYMBOL(__mmiowb_state); > > * towards that other CPU that it should break the lock ASAP. > > */ > > #define BUILD_LOCK_OPS(op, locktype) \ > > -void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ > > +static void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ > > { \ > > for (;;) { \ > > preempt_disable(); \ > > @@ -77,7 +77,7 @@ void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ > > } \ > > } \ > > \ > > -unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ > > +static unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ > > { \ > > unsigned long flags; \ > > \ > > @@ -95,12 +95,12 @@ unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ > > return flags; \ > > } \ > > \ > > -void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock) \ > > +static void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock) \ > > { \ > > _raw_##op##_lock_irqsave(lock); \ > > } \ > > \ > > -void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock) \ > > +static void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock) \ > > { \ > > unsigned long flags; \ > > \ > > This may not work if CONFIG_GENERIC_LOCKBREAK is defined. We had been sdk7786_defconfig sets CONFIG_GENERIC_LOCKBREAK=y? FTR, I checked all defconfigs, and it's set in three of them: - s390/debug_defconfig - sh/sdk7786_defconfig - sh/shx3_defconfig However, the first one has CONFIG_DEBUG_LOCK_ALLOC=y, so the issue does not trigger there (but see below). > talking about taking out CONFIG_GENERIC_LOCKBREAK before. See the thread > in [1]. However, we didn't proceed further at that time as we weren't > totally sure if there were still some configurations that required > CONFIG_GENERIC_LOCKBREAK. > > [1] https://lore.kernel.org/lkml/20211022120058.1031690-1-arnd@kernel.org/ > > Anyway, without taking out CONFIG_GENERIC_LOCKBREAK, the proper way to > fix this issue is probably to declare the proper function prototypes in > include/linux/rwlock_api_smp.h and include/linux/spinlock_api_smp.h when > CONFIG_GENERIC_LOCKBREAK is defined. What is the point of adding function prototypes to header files if the functions don't seem to be called outside kernel/locking/spinlock.c? Or is that part of the breakage? I do not have an sdk7786 or shx3, so I do not know if the kernel actually boots/works. The warnings are also seen with s390/debug_defconfig after changing CONFIG_DEBUG_LOCK_ALLOC=n CONFIG_DEBUG_WW_MUTEX_SLOWPATH=n CONFIG_LOCK_STAT=n CONFIG_PROVE_LOCKING=n Probably that is the easiest config to test on actual hardware? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 3/3/24 11:11, Geert Uytterhoeven wrote: > Hi Waiman, > > CC s390 > > On Sun, Mar 3, 2024 at 5:25 AM Waiman Long <longman@redhat.com> wrote: >> On 3/1/24 15:43, Geert Uytterhoeven wrote: >>> sh/sdk7786_defconfig (CONFIG_GENERIC_LOCKBREAK=y and >>> CONFIG_DEBUG_LOCK_ALLOC=n): >>> >>> kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_spin_lock' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_spin_lock_irqsave' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_spin_lock_irq' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_spin_lock_bh' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_read_lock' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_read_lock_irqsave' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_read_lock_irq' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_read_lock_bh' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_write_lock' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_write_lock_irqsave' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_write_lock_irq' [-Wmissing-prototypes] >>> kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_write_lock_bh' [-Wmissing-prototypes] >>> >>> Fix this by making the __raw_* lock ops static. >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> --- >>> Compile-tested only. >>> >>> Is SH really the only SMP platform where CONFIG_GENERIC_LOCKBREAK=y? >>> --- >>> kernel/locking/spinlock.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c >>> index 8475a0794f8c5ad2..7009b568e6255d64 100644 >>> --- a/kernel/locking/spinlock.c >>> +++ b/kernel/locking/spinlock.c >>> @@ -65,7 +65,7 @@ EXPORT_PER_CPU_SYMBOL(__mmiowb_state); >>> * towards that other CPU that it should break the lock ASAP. >>> */ >>> #define BUILD_LOCK_OPS(op, locktype) \ >>> -void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ >>> +static void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ >>> { \ >>> for (;;) { \ >>> preempt_disable(); \ >>> @@ -77,7 +77,7 @@ void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ >>> } \ >>> } \ >>> \ >>> -unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ >>> +static unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ >>> { \ >>> unsigned long flags; \ >>> \ >>> @@ -95,12 +95,12 @@ unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ >>> return flags; \ >>> } \ >>> \ >>> -void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock) \ >>> +static void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock) \ >>> { \ >>> _raw_##op##_lock_irqsave(lock); \ >>> } \ >>> \ >>> -void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock) \ >>> +static void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock) \ >>> { \ >>> unsigned long flags; \ >>> \ >> This may not work if CONFIG_GENERIC_LOCKBREAK is defined. We had been > sdk7786_defconfig sets CONFIG_GENERIC_LOCKBREAK=y? > > FTR, I checked all defconfigs, and it's set in three of them: > - s390/debug_defconfig > - sh/sdk7786_defconfig > - sh/shx3_defconfig > > However, the first one has CONFIG_DEBUG_LOCK_ALLOC=y, so the issue > does not trigger there (but see below). I was worrying about any of the INLINE_*_LOCK* config being turned on. It turns out that setting GENERIC_LOCKBREAK will not allow those locking functions to be inlined. So my concern is not warranted. With that, I think your patch should be safe. Acked-by: Waiman Long <longman@redhat.com> It will be nice if you can document that either in the change log or in a comment. Still the lock-break lock variants are simple TaS locks with preemption turned on in between successive attempts to acquire the lock. It will be slow and is only suitable for system with small number of cores. The long term goal should be to get rid of these variants and CONFIG_GENERIC_LOCKBREAK if possible. Cheers, Longman
diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c index 8475a0794f8c5ad2..7009b568e6255d64 100644 --- a/kernel/locking/spinlock.c +++ b/kernel/locking/spinlock.c @@ -65,7 +65,7 @@ EXPORT_PER_CPU_SYMBOL(__mmiowb_state); * towards that other CPU that it should break the lock ASAP. */ #define BUILD_LOCK_OPS(op, locktype) \ -void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ +static void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ { \ for (;;) { \ preempt_disable(); \ @@ -77,7 +77,7 @@ void __lockfunc __raw_##op##_lock(locktype##_t *lock) \ } \ } \ \ -unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ +static unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ { \ unsigned long flags; \ \ @@ -95,12 +95,12 @@ unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock) \ return flags; \ } \ \ -void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock) \ +static void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock) \ { \ _raw_##op##_lock_irqsave(lock); \ } \ \ -void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock) \ +static void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock) \ { \ unsigned long flags; \ \
sh/sdk7786_defconfig (CONFIG_GENERIC_LOCKBREAK=y and CONFIG_DEBUG_LOCK_ALLOC=n): kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_spin_lock' [-Wmissing-prototypes] kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_spin_lock_irqsave' [-Wmissing-prototypes] kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_spin_lock_irq' [-Wmissing-prototypes] kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_spin_lock_bh' [-Wmissing-prototypes] kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_read_lock' [-Wmissing-prototypes] kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_read_lock_irqsave' [-Wmissing-prototypes] kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_read_lock_irq' [-Wmissing-prototypes] kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_read_lock_bh' [-Wmissing-prototypes] kernel/locking/spinlock.c:68:17: warning: no previous prototype for '__raw_write_lock' [-Wmissing-prototypes] kernel/locking/spinlock.c:80:26: warning: no previous prototype for '__raw_write_lock_irqsave' [-Wmissing-prototypes] kernel/locking/spinlock.c:98:17: warning: no previous prototype for '__raw_write_lock_irq' [-Wmissing-prototypes] kernel/locking/spinlock.c:103:17: warning: no previous prototype for '__raw_write_lock_bh' [-Wmissing-prototypes] Fix this by making the __raw_* lock ops static. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Compile-tested only. Is SH really the only SMP platform where CONFIG_GENERIC_LOCKBREAK=y? --- kernel/locking/spinlock.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)