semaphore: Add might_sleep() to down_*() family
PATCH 1/1 semaphore: Add might_sleep() to down_*() family
Semaphore is sleeping lock. Add might_sleep() to down*() family (with exception of down_trylock()) to detect atomic context sleep.
Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]
--- kernel/locking/semaphore.c | 4 ++++ 1 file changed, 4 insertions(+)
kernel/locking/semaphore.c +4 -0
@@ -54,6 +54,7 @@ void down(struct semaphore *sem) {unsigned long flags; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
@@ -77,6 +78,7 @@ int down_interruptible(struct semaphore *sem)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
@@ -103,6 +105,7 @@ int down_killable(struct semaphore *sem)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
@@ -157,6 +160,7 @@ int down_timeout(struct semaphore *sem, long timeout)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
Signature · 1 line
2.27.0
Quoted 40 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]Signed-off-by Xiaoming Ni <[email protected]>--- kernel/locking/semaphore.c | 4 ++++ 1 file changed, 4 insertions(+)kernel/locking/semaphore.c +3
-0@@ -54,6 +54,7 @@ void down(struct semaphore *sem) {unsigned long flags; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;@@ -77,6 +78,7 @@ int down_interruptible(struct semaphore *sem)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;@@ -103,6 +105,7 @@ int down_killable(struct semaphore *sem)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
kernel/locking/semaphore.c int down_timeout(struct semaphore *sem, long timeout) · line 166
Quoted 7 lines
kernel/locking/semaphore.c +1
-0@@ -157,6 +160,7 @@ int down_timeout(struct semaphore *sem, long timeout)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
I think it is simpler to just put a "might_sleep()" in __down_common() which is the function where sleep can actually happen.
Cheers,
Longman
Quoted 40 lines · Waiman Long
Quoted 39 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]
Signed-off-by Xiaoming Ni <[email protected]>---
kernel/locking/semaphore.c| 4 ++++1 file changed, 4 insertions(+)kernel/locking/semaphore.c +3
-0@@ -54,6 +54,7 @@ void down(struct semaphore *sem) { unsigned long flags; + might_sleep(); raw_spin_lock_irqsave(&sem->lock, flags); if (likely(sem->count > 0)) sem->count--;@@ -77,6 +78,7 @@ int down_interruptible(struct semaphore *sem) unsigned long flags; int result = 0; + might_sleep(); raw_spin_lock_irqsave(&sem->lock, flags); if (likely(sem->count > 0)) sem->count--;@@ -103,6 +105,7 @@ int down_killable(struct semaphore *sem) unsigned long flags; int result = 0; + might_sleep(); raw_spin_lock_irqsave(&sem->lock, flags); if (likely(sem->count > 0)) sem->count--;@@ -157,6 +160,7 @@ int down_timeout(struct semaphore *sem, long timeout)
Quoted 9 lines
kernel/locking/semaphore.c int down_interruptible(struct semaphore *sem) · line 84
Quoted 6 lines
kernel/locking/semaphore.c +1
-0unsigned long flags; int result = 0; + might_sleep(); raw_spin_lock_irqsave(&sem->lock, flags); if (likely(sem->count > 0)) sem->count--;I think it is simpler to just put a "
might_sleep()" in__down_common()which is the function where sleep can actually happen.
If the actual atomic context hibernation occurs, the corresponding alarm log is generated in __schedule_bug().
__schedule() --> schedule_debug() --> __schedule_bug()
However, "might_sleep()" indicates the possibility of sleep, so that code writers can identify and fix the problem as soon as possible, but does not trigger atomic context sleep.
Is it better to put "might_sleep()" in each down API entry than __down_common() to help identify potential code problems?
Thanks
Xiaoming Ni
Quoted 5 lines · Xiaoming Ni
Quoted 3 lines · Waiman Long
I think it is simpler to just put a "
might_sleep()" in__down_common()which is the function where sleep can actually happen.
Quoted 8 lines
If the actual atomic context hibernation occurs, the corresponding alarm log is generated in
__schedule_bug().__schedule()
-->schedule_debug()
-->__schedule_bug()However, "
might_sleep()" indicates the possibility of sleep, so that code writers can identify and fix the problem as soon as possible, but does not trigger atomic context sleep.Is it better to put "
might_sleep()" in each down API entry than__down_common()to help identify potential code problems?
Putting "might_sleep()" in each down_*() functions mean that whenever we add a new API function, we have to remember to add "might_sleep()". If we put it in down_common(), it will work for any newly added API function in the future even though I doubt we will add any.
Cheers,
Longman
Quoted 11 lines · Waiman Long
Quoted 10 lines · Xiaoming Ni
Quoted 3 lines · Waiman Long
I think it is simpler to just put a "
might_sleep()" in__down_common()which is the function where sleep can actually happen.If the actual atomic context hibernation occurs, the corresponding alarm log is generated in
__schedule_bug().__schedule()
-->schedule_debug()
-->__schedule_bug()
Quoted 7 lines
Quoted 3 lines
However, "
might_sleep()" indicates the possibility of sleep, so that code writers can identify and fix the problem as soon as possible, but does not trigger atomic context sleep.Is it better to put "
might_sleep()" in each down API entry than__down_common()to help identify potential code problems?Putting "
might_sleep()" in each down_*() functions mean that whenever we add a new API function, we have to remember to add "might_sleep()". If we put it indown_common(), it will work for any newly added API function in the future even though I doubt we will add any.
If the code enters down_common(), it is not "might" sleep but "will" sleep, and an alarm is printed in __schedule_bug() later.
"might_sleep()" is used to check potential problems, and "_schedule_bug()" is used to check actual faults.
So, I still think we should add "might_sleep()" to each down_*() function to alert code owner to potential problems early.
Thanks
Xiaoming Ni
Quoted 7 lines · Waiman Long
Quoted 6 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.kernel/locking/semaphore.c +0
-0@@ -157,6 +160,7 @@ int down_timeout(struct semaphore *sem, long timeout)unsigned long flags;int result =0 ;
Quoted 7 lines
kernel/locking/semaphore.c void down(struct semaphore *sem) · line 60
Quoted 4 lines
kernel/locking/semaphore.c +1
-0+might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;I think it is simpler to just put a "
might_sleep()" in__down_common()which is the function where sleep can actually happen.
No. Putting it in __down_common() is wrong, because that covers only the contended case.
But we want to cover the potential sleep, i.e. checking even in the non-contended case, which is what might_sleep() is about.
Thanks,
tglx
Quoted 14 lines · Thomas Gleixner
Quoted 7 lines · Waiman Long
Quoted 6 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.kernel/locking/semaphore.c +0
-0@@ -157,6 +160,7 @@ int down_timeout(struct semaphore *sem, long timeout)unsigned long flags;int result =0 ;Quoted 6 lines
kernel/locking/semaphore.c void down(struct semaphore *sem) · line 60
Quoted 4 lines
kernel/locking/semaphore.c +1
-0+might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;I think it is simpler to just put a "
might_sleep()" in__down_common()which is the function where sleep can actually happen.
Quoted 5 lines
No. Putting it in
__down_common()is wrong, because that covers only the contended case.But we want to cover the potential sleep, i.e. checking even in the non-contended case, which is what
might_sleep()is about.
You are right. Thanks for the correction.
Cheers,
Longman
Quoted 5 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]
You can use a Link: tag here.
Quoted 4 lines
Signed-off-by Xiaoming Ni <[email protected]>--- kernel/locking/semaphore.c | 4 ++++ 1 file changed, 4 insertions(+)
Will
[tip: locking/core] locking/semaphore: Add might_sleep() to down_*() family
In reply to Xiaoming NiThe following commit has been merged into the locking/core branch of tip:
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 99409b935c9ac5ea36ab5218954115c52449234d Gitweb: https://git.kernel.org/tip/99409b935c9ac5ea36ab5218954115c52449234d Author: Xiaoming Ni <[email protected]> AuthorDate: Mon, 09 Aug 2021 10:12:15 +08:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 20 Aug 2021 12:33:17 +02:00
locking/semaphore: Add might_sleep() to down_*() family
Semaphore is sleeping lock. Add might_sleep() to down*() family (with exception of down_trylock()) to detect atomic context sleep.
--- kernel/locking/semaphore.c | 4 ++++ 1 file changed, 4 insertions(+)
kernel/locking/semaphore.c +4 -0
@@ -54,6 +54,7 @@ void down(struct semaphore *sem) {unsigned long flags; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
@@ -77,6 +78,7 @@ int down_interruptible(struct semaphore *sem)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
@@ -103,6 +105,7 @@ int down_killable(struct semaphore *sem)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
@@ -157,6 +160,7 @@ int down_timeout(struct semaphore *sem, long timeout)unsigned long flags;int result =0 ; +might_sleep ();raw_spin_lock_irqsave (&sem->lock, flags);if (likely (sem->count >0 )) sem->count--;
Hi,
Quoted 8 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]Signed-off-by Xiaoming Ni <[email protected]>Acked-by Will Deacon <[email protected]>
This patch results in the following traceback on all arm64 boots with EFI BIOS.
The problem is only seen with CONFIG_ACPI_PPTT=y, and thus only on arm64.
Guenter
---
[ 14.048540] BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163
[ 14.048700] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 14, name: cpuhp/0
[ 14.048865] 2 locks held by cpuhp/0/14:
[ 14.048943] #0: ffff8000125799b0 (cpu_hotplug_lock){++++}-{0:0}, at: cpuhp_thread_fun+0x38/0x254
[ 14.049320] #1: ffff8000125799d8 (cpuhp_state-up){+.+.}-{0:0}, at: cpuhp_thread_fun+0x38/0x254
[ 14.049523] irq event stamp: 62
[ 14.049580] hardirqs last enabled at (61): [<ffff800010269690>] finish_task_switch.isra.0+0xd0/0x2f0
[ 14.049689] hardirqs last disabled at (62): [<ffff800010313ce8>] generic_exec_single+0x138/0x190
[ 14.049785] softirqs last enabled at (0): [<ffff8000102245d4>] copy_process+0x634/0x1af4
[ 14.049876] softirqs last disabled at (0): [<0000000000000000>] 0x0
[ 14.050299] CPU: 0 PID: 14 Comm: cpuhp/0 Not tainted 5.14.0-01100-gb91db6a0b52e #1
[ 14.050452] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
[ 14.050694] Call trace:
[ 14.050753] dump_backtrace+0x0/0x19c
[ 14.050839] show_stack+0x1c/0x30
[ 14.050892] dump_stack_lvl+0x9c/0xd8
[ 14.050949] dump_stack+0x1c/0x38
[ 14.050999] ___might_sleep+0x154/0x200
[ 14.051053] __might_sleep+0x54/0x90
[ 14.051106] down_timeout+0x34/0x90
[ 14.051159] acpi_os_wait_semaphore+0x68/0x9c
[ 14.051218] acpi_ut_acquire_mutex+0x50/0xbc
[ 14.051277] acpi_get_table+0x3c/0xc0
[ 14.051330] acpi_find_last_cache_level+0x44/0x12c
[ 14.051391] _init_cache_level+0xd8/0xe4
[ 14.051446] generic_exec_single+0xf8/0x190
[ 14.051502] smp_call_function_single+0x174/0x1e0
[ 14.051561] init_cache_level+0x30/0x60
[ 14.051614] cacheinfo_cpu_online+0x28/0x840
[ 14.051675] cpuhp_invoke_callback+0x168/0x2ac
[ 14.051751] cpuhp_thread_fun+0x198/0x254
[ 14.051810] smpboot_thread_fn+0x200/0x2c0
[ 14.051867] kthread+0x164/0x170
[ 14.051921] ret_from_fork+0x10/0x18Hi Guenter,
Quoted 9 lines · Guenter Roeck
Hi,
Quoted 6 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]
Quoted 7 lines
Quoted 2 lines
Signed-off-by Xiaoming Ni <[email protected]>Acked-by Will Deacon <[email protected]>This patch results in the following traceback on all arm64 boots with EFI BIOS.
The problem is only seen with
CONFIG_ACPI_PPTT=y, and thus only on arm64.
I Cced Xiongfeng, he sent a patch to fix this issue over a month ago:
https://lore.kernel.org/linux-arm-kernel/[email protected]/T/
and the calltrace is exactly the same as below.
Sudeep, would you mind take a look again?
Quoted 27 lines
---
[ 14.048540] BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163 [ 14.048700] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 14, name: cpuhp/0 [ 14.048865] 2 locks held by cpuhp/0/14: [ 14.048943] #0: ffff8000125799b0 (cpu_hotplug_lock){++++}-{0:0}, at: cpuhp_thread_fun+0x38/0x254 [ 14.049320] #1: ffff8000125799d8 (cpuhp_state-up){+.+.}-{0:0}, at: cpuhp_thread_fun+0x38/0x254 [ 14.049523] irq event stamp: 62 [ 14.049580] hardirqs last enabled at (61): [<ffff800010269690>] finish_task_switch.isra.0+0xd0/0x2f0 [ 14.049689] hardirqs last disabled at (62): [<ffff800010313ce8>] generic_exec_single+0x138/0x190 [ 14.049785] softirqs last enabled at (0): [<ffff8000102245d4>] copy_process+0x634/0x1af4 [ 14.049876] softirqs last disabled at (0): [<0000000000000000>] 0x0 [ 14.050299] CPU: 0 PID: 14 Comm: cpuhp/0 Not tainted 5.14.0-01100-gb91db6a0b52e #1 [ 14.050452] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015 [ 14.050694] Call trace: [ 14.050753] dump_backtrace+0x0/0x19c [ 14.050839] show_stack+0x1c/0x30 [ 14.050892] dump_stack_lvl+0x9c/0xd8 [ 14.050949] dump_stack+0x1c/0x38 [ 14.050999] ___might_sleep+0x154/0x200 [ 14.051053] __might_sleep+0x54/0x90 [ 14.051106] down_timeout+0x34/0x90 [ 14.051159] acpi_os_wait_semaphore+0x68/0x9c [ 14.051218] acpi_ut_acquire_mutex+0x50/0xbc [ 14.051277] acpi_get_table+0x3c/0xc0 [ 14.051330] acpi_find_last_cache_level+0x44/0x12c [ 14.051391] _init_cache_level+0xd8/0xe4 [ 14.051446] generic_exec_single+0xf8/0x190
Quoted 8 lines
[ 14.051502] smp_call_function_single+0x174/0x1e0 [ 14.051561] init_cache_level+0x30/0x60 [ 14.051614] cacheinfo_cpu_online+0x28/0x840 [ 14.051675] cpuhp_invoke_callback+0x168/0x2ac [ 14.051751] cpuhp_thread_fun+0x198/0x254 [ 14.051810] smpboot_thread_fn+0x200/0x2c0 [ 14.051867] kthread+0x164/0x170 [ 14.051921] ret_from_fork+0x10/0x18
Thanks
Hanjun
Quoted 9 lines · Hanjun Guo
Quoted 4 lines · Guenter Roeck
This patch results in the following traceback on all arm64 boots with EFI BIOS.
The problem is only seen with
CONFIG_ACPI_PPTT=y, and thus only on arm64.I Cced Xiongfeng, he sent a patch to fix this issue over a month ago:
https://lore.kernel.org/linux-arm-kernel/[email protected]/T/
OMG. None of this is necessary.
Peter Zijlstra
maintainerQuoted 39 lines · Guenter Roeck
Hi,
Quoted 8 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]Signed-off-by Xiaoming Ni <[email protected]>Acked-by Will Deacon <[email protected]>This patch results in the following traceback on all arm64 boots with EFI BIOS.
The problem is only seen with
CONFIG_ACPI_PPTT=y, and thus only on arm64.Guenter
---
[ 14.048540] BUG: sleeping function called from invalid context at kernel/locking/semaphore.c:163 [ 14.048700] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 14, name: cpuhp/0 [ 14.048865] 2 locks held by cpuhp/0/14: [ 14.048943] #0: ffff8000125799b0 (cpu_hotplug_lock){++++}-{0:0}, at: cpuhp_thread_fun+0x38/0x254 [ 14.049320] #1: ffff8000125799d8 (cpuhp_state-up){+.+.}-{0:0}, at: cpuhp_thread_fun+0x38/0x254 [ 14.049523] irq event stamp: 62 [ 14.049580] hardirqs last enabled at (61): [<ffff800010269690>] finish_task_switch.isra.0+0xd0/0x2f0 [ 14.049689] hardirqs last disabled at (62): [<ffff800010313ce8>] generic_exec_single+0x138/0x190 [ 14.049785] softirqs last enabled at (0): [<ffff8000102245d4>] copy_process+0x634/0x1af4 [ 14.049876] softirqs last disabled at (0): [<0000000000000000>] 0x0 [ 14.050299] CPU: 0 PID: 14 Comm: cpuhp/0 Not tainted 5.14.0-01100-gb91db6a0b52e #1 [ 14.050452] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015 [ 14.050694] Call trace: [ 14.050753] dump_backtrace+0x0/0x19c [ 14.050839] show_stack+0x1c/0x30 [ 14.050892] dump_stack_lvl+0x9c/0xd8 [ 14.050949] dump_stack+0x1c/0x38 [ 14.050999] ___might_sleep+0x154/0x200 [ 14.051053] __might_sleep+0x54/0x90
Quoted 8 lines
[ 14.051106] down_timeout+0x34/0x90 [ 14.051159] acpi_os_wait_semaphore+0x68/0x9c [ 14.051218] acpi_ut_acquire_mutex+0x50/0xbc [ 14.051277] acpi_get_table+0x3c/0xc0 [ 14.051330] acpi_find_last_cache_level+0x44/0x12c [ 14.051391] _init_cache_level+0xd8/0xe4 [ 14.051446] generic_exec_single+0xf8/0x190 [ 14.051502] smp_call_function_single+0x174/0x1e0
This is the patch working as intended.. You simply cannot schedule with interrupts disabled as per the callchain.
Quoted 6 lines · Guenter Roeck
Hi,
Quoted 3 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.
Quoted 8 lines
Quoted 5 lines
Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]Signed-off-by Xiaoming Ni <[email protected]>Acked-by Will Deacon <[email protected]>This patch results in the following traceback on all arm64 boots with EFI BIOS.
That's what this change was supposed to catch :)
Quoted 1 line
The problem is only seen with
CONFIG_ACPI_PPTT=y, and thus only on arm64.
The below should fix this.
Thanks,
tglx
---
Subject: drivers: base: cacheinfo: Get rid of DEFINE_SMP_CALL_CACHE_FUNCTION()
From: Thomas Gleixner <[email protected]>
Date: Tue, 31 Aug 2021 13:48:34 +0200
DEFINE_SMP_CALL_CACHE_FUNCTION() was usefel before the CPU hotplug rework to ensure that the cache related functions are called on the upcoming CPU because the notifier itself could run on any online CPU.
The hotplug state machine guarantees that the callbacks are invoked on the upcoming CPU. So there is no need to have this SMP function call obfuscation. That indirection was missed when the hotplug notifiers were converted.
This also solves the problem of ARM64 init_cache_level() invoking ACPI functions which take a semaphore in that context. That's invalid as SMP function calls run with interrupts disabled. Running it just from the callback in context of the CPU hotplug thread solves this.
--- arch/arm64/kernel/cacheinfo.c | 7 ++----- arch/mips/kernel/cacheinfo.c | 7 ++----- arch/riscv/kernel/cacheinfo.c | 7 ++----- arch/x86/kernel/cpu/cacheinfo.c | 7 ++----- include/linux/cacheinfo.h | 18 ------------------ 5 files changed, 8 insertions(+), 38 deletions(-)
arch/arm64/kernel/cacheinfo.c +2 -5
@@ -43,7 +43,7 @@ static void ci_leaf_init(struct cacheinf this_leaf->type = type; } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {unsigned int ctype, level, leaves, fw_level;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -78,7 +78,7 @@ static int __init_cache_level(unsigned ireturn 0 ; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {unsigned int level, idx;enum cache_type type;
@@ -97,6 +97,3 @@ static int __populate_cache_leaves(unsig }return 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
arch/mips/kernel/cacheinfo.c +2 -5
@@ -17,7 +17,7 @@ do { \ leaf++; \ }while (0 ) -static int __init_cache_level(unsigned int cpu) +int init_cache_level(unsigned int cpu) {struct cpuinfo_mips *c = ¤t_cpu_data;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -74,7 +74,7 @@ static void fill_cpumask_cluster(int cpucpumask_set_cpu (cpu1, cpu_map); } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {struct cpuinfo_mips *c = ¤t_cpu_data;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -114,6 +114,3 @@ static int __populate_cache_leaves(unsigreturn 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
arch/riscv/kernel/cacheinfo.c +2 -5
@@ -113,7 +113,7 @@ static void fill_cacheinfo(struct cachei } } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct device_node *np =of_cpu_device_node_get (cpu);
@@ -155,7 +155,7 @@ static int __init_cache_level(unsigned ireturn 0 ; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list;
@@ -187,6 +187,3 @@ static int __populate_cache_leaves(unsigreturn 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
arch/x86/kernel/cpu/cacheinfo.c +2 -5
@@ -985,7 +985,7 @@ static void ci_leaf_init(struct cacheinf this_leaf->priv = base->nb; } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -1014,7 +1014,7 @@ static void get_cache_id(int cpu, struct id4_regs->id = c->apicid >> index_msb; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {unsigned int idx, ret;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -1033,6 +1033,3 @@ static int __populate_cache_leaves(unsigreturn 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
include/linux/cacheinfo.h +0 -18
@@ -79,24 +79,6 @@ struct cpu_cacheinfo {bool cpu_map_populated; }; -/* -* Helpers to make sure "func" is executed on the cpu whose cache -* attributes are being detected -*/ -#define DEFINE_SMP_CALL_CACHE_FUNCTION (func) \ -static inline void _##func(void *ret) \ -{ \ - int cpu = smp_processor_id(); \ - *(int *)ret = __##func(cpu); \ -} \ - \ -int func(unsigned int cpu) \ -{ \ - int ret; \ - smp_call_function_single(cpu, _##func, &ret, true); \ - return ret; \ -} -struct cpu_cacheinfo *get_cpu_cacheinfo (unsigned int cpu);int init_cache_level (unsigned int cpu);int populate_cache_leaves (unsigned int cpu);
Quoted 22 lines · Thomas Gleixner
Quoted 4 lines · Guenter Roeck
Quoted 3 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.Quoted 8 lines
Quoted 5 lines
Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]Signed-off-by Xiaoming Ni <[email protected]>Acked-by Will Deacon <[email protected]>This patch results in the following traceback on all arm64 boots with EFI BIOS.
That's what this change was supposed to catch :)
Quoted 1 line
The problem is only seen with
CONFIG_ACPI_PPTT=y, and thus only on arm64.The below should fix this.
Thanks,
Quoted 7 lines
tglx
---
Subject: drivers: base: cacheinfo: Get rid ofDEFINE_SMP_CALL_CACHE_FUNCTION()
From: Thomas Gleixner <[email protected]>
Date: Tue, 31 Aug 2021 13:48:34 +0200
DEFINE_SMP_CALL_CACHE_FUNCTION()was usefel before the CPU hotplug rework
typo: "usefel"
Quoted 45 lines
to ensure that the cache related functions are called on the upcoming CPU because the notifier itself could run on any online CPU.
The hotplug state machine guarantees that the callbacks are invoked on the upcoming CPU. So there is no need to have this SMP function call obfuscation. That indirection was missed when the hotplug notifiers were converted.
This also solves the problem of ARM64
init_cache_level()invoking ACPI functions which take a semaphore in that context. That's invalid as SMP function calls run with interrupts disabled. Running it just from the callback in context of the CPU hotplug thread solves this.Reported-by Guenter Roeck <[email protected]>Fixes 8571890e1513 ("arm64: Add support for ACPI based firmware tables")Signed-off-by Thomas Gleixner <[email protected]>--- arch/arm64/kernel/cacheinfo.c | 7 ++----- arch/mips/kernel/cacheinfo.c | 7 ++----- arch/riscv/kernel/cacheinfo.c | 7 ++----- arch/x86/kernel/cpu/cacheinfo.c | 7 ++----- include/linux/cacheinfo.h | 18 ------------------ 5 files changed, 8 insertions(+), 38 deletions(-)arch/arm64/kernel/cacheinfo.c +2
-2@@ -43,7 +43,7 @@ static void ci_leaf_init(struct cacheinf this_leaf->type = type; } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {unsigned int ctype, level, leaves, fw_level;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);@@ -78,7 +78,7 @@ static int __init_cache_level(unsigned ireturn 0 ; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {unsigned int level, idx;enum cache_type type;
arch/arm64/kernel/cacheinfo.c static int __populate_cache_leaves(unsig · line 102
Quoted 6 lines
arch/arm64/kernel/cacheinfo.c +0
-3@@ -97,6 +97,3 @@ static int __populate_cache_leaves(unsig }return 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
Glad to see the back of this:
Will
Guenter Roeck
Tested-byQuoted 40 lines · Thomas Gleixner
Quoted 6 lines · Guenter Roeck
Hi,
Quoted 3 lines · Xiaoming Ni
Semaphore is sleeping lock. Add
might_sleep()to down*() family (with exception ofdown_trylock()) to detect atomic context sleep.Quoted 8 lines
Quoted 5 lines
Previously discussed with Peter Zijlstra, see link:
https://lore.kernel.org/lkml/[email protected]Signed-off-by Xiaoming Ni <[email protected]>Acked-by Will Deacon <[email protected]>This patch results in the following traceback on all arm64 boots with EFI BIOS.
That's what this change was supposed to catch :)
Quoted 1 line
The problem is only seen with
CONFIG_ACPI_PPTT=y, and thus only on arm64.The below should fix this.
Thanks,
tglx
---
Subject: drivers: base: cacheinfo: Get rid ofDEFINE_SMP_CALL_CACHE_FUNCTION()
From: Thomas Gleixner <[email protected]>
Date: Tue, 31 Aug 2021 13:48:34 +0200
DEFINE_SMP_CALL_CACHE_FUNCTION()was usefel before the CPU hotplug rework to ensure that the cache related functions are called on the upcoming CPU because the notifier itself could run on any online CPU.The hotplug state machine guarantees that the callbacks are invoked on the upcoming CPU. So there is no need to have this SMP function call obfuscation. That indirection was missed when the hotplug notifiers were converted.
Quoted 8 lines
This also solves the problem of ARM64
init_cache_level()invoking ACPI functions which take a semaphore in that context. That's invalid as SMP function calls run with interrupts disabled. Running it just from the callback in context of the CPU hotplug thread solves this.Reported-by Guenter Roeck <[email protected]>Fixes 8571890e1513 ("arm64: Add support for ACPI based firmware tables")Signed-off-by Thomas Gleixner <[email protected]>
The warning is no longer seen with this patch applied on top of v5.14-1100-gb91db6a0b52e, and I don't see any new problems on riscv, x86/x86_64, or mips.
Thanks,
Guenter
Quoted 143 lines
--- arch/arm64/kernel/cacheinfo.c | 7 ++----- arch/mips/kernel/cacheinfo.c | 7 ++----- arch/riscv/kernel/cacheinfo.c | 7 ++----- arch/x86/kernel/cpu/cacheinfo.c | 7 ++----- include/linux/cacheinfo.h | 18 ------------------ 5 files changed, 8 insertions(+), 38 deletions(-)arch/arm64/kernel/cacheinfo.c +2
-5@@ -43,7 +43,7 @@ static void ci_leaf_init(struct cacheinf this_leaf->type = type; } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {unsigned int ctype, level, leaves, fw_level;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);@@ -78,7 +78,7 @@ static int __init_cache_level(unsigned ireturn 0 ; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {unsigned int level, idx;enum cache_type type;@@ -97,6 +97,3 @@ static int __populate_cache_leaves(unsig }return 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)arch/mips/kernel/cacheinfo.c +2
-5@@ -17,7 +17,7 @@ do { \ leaf++; \ }while (0 ) -static int __init_cache_level(unsigned int cpu) +int init_cache_level(unsigned int cpu) {struct cpuinfo_mips *c = ¤t_cpu_data;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);@@ -74,7 +74,7 @@ static void fill_cpumask_cluster(int cpucpumask_set_cpu (cpu1, cpu_map); } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {struct cpuinfo_mips *c = ¤t_cpu_data;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);@@ -114,6 +114,3 @@ static int __populate_cache_leaves(unsigreturn 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)arch/riscv/kernel/cacheinfo.c +2
-5@@ -113,7 +113,7 @@ static void fill_cacheinfo(struct cachei } } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct device_node *np =of_cpu_device_node_get (cpu);@@ -155,7 +155,7 @@ static int __init_cache_level(unsigned ireturn 0 ; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list;@@ -187,6 +187,3 @@ static int __populate_cache_leaves(unsigreturn 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)arch/x86/kernel/cpu/cacheinfo.c +2
-5@@ -985,7 +985,7 @@ static void ci_leaf_init(struct cacheinf this_leaf->priv = base->nb; } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);@@ -1014,7 +1014,7 @@ static void get_cache_id(int cpu, struct id4_regs->id = c->apicid >> index_msb; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {unsigned int idx, ret;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);@@ -1033,6 +1033,3 @@ static int __populate_cache_leaves(unsigreturn 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)include/linux/cacheinfo.h +0
-18@@ -79,24 +79,6 @@ struct cpu_cacheinfo {bool cpu_map_populated; }; -/* -* Helpers to make sure "func" is executed on the cpu whose cache -* attributes are being detected -*/ -#define DEFINE_SMP_CALL_CACHE_FUNCTION (func) \ -static inline void _##func(void *ret) \ -{ \ - int cpu = smp_processor_id(); \ - *(int *)ret = __##func(cpu); \ -} \ - \ -int func(unsigned int cpu) \ -{ \ - int ret; \ - smp_call_function_single(cpu, _##func, &ret, true); \ - return ret; \ -} -struct cpu_cacheinfo *get_cpu_cacheinfo (unsigned int cpu);int init_cache_level (unsigned int cpu);int populate_cache_leaves (unsigned int cpu);
[tip: smp/urgent] drivers: base: cacheinfo: Get rid of DEFINE_SMP_CALL_CACHE_FUNCTION()
In reply to Thomas GleixnerThe following commit has been merged into the smp/urgent branch of tip:
The following commit has been merged into the smp/urgent branch of tip:
Commit-ID: 4b92d4add5f6dcf21275185c997d6ecb800054cd Gitweb: https://git.kernel.org/tip/4b92d4add5f6dcf21275185c997d6ecb800054cd Author: Thomas Gleixner <[email protected]> AuthorDate: Tue, 31 Aug 2021 13:48:34 +02:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Wed, 01 Sep 2021 10:29:10 +02:00
drivers: base: cacheinfo: Get rid of DEFINE_SMP_CALL_CACHE_FUNCTION()
DEFINE_SMP_CALL_CACHE_FUNCTION() was usefel before the CPU hotplug rework to ensure that the cache related functions are called on the upcoming CPU because the notifier itself could run on any online CPU.
The hotplug state machine guarantees that the callbacks are invoked on the upcoming CPU. So there is no need to have this SMP function call obfuscation. That indirection was missed when the hotplug notifiers were converted.
This also solves the problem of ARM64 init_cache_level() invoking ACPI functions which take a semaphore in that context. That's invalid as SMP function calls run with interrupts disabled. Running it just from the callback in context of the CPU hotplug thread solves this.
--- arch/arm64/kernel/cacheinfo.c | 7 ++----- arch/mips/kernel/cacheinfo.c | 7 ++----- arch/riscv/kernel/cacheinfo.c | 7 ++----- arch/x86/kernel/cpu/cacheinfo.c | 7 ++----- include/linux/cacheinfo.h | 18 ------------------ 5 files changed, 8 insertions(+), 38 deletions(-)
arch/arm64/kernel/cacheinfo.c +2 -5
@@ -43,7 +43,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, this_leaf->type = type; } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {unsigned int ctype, level, leaves, fw_level;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -78,7 +78,7 @@ static int __init_cache_level(unsigned int cpu)return 0 ; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {unsigned int level, idx;enum cache_type type;
@@ -97,6 +97,3 @@ static int __populate_cache_leaves(unsigned int cpu) }return 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
arch/mips/kernel/cacheinfo.c +2 -5
@@ -17,7 +17,7 @@ do { \ leaf++; \ }while (0 ) -static int __init_cache_level(unsigned int cpu) +int init_cache_level(unsigned int cpu) {struct cpuinfo_mips *c = ¤t_cpu_data;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -74,7 +74,7 @@ static void fill_cpumask_cluster(int cpu, cpumask_t *cpu_map)cpumask_set_cpu (cpu1, cpu_map); } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {struct cpuinfo_mips *c = ¤t_cpu_data;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -114,6 +114,3 @@ static int __populate_cache_leaves(unsigned int cpu)return 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
arch/riscv/kernel/cacheinfo.c +2 -5
@@ -113,7 +113,7 @@ static void fill_cacheinfo(struct cacheinfo **this_leaf, } } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct device_node *np =of_cpu_device_node_get (cpu);
@@ -155,7 +155,7 @@ static int __init_cache_level(unsigned int cpu)return 0 ; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list;
@@ -187,6 +187,3 @@ static int __populate_cache_leaves(unsigned int cpu)return 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
arch/x86/kernel/cpu/cacheinfo.c +2 -5
@@ -985,7 +985,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, this_leaf->priv = base->nb; } -static int __init_cache_level (unsigned int cpu) +int init_cache_level (unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -1014,7 +1014,7 @@ static void get_cache_id(int cpu, struct _cpuid4_info_regs *id4_regs) id4_regs->id = c->apicid >> index_msb; } -static int __populate_cache_leaves (unsigned int cpu) +int populate_cache_leaves (unsigned int cpu) {unsigned int idx, ret;struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);
@@ -1033,6 +1033,3 @@ static int __populate_cache_leaves(unsigned int cpu)return 0 ; } - -DEFINE_SMP_CALL_CACHE_FUNCTION (init_cache_level) -DEFINE_SMP_CALL_CACHE_FUNCTION (populate_cache_leaves)
include/linux/cacheinfo.h +0 -18
@@ -79,24 +79,6 @@ struct cpu_cacheinfo {bool cpu_map_populated; }; -/* -* Helpers to make sure "func" is executed on the cpu whose cache -* attributes are being detected -*/ -#define DEFINE_SMP_CALL_CACHE_FUNCTION (func) \ -static inline void _##func(void *ret) \ -{ \ - int cpu = smp_processor_id(); \ - *(int *)ret = __##func(cpu); \ -} \ - \ -int func(unsigned int cpu) \ -{ \ - int ret; \ - smp_call_function_single(cpu, _##func, &ret, true); \ - return ret; \ -} -struct cpu_cacheinfo *get_cpu_cacheinfo (unsigned int cpu);int init_cache_level (unsigned int cpu);int populate_cache_leaves (unsigned int cpu);