LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] semaphore: Add might_sleep() to down_*() family
@ 2021-08-09  2:12 Xiaoming Ni
  2021-08-09  3:01 ` Waiman Long
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Xiaoming Ni @ 2021-08-09  2:12 UTC (permalink / raw)
  To: linux-kernel, peterz, mingo, will, longman, boqun.feng
  Cc: nixiaoming, wangle6, xiaoqian9, shaolexi

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/20210806082320.GD22037@worktop.programming.kicks-ass.net

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 kernel/locking/semaphore.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
index 9aa855a96c4a..9ee381e4d2a4 100644
--- a/kernel/locking/semaphore.c
+++ b/kernel/locking/semaphore.c
@@ -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--;
-- 
2.27.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-09  2:12 [PATCH] semaphore: Add might_sleep() to down_*() family Xiaoming Ni
@ 2021-08-09  3:01 ` Waiman Long
  2021-08-09  3:51   ` Xiaoming Ni
  2021-08-13 17:27   ` Thomas Gleixner
  2021-08-13 14:43 ` Will Deacon
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 17+ messages in thread
From: Waiman Long @ 2021-08-09  3:01 UTC (permalink / raw)
  To: Xiaoming Ni, linux-kernel, peterz, mingo, will, boqun.feng
  Cc: wangle6, xiaoqian9, shaolexi

On 8/8/21 10:12 PM, Xiaoming Ni wrote:
> 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/20210806082320.GD22037@worktop.programming.kicks-ass.net
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
>   kernel/locking/semaphore.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
> index 9aa855a96c4a..9ee381e4d2a4 100644
> --- a/kernel/locking/semaphore.c
> +++ b/kernel/locking/semaphore.c
> @@ -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--;

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-09  3:01 ` Waiman Long
@ 2021-08-09  3:51   ` Xiaoming Ni
  2021-08-09 12:52     ` Waiman Long
  2021-08-13 17:27   ` Thomas Gleixner
  1 sibling, 1 reply; 17+ messages in thread
From: Xiaoming Ni @ 2021-08-09  3:51 UTC (permalink / raw)
  To: Waiman Long, linux-kernel, peterz, mingo, will, boqun.feng
  Cc: wangle6, xiaoqian9, shaolexi

On 2021/8/9 11:01, Waiman Long wrote:
> On 8/8/21 10:12 PM, Xiaoming Ni wrote:
>> 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/20210806082320.GD22037@worktop.programming.kicks-ass.net 
>>
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>> ---
>>   kernel/locking/semaphore.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
>> index 9aa855a96c4a..9ee381e4d2a4 100644
>> --- a/kernel/locking/semaphore.c
>> +++ b/kernel/locking/semaphore.c
>> @@ -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--;
> 
> 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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-09  3:51   ` Xiaoming Ni
@ 2021-08-09 12:52     ` Waiman Long
  2021-08-09 14:33       ` Xiaoming Ni
  0 siblings, 1 reply; 17+ messages in thread
From: Waiman Long @ 2021-08-09 12:52 UTC (permalink / raw)
  To: Xiaoming Ni, Waiman Long, linux-kernel, peterz, mingo, will, boqun.feng
  Cc: wangle6, xiaoqian9, shaolexi

On 8/8/21 11:51 PM, Xiaoming Ni wrote:
> On 2021/8/9 11:01, Waiman Long wrote:
>>
>> 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? 

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-09 12:52     ` Waiman Long
@ 2021-08-09 14:33       ` Xiaoming Ni
  0 siblings, 0 replies; 17+ messages in thread
From: Xiaoming Ni @ 2021-08-09 14:33 UTC (permalink / raw)
  To: Waiman Long, linux-kernel, peterz, mingo, will, boqun.feng
  Cc: wangle6, xiaoqian9, shaolexi

On 2021/8/9 20:52, Waiman Long wrote:
> On 8/8/21 11:51 PM, Xiaoming Ni wrote:
>> On 2021/8/9 11:01, Waiman Long wrote:
>>>
>>> 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? 
> 
> 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.
> 
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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-09  2:12 [PATCH] semaphore: Add might_sleep() to down_*() family Xiaoming Ni
  2021-08-09  3:01 ` Waiman Long
@ 2021-08-13 14:43 ` Will Deacon
  2021-08-23  9:39 ` [tip: locking/core] locking/semaphore: " tip-bot2 for Xiaoming Ni
  2021-08-31 11:13 ` [PATCH] semaphore: " Guenter Roeck
  3 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2021-08-13 14:43 UTC (permalink / raw)
  To: Xiaoming Ni
  Cc: linux-kernel, peterz, mingo, longman, boqun.feng, wangle6,
	xiaoqian9, shaolexi

On Mon, Aug 09, 2021 at 10:12:15AM +0800, Xiaoming Ni wrote:
> 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/20210806082320.GD22037@worktop.programming.kicks-ass.net

You can use a Link: tag here.

> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
>  kernel/locking/semaphore.c | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Will Deacon <will@kernel.org>

Will

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-09  3:01 ` Waiman Long
  2021-08-09  3:51   ` Xiaoming Ni
@ 2021-08-13 17:27   ` Thomas Gleixner
  2021-08-13 18:47     ` Waiman Long
  1 sibling, 1 reply; 17+ messages in thread
From: Thomas Gleixner @ 2021-08-13 17:27 UTC (permalink / raw)
  To: Waiman Long, Xiaoming Ni, linux-kernel, peterz, mingo, will, boqun.feng
  Cc: wangle6, xiaoqian9, shaolexi

On Sun, Aug 08 2021 at 23:01, Waiman Long wrote:
> On 8/8/21 10:12 PM, Xiaoming Ni wrote:
>> Semaphore is sleeping lock. Add might_sleep() to down*() family
>> (with exception of down_trylock()) to detect atomic context sleep.
>> @@ -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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-13 17:27   ` Thomas Gleixner
@ 2021-08-13 18:47     ` Waiman Long
  0 siblings, 0 replies; 17+ messages in thread
From: Waiman Long @ 2021-08-13 18:47 UTC (permalink / raw)
  To: Thomas Gleixner, Waiman Long, Xiaoming Ni, linux-kernel, peterz,
	mingo, will, boqun.feng
  Cc: wangle6, xiaoqian9, shaolexi

On 8/13/21 1:27 PM, Thomas Gleixner wrote:
> On Sun, Aug 08 2021 at 23:01, Waiman Long wrote:
>> On 8/8/21 10:12 PM, Xiaoming Ni wrote:
>>> Semaphore is sleeping lock. Add might_sleep() to down*() family
>>> (with exception of down_trylock()) to detect atomic context sleep.
>>> @@ -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.
> 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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [tip: locking/core] locking/semaphore: Add might_sleep() to down_*() family
  2021-08-09  2:12 [PATCH] semaphore: Add might_sleep() to down_*() family Xiaoming Ni
  2021-08-09  3:01 ` Waiman Long
  2021-08-13 14:43 ` Will Deacon
@ 2021-08-23  9:39 ` tip-bot2 for Xiaoming Ni
  2021-08-31 11:13 ` [PATCH] semaphore: " Guenter Roeck
  3 siblings, 0 replies; 17+ messages in thread
From: tip-bot2 for Xiaoming Ni @ 2021-08-23  9:39 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Xiaoming Ni, Peter Zijlstra (Intel), Will Deacon, x86, linux-kernel

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 <nixiaoming@huawei.com>
AuthorDate:    Mon, 09 Aug 2021 10:12:15 +08:00
Committer:     Peter Zijlstra <peterz@infradead.org>
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.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Will Deacon <will@kernel.org>
Link: https://lore.kernel.org/r/20210809021215.19991-1-nixiaoming@huawei.com
---
 kernel/locking/semaphore.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
index 9aa855a..9ee381e 100644
--- a/kernel/locking/semaphore.c
+++ b/kernel/locking/semaphore.c
@@ -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--;

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-09  2:12 [PATCH] semaphore: Add might_sleep() to down_*() family Xiaoming Ni
                   ` (2 preceding siblings ...)
  2021-08-23  9:39 ` [tip: locking/core] locking/semaphore: " tip-bot2 for Xiaoming Ni
@ 2021-08-31 11:13 ` Guenter Roeck
  2021-08-31 11:39   ` Hanjun Guo
                     ` (2 more replies)
  3 siblings, 3 replies; 17+ messages in thread
From: Guenter Roeck @ 2021-08-31 11:13 UTC (permalink / raw)
  To: Xiaoming Ni
  Cc: linux-kernel, peterz, mingo, will, longman, boqun.feng, wangle6,
	xiaoqian9, shaolexi, linux-acpi, Lorenzo Pieralisi, Hanjun Guo,
	Sudeep Holla

Hi,

On Mon, Aug 09, 2021 at 10:12:15AM +0800, Xiaoming Ni wrote:
> 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/20210806082320.GD22037@worktop.programming.kicks-ass.net
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> Acked-by: Will Deacon <will@kernel.org>

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/0x18

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-31 11:13 ` [PATCH] semaphore: " Guenter Roeck
@ 2021-08-31 11:39   ` Hanjun Guo
  2021-08-31 12:20     ` Thomas Gleixner
  2021-08-31 11:40   ` Peter Zijlstra
  2021-08-31 12:13   ` Thomas Gleixner
  2 siblings, 1 reply; 17+ messages in thread
From: Hanjun Guo @ 2021-08-31 11:39 UTC (permalink / raw)
  To: Guenter Roeck, Xiaoming Ni
  Cc: linux-kernel, peterz, mingo, will, longman, boqun.feng, wangle6,
	xiaoqian9, shaolexi, linux-acpi, Lorenzo Pieralisi, Sudeep Holla,
	Xiongfeng Wang

Hi Guenter,

On 2021/8/31 19:13, Guenter Roeck wrote:
> Hi,
> 
> On Mon, Aug 09, 2021 at 10:12:15AM +0800, Xiaoming Ni wrote:
>> 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/20210806082320.GD22037@worktop.programming.kicks-ass.net
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>> Acked-by: Will Deacon <will@kernel.org>
> 
> 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/20210720112635.38565-1-wangxiongfeng2@huawei.com/T/

and the calltrace is exactly the same as below.

Sudeep, would you mind take a look again?

> ---
> [   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/0x18

Thanks
Hanjun

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-31 11:13 ` [PATCH] semaphore: " Guenter Roeck
  2021-08-31 11:39   ` Hanjun Guo
@ 2021-08-31 11:40   ` Peter Zijlstra
  2021-08-31 12:13   ` Thomas Gleixner
  2 siblings, 0 replies; 17+ messages in thread
From: Peter Zijlstra @ 2021-08-31 11:40 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Xiaoming Ni, linux-kernel, mingo, will, longman, boqun.feng,
	wangle6, xiaoqian9, shaolexi, linux-acpi, Lorenzo Pieralisi,
	Hanjun Guo, Sudeep Holla

On Tue, Aug 31, 2021 at 04:13:22AM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Mon, Aug 09, 2021 at 10:12:15AM +0800, Xiaoming Ni wrote:
> > 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/20210806082320.GD22037@worktop.programming.kicks-ass.net
> > 
> > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> > Acked-by: Will Deacon <will@kernel.org>
> 
> 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

This is the patch working as intended.. You simply cannot schedule with
interrupts disabled as per the callchain.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-31 11:13 ` [PATCH] semaphore: " Guenter Roeck
  2021-08-31 11:39   ` Hanjun Guo
  2021-08-31 11:40   ` Peter Zijlstra
@ 2021-08-31 12:13   ` Thomas Gleixner
  2021-08-31 12:34     ` Will Deacon
                       ` (2 more replies)
  2 siblings, 3 replies; 17+ messages in thread
From: Thomas Gleixner @ 2021-08-31 12:13 UTC (permalink / raw)
  To: Guenter Roeck, Xiaoming Ni
  Cc: linux-kernel, peterz, mingo, will, longman, boqun.feng, wangle6,
	xiaoqian9, shaolexi, linux-acpi, Lorenzo Pieralisi, Hanjun Guo,
	Sudeep Holla, Greg Kroah-Hartman

On Tue, Aug 31 2021 at 04:13, Guenter Roeck wrote:

> Hi,
>
> On Mon, Aug 09, 2021 at 10:12:15AM +0800, Xiaoming Ni wrote:
>> 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/20210806082320.GD22037@worktop.programming.kicks-ass.net
>> 
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>> Acked-by: Will Deacon <will@kernel.org>
>
> This patch results in the following traceback on all arm64 boots with
> EFI BIOS.

That's what this change was supposed to catch :)

> 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 <tglx@linutronix.de>
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.
 
Reported-by: Guenter Roeck <linux@roeck-us.net>
Fixes: 8571890e1513 ("arm64: Add support for ACPI based firmware tables")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 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(-)

--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -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 i
 	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(unsig
 	}
 	return 0;
 }
-
-DEFINE_SMP_CALL_CACHE_FUNCTION(init_cache_level)
-DEFINE_SMP_CALL_CACHE_FUNCTION(populate_cache_leaves)
--- a/arch/mips/kernel/cacheinfo.c
+++ b/arch/mips/kernel/cacheinfo.c
@@ -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 = &current_cpu_data;
 	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
@@ -74,7 +74,7 @@ static void fill_cpumask_cluster(int cpu
 			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 = &current_cpu_data;
 	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
@@ -114,6 +114,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)
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -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 i
 	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(unsig
 
 	return 0;
 }
-
-DEFINE_SMP_CALL_CACHE_FUNCTION(init_cache_level)
-DEFINE_SMP_CALL_CACHE_FUNCTION(populate_cache_leaves)
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -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(unsig
 
 	return 0;
 }
-
-DEFINE_SMP_CALL_CACHE_FUNCTION(init_cache_level)
-DEFINE_SMP_CALL_CACHE_FUNCTION(populate_cache_leaves)
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -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);

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-31 11:39   ` Hanjun Guo
@ 2021-08-31 12:20     ` Thomas Gleixner
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2021-08-31 12:20 UTC (permalink / raw)
  To: Hanjun Guo, Guenter Roeck, Xiaoming Ni
  Cc: linux-kernel, peterz, mingo, will, longman, boqun.feng, wangle6,
	xiaoqian9, shaolexi, linux-acpi, Lorenzo Pieralisi, Sudeep Holla,
	Xiongfeng Wang

On Tue, Aug 31 2021 at 19:39, Hanjun Guo wrote:
> On 2021/8/31 19:13, Guenter Roeck wrote:
>> 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/20210720112635.38565-1-wangxiongfeng2@huawei.com/T/

OMG. None of this is necessary.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-31 12:13   ` Thomas Gleixner
@ 2021-08-31 12:34     ` Will Deacon
  2021-08-31 17:42     ` Guenter Roeck
  2021-09-01  8:37     ` [tip: smp/urgent] drivers: base: cacheinfo: Get rid of DEFINE_SMP_CALL_CACHE_FUNCTION() tip-bot2 for Thomas Gleixner
  2 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2021-08-31 12:34 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Guenter Roeck, Xiaoming Ni, linux-kernel, peterz, mingo, longman,
	boqun.feng, wangle6, xiaoqian9, shaolexi, linux-acpi,
	Lorenzo Pieralisi, Hanjun Guo, Sudeep Holla, Greg Kroah-Hartman

On Tue, Aug 31, 2021 at 02:13:08PM +0200, Thomas Gleixner wrote:
> On Tue, Aug 31 2021 at 04:13, Guenter Roeck wrote:
> > On Mon, Aug 09, 2021 at 10:12:15AM +0800, Xiaoming Ni wrote:
> >> 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/20210806082320.GD22037@worktop.programming.kicks-ass.net
> >> 
> >> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> >> Acked-by: Will Deacon <will@kernel.org>
> >
> > This patch results in the following traceback on all arm64 boots with
> > EFI BIOS.
> 
> That's what this change was supposed to catch :)
> 
> > 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 <tglx@linutronix.de>
> Date: Tue, 31 Aug 2021 13:48:34 +0200
> 
> DEFINE_SMP_CALL_CACHE_FUNCTION() was usefel before the CPU hotplug rework

typo: "usefel"

> 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 <linux@roeck-us.net>
> Fixes: 8571890e1513 ("arm64: Add support for ACPI based firmware tables")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  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(-)
> 
> --- a/arch/arm64/kernel/cacheinfo.c
> +++ b/arch/arm64/kernel/cacheinfo.c
> @@ -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 i
>  	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(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:

Acked-by: Will Deacon <will@kernel.org>

Will

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] semaphore: Add might_sleep() to down_*() family
  2021-08-31 12:13   ` Thomas Gleixner
  2021-08-31 12:34     ` Will Deacon
@ 2021-08-31 17:42     ` Guenter Roeck
  2021-09-01  8:37     ` [tip: smp/urgent] drivers: base: cacheinfo: Get rid of DEFINE_SMP_CALL_CACHE_FUNCTION() tip-bot2 for Thomas Gleixner
  2 siblings, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2021-08-31 17:42 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Xiaoming Ni, linux-kernel, peterz, mingo, will, longman,
	boqun.feng, wangle6, xiaoqian9, shaolexi, linux-acpi,
	Lorenzo Pieralisi, Hanjun Guo, Sudeep Holla, Greg Kroah-Hartman

On Tue, Aug 31, 2021 at 02:13:08PM +0200, Thomas Gleixner wrote:
> On Tue, Aug 31 2021 at 04:13, Guenter Roeck wrote:
> 
> > Hi,
> >
> > On Mon, Aug 09, 2021 at 10:12:15AM +0800, Xiaoming Ni wrote:
> >> 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/20210806082320.GD22037@worktop.programming.kicks-ass.net
> >> 
> >> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> >> Acked-by: Will Deacon <will@kernel.org>
> >
> > This patch results in the following traceback on all arm64 boots with
> > EFI BIOS.
> 
> That's what this change was supposed to catch :)
> 
> > 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 <tglx@linutronix.de>
> 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.
>  
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 8571890e1513 ("arm64: Add support for ACPI based firmware tables")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

> ---
>  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(-)
> 
> --- a/arch/arm64/kernel/cacheinfo.c
> +++ b/arch/arm64/kernel/cacheinfo.c
> @@ -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 i
>  	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(unsig
>  	}
>  	return 0;
>  }
> -
> -DEFINE_SMP_CALL_CACHE_FUNCTION(init_cache_level)
> -DEFINE_SMP_CALL_CACHE_FUNCTION(populate_cache_leaves)
> --- a/arch/mips/kernel/cacheinfo.c
> +++ b/arch/mips/kernel/cacheinfo.c
> @@ -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 = &current_cpu_data;
>  	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> @@ -74,7 +74,7 @@ static void fill_cpumask_cluster(int cpu
>  			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 = &current_cpu_data;
>  	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> @@ -114,6 +114,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)
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -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 i
>  	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(unsig
>  
>  	return 0;
>  }
> -
> -DEFINE_SMP_CALL_CACHE_FUNCTION(init_cache_level)
> -DEFINE_SMP_CALL_CACHE_FUNCTION(populate_cache_leaves)
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -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(unsig
>  
>  	return 0;
>  }
> -
> -DEFINE_SMP_CALL_CACHE_FUNCTION(init_cache_level)
> -DEFINE_SMP_CALL_CACHE_FUNCTION(populate_cache_leaves)
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -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);

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [tip: smp/urgent] drivers: base: cacheinfo: Get rid of DEFINE_SMP_CALL_CACHE_FUNCTION()
  2021-08-31 12:13   ` Thomas Gleixner
  2021-08-31 12:34     ` Will Deacon
  2021-08-31 17:42     ` Guenter Roeck
@ 2021-09-01  8:37     ` tip-bot2 for Thomas Gleixner
  2 siblings, 0 replies; 17+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2021-09-01  8:37 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Guenter Roeck, Thomas Gleixner, Will Deacon, Peter Zijlstra, x86,
	linux-kernel

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 <tglx@linutronix.de>
AuthorDate:    Tue, 31 Aug 2021 13:48:34 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
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.

Fixes: 8571890e1513 ("arm64: Add support for ACPI based firmware tables")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Will Deacon <will@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/871r69ersb.ffs@tglx
---
 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(-)

diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
index 7fa6828..587543c 100644
--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -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)
diff --git a/arch/mips/kernel/cacheinfo.c b/arch/mips/kernel/cacheinfo.c
index 53d8ea7..495dd05 100644
--- a/arch/mips/kernel/cacheinfo.c
+++ b/arch/mips/kernel/cacheinfo.c
@@ -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 = &current_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 = &current_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)
diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index d867813..90deabf 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -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)
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index d66af29..b5e36bd 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -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)
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 4f72b47..2f909ed 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -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);

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-09-01  8:37 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09  2:12 [PATCH] semaphore: Add might_sleep() to down_*() family Xiaoming Ni
2021-08-09  3:01 ` Waiman Long
2021-08-09  3:51   ` Xiaoming Ni
2021-08-09 12:52     ` Waiman Long
2021-08-09 14:33       ` Xiaoming Ni
2021-08-13 17:27   ` Thomas Gleixner
2021-08-13 18:47     ` Waiman Long
2021-08-13 14:43 ` Will Deacon
2021-08-23  9:39 ` [tip: locking/core] locking/semaphore: " tip-bot2 for Xiaoming Ni
2021-08-31 11:13 ` [PATCH] semaphore: " Guenter Roeck
2021-08-31 11:39   ` Hanjun Guo
2021-08-31 12:20     ` Thomas Gleixner
2021-08-31 11:40   ` Peter Zijlstra
2021-08-31 12:13   ` Thomas Gleixner
2021-08-31 12:34     ` Will Deacon
2021-08-31 17:42     ` Guenter Roeck
2021-09-01  8:37     ` [tip: smp/urgent] drivers: base: cacheinfo: Get rid of DEFINE_SMP_CALL_CACHE_FUNCTION() tip-bot2 for Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).