[PATCH] smp: give WARN in case of in_interrupt() when calling smp_call_function_many/single

[PATCH] smp: give WARN in case of in_interrupt() when calling smp_call_function_many/single

[Chuansheng Liu]

Currently, in function smp_call_function_many/single, it will give WARN just in case
of irqs_disabled(), but it is not enough.

In many other cases such as softirq handling/interrupt handling, the two APIs still
can not be called, just as the smp_call_function_many() comments said:
 * You must not call this function with disabled interrupts or from a
 * hardware interrupt handler or from a bottom half handler. Preemption
 * must be disabled when calling this function.

There is a real case for softirq DEADLOCK case:
CPUA                            CPUB
                                spin_lock(&spinlock)
                                Any irq coming, call the irq handler
                                irq_exit()
spin_lock_irq(&spinlock)
<== Blocking here due to
CPUB hold it
                                  __do_softirq()
                                    run_timer_softirq()
                                      timer_cb()
                                        call smp_call_function_many()
                                          send IPI interrupt to CPUA
                                            wait_csd()

Then both CPUA and CPUB will be DEADLOCK here.

So we should give WARN in case of in_interrupt(), not only irqd_disabled().

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 kernel/smp.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 69f38bd..a2f0b2c 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -12,6 +12,7 @@
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
+#include <linux/hardirq.h>
 
 #include "smpboot.h"
 
@@ -323,7 +324,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
+	WARN_ON_ONCE(cpu_online(this_cpu) && (irqs_disabled() || in_interrupt())
 		     && !oops_in_progress);
 
 	if (cpu == this_cpu) {
@@ -421,8 +422,9 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
-		     && !oops_in_progress);
+	WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait
+			&& (irqs_disabled() || in_interrupt())
+			&& !oops_in_progress);
 
 	if (cpu == this_cpu) {
 		local_irq_save(flags);
-- 
1.7.0.4

[tip:core/locking] smp: Give WARN()ing if in_interrupt() when calling smp_call_function_many()/single()

[tip-bot for Chuansheng Liu]

Commit-ID:  b29f39c7c3e75a741a7da88244ec707f293ec04c
Gitweb:     http://git.kernel.org/tip/b29f39c7c3e75a741a7da88244ec707f293ec04c
Author:     Chuansheng Liu <chuansheng.liu@intel.com>
AuthorDate: Wed, 6 Feb 2013 23:18:21 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 6 Feb 2013 12:00:30 +0100

smp: Give WARN()ing if in_interrupt() when calling smp_call_function_many()/single()

Currently the functions smp_call_function_many()/single() will
give a WARN()ing only in the case of irqs_disabled(), but that
check is not enough to guarantee execution of the SMP
cross-calls.

In many other cases such as softirq handling/interrupt handling,
the two APIs still can not be called, just as the
smp_call_function_many() comments say:

  * You must not call this function with disabled interrupts or from a
  * hardware interrupt handler or from a bottom half handler. Preemption
  * must be disabled when calling this function.

There is a real case for softirq DEADLOCK case:

CPUA                            CPUB
                                spin_lock(&spinlock)
                                Any irq coming, call the irq handler
                                irq_exit()
spin_lock_irq(&spinlock)
<== Blocking here due to
CPUB hold it
                                  __do_softirq()
                                    run_timer_softirq()
                                      timer_cb()
                                        call smp_call_function_many()
                                          send IPI interrupt to CPUA
                                            wait_csd()

Then both CPUA and CPUB will be deadlocked here.

So we should give a warning in the in_interrupt() case as well.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
Cc: jun.zhang@intel.com
Cc: peterz@infradead.org
Cc: jbeulich@suse.com
Cc: paulmck@linux.vnet.ibm.com
Cc: mina86@mina86.org
Cc: srivatsa.bhat@linux.vnet.ibm.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1360163901.24670.13.camel@cliu38-desktop-build
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/smp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 29dd40a..fec45aa 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -12,6 +12,7 @@
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
+#include <linux/hardirq.h>
 
 #include "smpboot.h"
 
@@ -318,7 +319,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
+	WARN_ON_ONCE(cpu_online(this_cpu) && (irqs_disabled() || in_interrupt())
 		     && !oops_in_progress);
 
 	if (cpu == this_cpu) {
@@ -416,8 +417,9 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
-		     && !oops_in_progress);
+	WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait
+			&& (irqs_disabled() || in_interrupt())
+			&& !oops_in_progress);
 
 	if (cpu == this_cpu) {
 		local_irq_save(flags);

[tip:core/locking] Revert "smp: Give WARN()ing if in_interrupt() when calling smp_call_function_many()/single()"

[tip-bot for Ingo Molnar]

Commit-ID:  56624143151fdb84c32a43463864e6c12a5ebcfc
Gitweb:     http://git.kernel.org/tip/56624143151fdb84c32a43463864e6c12a5ebcfc
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Mon, 11 Feb 2013 10:03:29 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 11 Feb 2013 10:03:29 +0100

Revert "smp: Give WARN()ing if in_interrupt() when calling smp_call_function_many()/single()"

This reverts commit b29f39c7c3e75a741a7da88244ec707f293ec04c.

Fengguang Wu reported that the commit triggers a bogus warning
on the current networking tree:

[  229.339945] Call Trace:
[  229.341176]  [<ffffffff810353b4>] warn_slowpath_common+0x83/0x9c
[  229.343091]  [<ffffffff817089d6>] ? flow_cache_new_hashrnd+0x98/0x98
[  229.345105]  [<ffffffff810353e7>] warn_slowpath_null+0x1a/0x1c
[  229.346978]  [<ffffffff81090793>] smp_call_function_single+0xbd/0x1c7
[  229.349017]  [<ffffffff81090afe>] smp_call_function_many+0x121/0x23e
[  229.350996]  [<ffffffff817089d6>] ? flow_cache_new_hashrnd+0x98/0x98
[  229.353005]  [<ffffffff81090e09>] smp_call_function+0x37/0x40
[  229.354860]  [<ffffffff8170907e>] flow_cache_flush+0x72/0xa0
[  229.356735]  [<ffffffff81759e33>] xfrm_dev_event+0x14/0x20
[  229.358545]  [<ffffffff817ff8b0>] notifier_call_chain+0x65/0x95
[  229.360469]  [<ffffffff8105ce16>] __raw_notifier_call_chain+0xe/0x10
[  229.362453]  [<ffffffff8105ce2c>] raw_notifier_call_chain+0x14/0x16
[  229.364453]  [<ffffffff816f63d6>] call_netdevice_notifiers+0x4a/0x4f
[  229.366434]  [<ffffffff816fa2ab>] __dev_notify_flags+0x37/0x5b
[  229.368342]  [<ffffffff816fa318>] dev_change_flags+0x49/0x54
[  229.370184]  [<ffffffff8174574a>] devinet_ioctl+0x24f/0x542
[  229.372036]  [<ffffffff81746975>] inet_ioctl+0x97/0xb1
[  229.373774]  [<ffffffff816e5042>] sock_do_ioctl.constprop.42+0x18/0x37
[  229.375791]  [<ffffffff816e5480>] sock_ioctl+0x1fd/0x20a
[  229.377648]  [<ffffffff810bb9cd>] ?  trace_buffer_lock_reserve+0x41/0x56
[  229.379701]  [<ffffffff8112b7c6>] vfs_ioctl+0x26/0x39
[  229.381459]  [<ffffffff8112c0d2>] do_vfs_ioctl+0x41b/0x45e
[  229.383269]  [<ffffffff8100bc3a>] ?  ftrace_raw_event_sys_enter+0x10b/0x11a
[  229.385404]  [<ffffffff817fc118>] ? retint_swapgs+0x13/0x1b
[  229.387227]  [<ffffffff8112c15a>] sys_ioctl+0x45/0x73
[  229.388975]  [<ffffffff818034be>] tracesys+0xd0/0xd5

The intention of the warning is to warn about IPIs generated from
hardirq contexts. But in_interrupt() will also warn from
softirq-disabled contexts.

The warning should probably use in_irq() - but that primitive
is not avaiable on non-genirq platform. So this change needs
more work - revert it until it's ready.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Cc: liu chuansheng <chuansheng.liu@intel.com>
Cc: jun.zhang@intel.com
Cc: peterz@infradead.org
Cc: jbeulich@suse.com
Cc: paulmck@linux.vnet.ibm.com
Cc: mina86@mina86.org
Cc: srivatsa.bhat@linux.vnet.ibm.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1360163901.24670.13.camel@cliu38-desktop-build
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/smp.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index a2f0b2c..69f38bd 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -12,7 +12,6 @@
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
-#include <linux/hardirq.h>
 
 #include "smpboot.h"
 
@@ -324,7 +323,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && (irqs_disabled() || in_interrupt())
+	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
 		     && !oops_in_progress);
 
 	if (cpu == this_cpu) {
@@ -422,9 +421,8 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait
-			&& (irqs_disabled() || in_interrupt())
-			&& !oops_in_progress);
+	WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
+		     && !oops_in_progress);
 
 	if (cpu == this_cpu) {
 		local_irq_save(flags);

RE: [tip:core/locking] Revert "smp: Give WARN()ing if in_interrupt() when calling smp_call_function_many()/single()"

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3504 bytes --]



> -----Original Message-----
> From: tip tree robot [mailto:tipbot@zytor.com] On Behalf Of tip-bot for Ingo
> Molnar
> Sent: Monday, February 11, 2013 8:20 PM
> To: linux-tip-commits@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; hpa@zytor.com; mingo@kernel.org;
> a.p.zijlstra@chello.nl; torvalds@linux-foundation.org;
> akpm@linux-foundation.org; tglx@linutronix.de; Wu, Fengguang; Liu,
> Chuansheng
> Subject: [tip:core/locking] Revert "smp: Give WARN()ing if in_interrupt() when
> calling smp_call_function_many()/single()"
> 
> Commit-ID:  56624143151fdb84c32a43463864e6c12a5ebcfc
> Gitweb:
> http://git.kernel.org/tip/56624143151fdb84c32a43463864e6c12a5ebcfc
> Author:     Ingo Molnar <mingo@kernel.org>
> AuthorDate: Mon, 11 Feb 2013 10:03:29 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Mon, 11 Feb 2013 10:03:29 +0100
> 
> Revert "smp: Give WARN()ing if in_interrupt() when calling
> smp_call_function_many()/single()"
> 
> This reverts commit b29f39c7c3e75a741a7da88244ec707f293ec04c.
> 
> Fengguang Wu reported that the commit triggers a bogus warning
> on the current networking tree:
> 
> [  229.339945] Call Trace:
> [  229.341176]  [<ffffffff810353b4>] warn_slowpath_common+0x83/0x9c
> [  229.343091]  [<ffffffff817089d6>] ? flow_cache_new_hashrnd+0x98/0x98
> [  229.345105]  [<ffffffff810353e7>] warn_slowpath_null+0x1a/0x1c
> [  229.346978]  [<ffffffff81090793>] smp_call_function_single+0xbd/0x1c7
> [  229.349017]  [<ffffffff81090afe>] smp_call_function_many+0x121/0x23e
> [  229.350996]  [<ffffffff817089d6>] ? flow_cache_new_hashrnd+0x98/0x98
> [  229.353005]  [<ffffffff81090e09>] smp_call_function+0x37/0x40
> [  229.354860]  [<ffffffff8170907e>] flow_cache_flush+0x72/0xa0
> [  229.356735]  [<ffffffff81759e33>] xfrm_dev_event+0x14/0x20
> [  229.358545]  [<ffffffff817ff8b0>] notifier_call_chain+0x65/0x95
> [  229.360469]  [<ffffffff8105ce16>] __raw_notifier_call_chain+0xe/0x10
> [  229.362453]  [<ffffffff8105ce2c>] raw_notifier_call_chain+0x14/0x16
> [  229.364453]  [<ffffffff816f63d6>] call_netdevice_notifiers+0x4a/0x4f
> [  229.366434]  [<ffffffff816fa2ab>] __dev_notify_flags+0x37/0x5b
> [  229.368342]  [<ffffffff816fa318>] dev_change_flags+0x49/0x54
> [  229.370184]  [<ffffffff8174574a>] devinet_ioctl+0x24f/0x542
> [  229.372036]  [<ffffffff81746975>] inet_ioctl+0x97/0xb1
> [  229.373774]  [<ffffffff816e5042>] sock_do_ioctl.constprop.42+0x18/0x37
> [  229.375791]  [<ffffffff816e5480>] sock_ioctl+0x1fd/0x20a
> [  229.377648]  [<ffffffff810bb9cd>] ?
> trace_buffer_lock_reserve+0x41/0x56
> [  229.379701]  [<ffffffff8112b7c6>] vfs_ioctl+0x26/0x39
> [  229.381459]  [<ffffffff8112c0d2>] do_vfs_ioctl+0x41b/0x45e
> [  229.383269]  [<ffffffff8100bc3a>] ?
> ftrace_raw_event_sys_enter+0x10b/0x11a
> [  229.385404]  [<ffffffff817fc118>] ? retint_swapgs+0x13/0x1b
> [  229.387227]  [<ffffffff8112c15a>] sys_ioctl+0x45/0x73
> [  229.388975]  [<ffffffff818034be>] tracesys+0xd0/0xd5
> 
> The intention of the warning is to warn about IPIs generated from
> hardirq contexts. But in_interrupt() will also warn from
> softirq-disabled contexts.
Thanks Ingo and Fengguang's pointing out.
I have written a new patch https://lkml.org/lkml/2013/2/16/1 which use the below macro to know if
nmi/hard/soft irq is handling.
+#define in_serving_irq() (in_nmi() || in_irq() || in_serving_softirq())
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Chuansheng Liu]

Currently the functions smp_call_function_many()/single() will
give a WARN()ing only in the case of irqs_disabled(), but that
check is not enough to guarantee execution of the SMP
cross-calls.

In many other cases such as softirq handling/interrupt handling,
the two APIs still can not be called, just as the
smp_call_function_many() comments say:

  * You must not call this function with disabled interrupts or from a
  * hardware interrupt handler or from a bottom half handler. Preemption
  * must be disabled when calling this function.

There is a real case for softirq DEADLOCK case:

CPUA                            CPUB
                                spin_lock(&spinlock)
                                Any irq coming, call the irq handler
                                irq_exit()
spin_lock_irq(&spinlock)
<== Blocking here due to
CPUB hold it
                                  __do_softirq()
                                    run_timer_softirq()
                                      timer_cb()
                                        call smp_call_function_many()
                                          send IPI interrupt to CPUA
                                            wait_csd()

Then both CPUA and CPUB will be deadlocked here.

So we should give a warning in the nmi, hardirq or softirq context as well.

Moreover, adding one new macro in_serving_irq() which indicates
we are processing nmi, hardirq or sofirq.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 include/linux/hardirq.h |    5 +++++
 kernel/smp.c            |   10 ++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index 624ef3f..e07663f 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -94,6 +94,11 @@
  */
 #define in_nmi()	(preempt_count() & NMI_MASK)
 
+/*
+ * Are we in nmi,irq context, or softirq context?
+ */
+#define in_serving_irq() (in_nmi() || in_irq() || in_serving_softirq())
+
 #if defined(CONFIG_PREEMPT_COUNT)
 # define PREEMPT_CHECK_OFFSET 1
 #else
diff --git a/kernel/smp.c b/kernel/smp.c
index 69f38bd..ba43dd7 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -323,8 +323,9 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
-		     && !oops_in_progress);
+	WARN_ON_ONCE(cpu_online(this_cpu)
+		&& (irqs_disabled() || in_serving_irq())
+		&& !oops_in_progress);
 
 	if (cpu == this_cpu) {
 		local_irq_save(flags);
@@ -462,8 +463,9 @@ void smp_call_function_many(const struct cpumask *mask,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
-		     && !oops_in_progress && !early_boot_irqs_disabled);
+	WARN_ON_ONCE(cpu_online(this_cpu)
+		&& (irqs_disabled() || in_serving_irq())
+		&& !oops_in_progress && !early_boot_irqs_disabled);
 
 	/* Try to fastpath.  So, what's a CPU they want? Ignoring this one. */
 	cpu = cpumask_first_and(mask, cpu_online_mask);
-- 
1.7.0.4

[PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Chuansheng Liu]

Currently the functions smp_call_function_many()/single() will
give a WARN()ing only in the case of irqs_disabled(), but that
check is not enough to guarantee execution of the SMP
cross-calls.

In many other cases such as softirq handling/interrupt handling,
the two APIs still can not be called, just as the
smp_call_function_many() comments say:

  * You must not call this function with disabled interrupts or from a
  * hardware interrupt handler or from a bottom half handler. Preemption
  * must be disabled when calling this function.

There is a real case for softirq DEADLOCK case:

CPUA                            CPUB
                                spin_lock(&spinlock)
                                Any irq coming, call the irq handler
                                irq_exit()
spin_lock_irq(&spinlock)
<== Blocking here due to
CPUB hold it
                                  __do_softirq()
                                    run_timer_softirq()
                                      timer_cb()
                                        call smp_call_function_many()
                                          send IPI interrupt to CPUA
                                            wait_csd()

Then both CPUA and CPUB will be deadlocked here.

So we should give a warning in the nmi, hardirq or softirq context as well.

Moreover, adding one new macro in_serving_irq() which indicates
we are processing nmi, hardirq or sofirq.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 include/linux/hardirq.h |    5 +++++
 kernel/smp.c            |   11 +++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index 624ef3f..e07663f 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -94,6 +94,11 @@
  */
 #define in_nmi()	(preempt_count() & NMI_MASK)
 
+/*
+ * Are we in nmi,irq context, or softirq context?
+ */
+#define in_serving_irq() (in_nmi() || in_irq() || in_serving_softirq())
+
 #if defined(CONFIG_PREEMPT_COUNT)
 # define PREEMPT_CHECK_OFFSET 1
 #else
diff --git a/kernel/smp.c b/kernel/smp.c
index 69f38bd..b0a5d21 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -12,6 +12,7 @@
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
+#include <linux/hardirq.h>
 
 #include "smpboot.h"
 
@@ -323,8 +324,9 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
-		     && !oops_in_progress);
+	WARN_ON_ONCE(cpu_online(this_cpu)
+		&& (irqs_disabled() || in_serving_irq())
+		&& !oops_in_progress);
 
 	if (cpu == this_cpu) {
 		local_irq_save(flags);
@@ -462,8 +464,9 @@ void smp_call_function_many(const struct cpumask *mask,
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
-		     && !oops_in_progress && !early_boot_irqs_disabled);
+	WARN_ON_ONCE(cpu_online(this_cpu)
+		&& (irqs_disabled() || in_serving_irq())
+		&& !oops_in_progress && !early_boot_irqs_disabled);
 
 	/* Try to fastpath.  So, what's a CPU they want? Ignoring this one. */
 	cpu = cpumask_first_and(mask, cpu_online_mask);
-- 
1.7.0.4

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Fengguang Wu]

Chuansheng,

It works fine on tip/next. Thanks for the fix!

Tested-by: Fengguang Wu <fengguang.wu@intel.com>

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Andrew Morton]

On Mon, 18 Feb 2013 09:38:52 +0800
Fengguang Wu <fengguang.wu@intel.com> wrote:

> Chuansheng,
> 
> It works fine on tip/next. Thanks for the fix!
> 
> Tested-by: Fengguang Wu <fengguang.wu@intel.com>

How did you test this?  What did you do?

afacit the patch adds additional warnings, so you must have had some
buggy code and with the patch, runtime warnings are generated which
inform you of that bug?

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Fengguang Wu]

On Tue, Feb 19, 2013 at 03:01:23PM -0800, Andrew Morton wrote:
> On Mon, 18 Feb 2013 09:38:52 +0800
> Fengguang Wu <fengguang.wu@intel.com> wrote:
> 
> > Chuansheng,
> > 
> > It works fine on tip/next. Thanks for the fix!
> > 
> > Tested-by: Fengguang Wu <fengguang.wu@intel.com>
> 
> How did you test this?  What did you do?

I used the same kconfig to build a kernel from tip/next HEAD *plus*
the v2 patch. The result is, no smp_call_function_single/many warnings
are observed.

> afacit the patch adds additional warnings, so you must have had some
> buggy code and with the patch, runtime warnings are generated which
> inform you of that bug?

Andrew, the background is: Ingo has *reverted* the original patch that
triggered the smp_call_function_single warning from his tip tree, so
the test result means this patch's warning messages won't trigger in
my test boots.

Thanks,
Fengguang

RE: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Liu, Chuansheng]

> -----Original Message-----
> From: Wu, Fengguang
> Sent: Wednesday, February 20, 2013 9:07 AM
> To: Andrew Morton
> Cc: Liu, Chuansheng; mingo@kernel.org; peterz@infradead.org;
> jbeulich@suse.com; paulmck@linux.vnet.ibm.com; mina86@mina86.org;
> srivatsa.bhat@linux.vnet.ibm.com; linux-kernel@vger.kernel.org; Zhang, Jun
> Subject: Re: [PATCH V2] smp: Give WARN()ing when calling
> smp_call_function_many()/single() in serving irq
> 
> On Tue, Feb 19, 2013 at 03:01:23PM -0800, Andrew Morton wrote:
> > On Mon, 18 Feb 2013 09:38:52 +0800
> > Fengguang Wu <fengguang.wu@intel.com> wrote:
> >
> > > Chuansheng,
> > >
> > > It works fine on tip/next. Thanks for the fix!
> > >
> > > Tested-by: Fengguang Wu <fengguang.wu@intel.com>
> >
> > How did you test this?  What did you do?
> 
> I used the same kconfig to build a kernel from tip/next HEAD *plus*
> the v2 patch. The result is, no smp_call_function_single/many warnings
> are observed.
> 
> > afacit the patch adds additional warnings, so you must have had some
> > buggy code and with the patch, runtime warnings are generated which
> > inform you of that bug?
> 
> Andrew, the background is: Ingo has *reverted* the original patch that
> triggered the smp_call_function_single warning from his tip tree, so
> the test result means this patch's warning messages won't trigger in
> my test boots.
I also locally tested the below several cases, thanks.
1/ local_bh_disable(), then called smp_call_function_single(), no WARNING;
2/ In timer callback, then called smp_call_function_single(), WARNING;

> 
> Thanks,
> Fengguang

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Lai Jiangshan]

On Sat, Feb 16, 2013 at 10:10 PM, Chuansheng Liu
<chuansheng.liu@intel.com> wrote:
> Currently the functions smp_call_function_many()/single() will
> give a WARN()ing only in the case of irqs_disabled(), but that
> check is not enough to guarantee execution of the SMP
> cross-calls.
>
> In many other cases such as softirq handling/interrupt handling,
> the two APIs still can not be called, just as the
> smp_call_function_many() comments say:
>
>   * You must not call this function with disabled interrupts or from a
>   * hardware interrupt handler or from a bottom half handler. Preemption
>   * must be disabled when calling this function.
>
> There is a real case for softirq DEADLOCK case:
>
> CPUA                            CPUB
>                                 spin_lock(&spinlock)
>                                 Any irq coming, call the irq handler
>                                 irq_exit()
> spin_lock_irq(&spinlock)
> <== Blocking here due to
> CPUB hold it
>                                   __do_softirq()
>                                     run_timer_softirq()
>                                       timer_cb()
>                                         call smp_call_function_many()
>                                           send IPI interrupt to CPUA
>                                             wait_csd()
>
> Then both CPUA and CPUB will be deadlocked here.
>
> So we should give a warning in the nmi, hardirq or softirq context as well.
>
> Moreover, adding one new macro in_serving_irq() which indicates
> we are processing nmi, hardirq or sofirq.

The code smells bad. in_serving_softirq() don't take spin_lock_bh() in account.

CPUA                    CPUB                             CPUC
                        spin_lock(&lockA)
                          Any irq coming, call
                          the irq handler
                          irq_exit()
spin_lock_irq(&lockA)
*Blocking* here
due to CPUB hold it                                      spin_lock_bh(&lockB)
                            __do_softirq()
                              run_timer_softirq()
                                spin_lock_bh(&lockB)
                                *Blocking* heredue to
                                CPUC hold it
                                                         call
smp_call_function_many()
                                                         send IPI
interrupt to CPUA
                                                           wait_csd()
                                                           *Blocking* here.

So it is still deadlock. but your code does not warn it.
so in_softirq() is better than in_serving_softirq() in in_serving_irq(),
and results in_serving_irq() is the same as in_interrupt().

so please remove in_serving_irq() and use in_interrupt() instead.
And add:

Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com>

In the long-term, the best solution is using percpu lockdep for
local_irq_disable()
and smp_call_function_many():

CPUA                    CPUB
                        spin_lock(&lockA)
spin_lock_irq(&lockA)
*Blocking* here
due to CPUB hold it
                        call smp_call_function_many()
                          send IPI interrupt to CPUA
                            wait_csd()
                            *Blocking* here.

I will do it in the next week after the next week.

Thanks,
Lai


>
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
>  include/linux/hardirq.h |    5 +++++
>  kernel/smp.c            |   11 +++++++----
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index 624ef3f..e07663f 100644
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -94,6 +94,11 @@
>   */
>  #define in_nmi()       (preempt_count() & NMI_MASK)
>
> +/*
> + * Are we in nmi,irq context, or softirq context?
> + */
> +#define in_serving_irq() (in_nmi() || in_irq() || in_serving_softirq())
> +
>  #if defined(CONFIG_PREEMPT_COUNT)
>  # define PREEMPT_CHECK_OFFSET 1
>  #else
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 69f38bd..b0a5d21 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -12,6 +12,7 @@
>  #include <linux/gfp.h>
>  #include <linux/smp.h>
>  #include <linux/cpu.h>
> +#include <linux/hardirq.h>
>
>  #include "smpboot.h"
>
> @@ -323,8 +324,9 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
>          * send smp call function interrupt to this cpu and as such deadlocks
>          * can't happen.
>          */
> -       WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
> -                    && !oops_in_progress);
> +       WARN_ON_ONCE(cpu_online(this_cpu)
> +               && (irqs_disabled() || in_serving_irq())
> +               && !oops_in_progress);
>
>         if (cpu == this_cpu) {
>                 local_irq_save(flags);
> @@ -462,8 +464,9 @@ void smp_call_function_many(const struct cpumask *mask,
>          * send smp call function interrupt to this cpu and as such deadlocks
>          * can't happen.
>          */
> -       WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
> -                    && !oops_in_progress && !early_boot_irqs_disabled);
> +       WARN_ON_ONCE(cpu_online(this_cpu)
> +               && (irqs_disabled() || in_serving_irq())
> +               && !oops_in_progress && !early_boot_irqs_disabled);
>
>         /* Try to fastpath.  So, what's a CPU they want? Ignoring this one. */
>         cpu = cpumask_first_and(mask, cpu_online_mask);
> --
> 1.7.0.4
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Liu, Chuansheng]

> -----Original Message-----
> From: Lai Jiangshan [mailto:eag0628@gmail.com]
> Sent: Wednesday, February 27, 2013 10:51 PM
> To: Liu, Chuansheng
> Cc: mingo@kernel.org; peterz@infradead.org; jbeulich@suse.com;
> paulmck@linux.vnet.ibm.com; akpm@linux-foundation.org;
> mina86@mina86.org; srivatsa.bhat@linux.vnet.ibm.com;
> linux-kernel@vger.kernel.org; Zhang, Jun; Wu, Fengguang
> Subject: Re: [PATCH V2] smp: Give WARN()ing when calling
> smp_call_function_many()/single() in serving irq
> 
> On Sat, Feb 16, 2013 at 10:10 PM, Chuansheng Liu
> <chuansheng.liu@intel.com> wrote:
> > Currently the functions smp_call_function_many()/single() will
> > give a WARN()ing only in the case of irqs_disabled(), but that
> > check is not enough to guarantee execution of the SMP
> > cross-calls.
> >
> > In many other cases such as softirq handling/interrupt handling,
> > the two APIs still can not be called, just as the
> > smp_call_function_many() comments say:
> >
> >   * You must not call this function with disabled interrupts or from a
> >   * hardware interrupt handler or from a bottom half handler. Preemption
> >   * must be disabled when calling this function.
> >
> > There is a real case for softirq DEADLOCK case:
> >
> > CPUA                            CPUB
> >                                 spin_lock(&spinlock)
> >                                 Any irq coming, call the irq handler
> >                                 irq_exit()
> > spin_lock_irq(&spinlock)
> > <== Blocking here due to
> > CPUB hold it
> >                                   __do_softirq()
> >                                     run_timer_softirq()
> >                                       timer_cb()
> >                                         call
> smp_call_function_many()
> >                                           send IPI interrupt to
> CPUA
> >                                             wait_csd()
> >
> > Then both CPUA and CPUB will be deadlocked here.
> >
> > So we should give a warning in the nmi, hardirq or softirq context as well.
> >
> > Moreover, adding one new macro in_serving_irq() which indicates
> > we are processing nmi, hardirq or sofirq.
> 
> The code smells bad. in_serving_softirq() don't take spin_lock_bh() in account.
> 
> CPUA                    CPUB                             CPUC
>                         spin_lock(&lockA)
>                           Any irq coming, call
>                           the irq handler
>                           irq_exit()
> spin_lock_irq(&lockA)
> *Blocking* here
> due to CPUB hold it
> spin_lock_bh(&lockB)
>                             __do_softirq()
>                               run_timer_softirq()
>                                 spin_lock_bh(&lockB)
>                                 *Blocking* heredue to
>                                 CPUC hold it
>                                                          call
> smp_call_function_many()
>                                                          send IPI
> interrupt to CPUA
> 
> wait_csd()
> 
> *Blocking* here.
> 
> So it is still deadlock. but your code does not warn it.
In your case, even you change spin_lock_bh() to spin_lock(), the deadlock is still there. So no relation with _bh() at all,
Do not need warning for such deadlock case in smp_call_xxx() or for _bh() case.

> so in_softirq() is better than in_serving_softirq() in in_serving_irq(),
> and results in_serving_irq() is the same as in_interrupt().
> 
> so please remove in_serving_irq() and use in_interrupt() instead.
The original patch is using in_interrupt(). https://lkml.org/lkml/2013/2/6/34 

> And add:
> 
> Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com>

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Andrew Morton]

On Fri, 1 Mar 2013 03:37:11 +0000 "Liu, Chuansheng" <chuansheng.liu@intel.com> wrote:

> 
> >                                 spin_lock_bh(&lockB)
> >                                 *Blocking* heredue to
> >                                 CPUC hold it
> >                                                          call
> > smp_call_function_many()
> >                                                          send IPI
> > interrupt to CPUA
> > 
> > wait_csd()
> > 
> > *Blocking* here.
> > 
> > So it is still deadlock. but your code does not warn it.
> In your case, even you change spin_lock_bh() to spin_lock(), the deadlock is still there. So no relation with _bh() at all,
> Do not need warning for such deadlock case in smp_call_xxx() or for _bh() case.
> 
> > so in_softirq() is better than in_serving_softirq() in in_serving_irq(),
> > and results in_serving_irq() is the same as in_interrupt().
> > 
> > so please remove in_serving_irq() and use in_interrupt() instead.
> The original patch is using in_interrupt(). https://lkml.org/lkml/2013/2/6/34 
> 

(ancient thread)

It's not clear (to me) that all these issues are settled.  Can we
please take another look at this?

The patch has been in -mm and linux-next for five months with no
issues.  But as far as I know, it hasn't detected any kernel bugs, so
perhaps we just don't need it?


From: Chuansheng Liu <chuansheng.liu@intel.com>
Subject: smp: give WARN()ing when calling smp_call_function_many()/single() in serving irq

Currently the functions smp_call_function_many()/single() will give a
WARN()ing only in the case of irqs_disabled(), but that check is not
enough to guarantee execution of the SMP cross-calls.

In many other cases such as softirq handling/interrupt handling, the two
APIs still can not be called, just as the smp_call_function_many()
comments say:

  * You must not call this function with disabled interrupts or from a
  * hardware interrupt handler or from a bottom half handler. Preemption
  * must be disabled when calling this function.

There is a real case for softirq DEADLOCK case:

CPUA                            CPUB
                                spin_lock(&spinlock)
                                Any irq coming, call the irq handler
                                irq_exit()
spin_lock_irq(&spinlock)
<== Blocking here due to
CPUB hold it
                                  __do_softirq()
                                    run_timer_softirq()
                                      timer_cb()
                                        call smp_call_function_many()
                                          send IPI interrupt to CPUA
                                            wait_csd()

Then both CPUA and CPUB will be deadlocked here.

So we should give a warning in the nmi, hardirq or softirq context as well.

Moreover, adding one new macro in_serving_irq() which indicates we are
processing nmi, hardirq or sofirq.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Tested-by: Fengguang Wu <fengguang.wu@intel.com>
Cc: Lai Jiangshan <eag0628@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/hardirq.h |    5 +++++
 kernel/smp.c            |   11 +++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff -puN include/linux/hardirq.h~smp-give-warning-when-calling-smp_call_function_many-single-in-serving-irq include/linux/hardirq.h
--- a/include/linux/hardirq.h~smp-give-warning-when-calling-smp_call_function_many-single-in-serving-irq
+++ a/include/linux/hardirq.h
@@ -94,6 +94,11 @@
  */
 #define in_nmi()	(preempt_count() & NMI_MASK)
 
+/*
+ * Are we in nmi,irq context, or softirq context?
+ */
+#define in_serving_irq() (in_nmi() || in_irq() || in_serving_softirq())
+
 #if defined(CONFIG_PREEMPT_COUNT)
 # define PREEMPT_CHECK_OFFSET 1
 #else
diff -puN kernel/smp.c~smp-give-warning-when-calling-smp_call_function_many-single-in-serving-irq kernel/smp.c
--- a/kernel/smp.c~smp-give-warning-when-calling-smp_call_function_many-single-in-serving-irq
+++ a/kernel/smp.c
@@ -12,6 +12,7 @@
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
+#include <linux/hardirq.h>
 
 #include "smpboot.h"
 
@@ -243,8 +244,9 @@ int smp_call_function_single(int cpu, sm
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
-		     && !oops_in_progress);
+	WARN_ON_ONCE(cpu_online(this_cpu)
+		&& (irqs_disabled() || in_serving_irq())
+		&& !oops_in_progress);
 
 	if (cpu == this_cpu) {
 		local_irq_save(flags);
@@ -381,8 +383,9 @@ void smp_call_function_many(const struct
 	 * send smp call function interrupt to this cpu and as such deadlocks
 	 * can't happen.
 	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
-		     && !oops_in_progress && !early_boot_irqs_disabled);
+	WARN_ON_ONCE(cpu_online(this_cpu)
+		&& (irqs_disabled() || in_serving_irq())
+		&& !oops_in_progress && !early_boot_irqs_disabled);
 
 	/* Try to fastpath.  So, what's a CPU they want? Ignoring this one. */
 	cpu = cpumask_first_and(mask, cpu_online_mask);
_

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Thomas Gleixner]

On Sat, 16 Feb 2013, Chuansheng Liu wrote:
> Currently the functions smp_call_function_many()/single() will
> give a WARN()ing only in the case of irqs_disabled(), but that
> check is not enough to guarantee execution of the SMP
> cross-calls.
> 
> In many other cases such as softirq handling/interrupt handling,
> the two APIs still can not be called, just as the
> smp_call_function_many() comments say:
> 
>   * You must not call this function with disabled interrupts or from a
>   * hardware interrupt handler or from a bottom half handler. Preemption
>   * must be disabled when calling this function.
> 
> There is a real case for softirq DEADLOCK case:
> 
> CPUA                            CPUB
>                                 spin_lock(&spinlock)
>                                 Any irq coming, call the irq handler
>                                 irq_exit()
> spin_lock_irq(&spinlock)
> <== Blocking here due to
> CPUB hold it
>                                   __do_softirq()
>                                     run_timer_softirq()
>                                       timer_cb()
>                                         call smp_call_function_many()
>                                           send IPI interrupt to CPUA
>                                             wait_csd()
> 
> Then both CPUA and CPUB will be deadlocked here.

That's not true if called with wait = 0 as we won't wait for the csd
in that case. The function will be invoked on cpuA after it reenables
interrupt. So for callers who don't care about synchronous execution
it should not warn in softirq context.

Thanks,

	tglx

 

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Thomas Gleixner]

On Fri, 5 Jul 2013, Thomas Gleixner wrote:
> On Sat, 16 Feb 2013, Chuansheng Liu wrote:
> > Currently the functions smp_call_function_many()/single() will
> > give a WARN()ing only in the case of irqs_disabled(), but that
> > check is not enough to guarantee execution of the SMP
> > cross-calls.
> > 
> > In many other cases such as softirq handling/interrupt handling,
> > the two APIs still can not be called, just as the
> > smp_call_function_many() comments say:
> > 
> >   * You must not call this function with disabled interrupts or from a
> >   * hardware interrupt handler or from a bottom half handler. Preemption
> >   * must be disabled when calling this function.
> > 
> > There is a real case for softirq DEADLOCK case:
> > 
> > CPUA                            CPUB
> >                                 spin_lock(&spinlock)
> >                                 Any irq coming, call the irq handler
> >                                 irq_exit()
> > spin_lock_irq(&spinlock)
> > <== Blocking here due to
> > CPUB hold it
> >                                   __do_softirq()
> >                                     run_timer_softirq()
> >                                       timer_cb()
> >                                         call smp_call_function_many()
> >                                           send IPI interrupt to CPUA
> >                                             wait_csd()
> > 
> > Then both CPUA and CPUB will be deadlocked here.
> 
> That's not true if called with wait = 0 as we won't wait for the csd
> in that case. The function will be invoked on cpuA after it reenables
> interrupt. So for callers who don't care about synchronous execution
> it should not warn in softirq context.

Hmm, even there it matters, because of the following scenario:

CPU 0
smp_call_function_single(CPU 1)
    csd_lock(CPU 1)
    irq_enter()
    irq_exit()
    __do_softirq()
    smp_call_function_many()
      setup csd (CPU 1)
        csd_lock(CPU 1) ==> CPU 0 deadlocked itself.

And this is even more likely to happen than the lock issue.

Thanks,

	tglx

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Wang YanQing]

On Fri, Jul 05, 2013 at 04:37:14PM +0200, Thomas Gleixner wrote:
> Hmm, even there it matters, because of the following scenario:
> 
> CPU 0
> smp_call_function_single(CPU 1)
>     csd_lock(CPU 1)

No, smpl_call_function_single(CPU 1)
will csd_lock(CPU 0), not csd_lock(CPU 1)

>     irq_enter()
>     irq_exit()
>     __do_softirq()
>     smp_call_function_many()
>       setup csd (CPU 1)
>         csd_lock(CPU 1) ==> CPU 0 deadlocked itself.
> 

maybe below is the scenario:
     irq_enter()
     irq_exit()
     __do_softirq()
     smp_call_function_single()
       setup csd (CPU 1)
         csd_lock(CPU 0) ==> CPU 0 deadlocked itself.
 

> And this is even more likely to happen than the lock issue.
> 
> Thanks,
> 
> 	tglx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Thomas Gleixner]

On Sun, 7 Jul 2013, Wang YanQing wrote:
> On Fri, Jul 05, 2013 at 04:37:14PM +0200, Thomas Gleixner wrote:
> > Hmm, even there it matters, because of the following scenario:
> > 
> > CPU 0
> > smp_call_function_single(CPU 1)
> >     csd_lock(CPU 1)
> 
> No, smpl_call_function_single(CPU 1)
> will csd_lock(CPU 0), not csd_lock(CPU 1)
> 
> >     irq_enter()
> >     irq_exit()
> >     __do_softirq()
> >     smp_call_function_many()
> >       setup csd (CPU 1)
> >         csd_lock(CPU 1) ==> CPU 0 deadlocked itself.
> > 
> 
> maybe below is the scenario:
>      irq_enter()
>      irq_exit()
>      __do_softirq()
>      smp_call_function_single()
>        setup csd (CPU 1)
>          csd_lock(CPU 0) ==> CPU 0 deadlocked itself.

Right, I fatfingered that :)

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Max Filippov]

Hi Thomas,

On Fri, Jul 5, 2013 at 6:37 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 5 Jul 2013, Thomas Gleixner wrote:
>> On Sat, 16 Feb 2013, Chuansheng Liu wrote:
>> > Currently the functions smp_call_function_many()/single() will
>> > give a WARN()ing only in the case of irqs_disabled(), but that
>> > check is not enough to guarantee execution of the SMP
>> > cross-calls.
>> >
>> > In many other cases such as softirq handling/interrupt handling,
>> > the two APIs still can not be called, just as the
>> > smp_call_function_many() comments say:
>> >
>> >   * You must not call this function with disabled interrupts or from a
>> >   * hardware interrupt handler or from a bottom half handler. Preemption
>> >   * must be disabled when calling this function.
>> >
>> > There is a real case for softirq DEADLOCK case:
>> >
>> > CPUA                            CPUB
>> >                                 spin_lock(&spinlock)
>> >                                 Any irq coming, call the irq handler
>> >                                 irq_exit()
>> > spin_lock_irq(&spinlock)
>> > <== Blocking here due to
>> > CPUB hold it
>> >                                   __do_softirq()
>> >                                     run_timer_softirq()
>> >                                       timer_cb()
>> >                                         call smp_call_function_many()
>> >                                           send IPI interrupt to CPUA
>> >                                             wait_csd()
>> >
>> > Then both CPUA and CPUB will be deadlocked here.
>>
>> That's not true if called with wait = 0 as we won't wait for the csd
>> in that case. The function will be invoked on cpuA after it reenables
>> interrupt. So for callers who don't care about synchronous execution
>> it should not warn in softirq context.
>
> Hmm, even there it matters, because of the following scenario:
>
> CPU 0
> smp_call_function_single(CPU 1)
>     csd_lock(CPU 1)
>     irq_enter()
>     irq_exit()
>     __do_softirq()
>     smp_call_function_many()
>       setup csd (CPU 1)
>         csd_lock(CPU 1) ==> CPU 0 deadlocked itself.
>
> And this is even more likely to happen than the lock issue.

I've observed similar deadlock in a real system which has network
driver that uses smp_call_function_single in the softirq context.

The proposed fix below keeps IRQs disabled on the sending CPU
during the period between marking csd locked and sending IPI,
making it possible to use smp_call_function_single from the softirq
context. What do you think?

--->8---
>From 5fa496ce12eaf994debab202cde618b9da7d9402 Mon Sep 17 00:00:00 2001
From: Max Filippov <jcmvbkbc@gmail.com>
Date: Fri, 6 Dec 2013 04:50:03 +0400
Subject: [PATCH] smp: allow calling smp_call_function_single from softirq

This prevents the following deadlocks on the sending CPU by eliminating
interrupts between the point where CSD is locked and IPI is sent to peer
CPU.

Case 1:
 CPU 0
 smp_call_function_single(CPU 1, wait = 0)
     csd_lock(CPU 0)
     irq_enter()
     irq_exit()
     __do_softirq()
     smp_call_function_single(CPU 1, wait = 0)
       csd_lock(CPU 0) => deadlock, as csd will never be unlocked

Case 2:
 CPU 0
 smp_call_function_single(CPU 1, wait = 1)
     csd_lock(on stack)
     queue csd to CPU 1 call_single_queue
     irq_enter()
     irq_exit()
     __do_softirq()
     smp_call_function_single(CPU 1, wait = 1)
       setup csd (on stack)
       queue csd to CPU 1 call_single_queue
       csd_lock_wait => never returns, as IPI was never sent to CPU 1

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 kernel/smp.c | 47 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 0564571..7bc9a01 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -122,6 +122,30 @@ static void csd_lock(struct call_single_data *csd)
 	smp_mb();
 }

+static unsigned long csd_lock_irqsave(struct call_single_data *csd)
+{
+	unsigned long flags;
+
+	for (;;) {
+		csd_lock_wait(csd);
+		local_irq_save(flags);
+		if (csd->flags & CSD_FLAG_LOCK)
+			local_irq_restore(flags);
+		else
+			break;
+	}
+	csd->flags |= CSD_FLAG_LOCK;
+
+	/*
+	 * prevent CPU from reordering the above assignment
+	 * to ->flags with any subsequent assignments to other
+	 * fields of the specified call_single_data structure:
+	 */
+	smp_mb();
+
+	return flags;
+}
+
 static void csd_unlock(struct call_single_data *csd)
 {
 	WARN_ON(!(csd->flags & CSD_FLAG_LOCK));
@@ -140,16 +164,20 @@ static void csd_unlock(struct call_single_data *csd)
  * ->func, ->info, and ->flags set.
  */
 static
-void generic_exec_single(int cpu, struct call_single_data *csd, int wait)
+void generic_exec_single(int cpu, struct call_single_data *csd,
+			 smp_call_func_t func, void *info, int wait)
 {
 	struct call_single_queue *dst = &per_cpu(call_single_queue, cpu);
-	unsigned long flags;
+	unsigned long flags = csd_lock_irqsave(csd);
 	int ipi;

-	raw_spin_lock_irqsave(&dst->lock, flags);
+	csd->func = func;
+	csd->info = info;
+
+	raw_spin_lock(&dst->lock);
 	ipi = list_empty(&dst->list);
 	list_add_tail(&csd->list, &dst->list);
-	raw_spin_unlock_irqrestore(&dst->lock, flags);
+	raw_spin_unlock(&dst->lock);

 	/*
 	 * The list addition should be visible before sending the IPI
@@ -165,6 +193,8 @@ void generic_exec_single(int cpu, struct call_single_data *csd, int wait)
 	if (ipi)
 		arch_send_call_function_single_ipi(cpu);

+	local_irq_restore(flags);
+
 	if (wait)
 		csd_lock_wait(csd);
 }
@@ -245,11 +275,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
 			if (!wait)
 				csd = &__get_cpu_var(csd_data);

-			csd_lock(csd);
-
-			csd->func = func;
-			csd->info = info;
-			generic_exec_single(cpu, csd, wait);
+			generic_exec_single(cpu, csd, func, info, wait);
 		} else {
 			err = -ENXIO;	/* CPU not online */
 		}
@@ -335,8 +361,7 @@ void __smp_call_function_single(int cpu, struct call_single_data *csd,
 		csd->func(csd->info);
 		local_irq_restore(flags);
 	} else {
-		csd_lock(csd);
-		generic_exec_single(cpu, csd, wait);
+		generic_exec_single(cpu, csd, csd->func, csd->info, wait);
 	}
 	put_cpu();
 }
-- 
1.8.1.4


-- 
Thanks.
-- Max

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Thomas Gleixner]

On Fri, 6 Dec 2013, Max Filippov wrote:
> Hi Thomas,
 
> On Fri, Jul 5, 2013 at 6:37 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Fri, 5 Jul 2013, Thomas Gleixner wrote:
> >> On Sat, 16 Feb 2013, Chuansheng Liu wrote:
> >> > Currently the functions smp_call_function_many()/single() will
> >> > give a WARN()ing only in the case of irqs_disabled(), but that
> >> > check is not enough to guarantee execution of the SMP
> >> > cross-calls.
> >> >
> >> > In many other cases such as softirq handling/interrupt handling,
> >> > the two APIs still can not be called, just as the
> >> > smp_call_function_many() comments say:
> >> >
> >> >   * You must not call this function with disabled interrupts or from a
> >> >   * hardware interrupt handler or from a bottom half handler. Preemption
> >> >   * must be disabled when calling this function.
> >> >
> >> > There is a real case for softirq DEADLOCK case:
> >> >
> >> > CPUA                            CPUB
> >> >                                 spin_lock(&spinlock)
> >> >                                 Any irq coming, call the irq handler
> >> >                                 irq_exit()
> >> > spin_lock_irq(&spinlock)
> >> > <== Blocking here due to
> >> > CPUB hold it
> >> >                                   __do_softirq()
> >> >                                     run_timer_softirq()
> >> >                                       timer_cb()
> >> >                                         call smp_call_function_many()
> >> >                                           send IPI interrupt to CPUA
> >> >                                             wait_csd()
> >> >
> >> > Then both CPUA and CPUB will be deadlocked here.
> >>
> >> That's not true if called with wait = 0 as we won't wait for the csd
> >> in that case. The function will be invoked on cpuA after it reenables
> >> interrupt. So for callers who don't care about synchronous execution
> >> it should not warn in softirq context.
> >
> > Hmm, even there it matters, because of the following scenario:
> >
> > CPU 0
> > smp_call_function_single(CPU 1)
> >     csd_lock(CPU 1)
> >     irq_enter()
> >     irq_exit()
> >     __do_softirq()
> >     smp_call_function_many()
> >       setup csd (CPU 1)
> >         csd_lock(CPU 1) ==> CPU 0 deadlocked itself.
> >
> > And this is even more likely to happen than the lock issue.
> 
> I've observed similar deadlock in a real system which has network
> driver that uses smp_call_function_single in the softirq context.
> 
> The proposed fix below keeps IRQs disabled on the sending CPU
> during the period between marking csd locked and sending IPI,
> making it possible to use smp_call_function_single from the softirq
> context. What do you think?

I'm not really exited to encourage IPIs from irq context. Just because
some network driver uses it, is definitely not a good argument. If we
really want to support that, then we need a proper justification why
it is necessary in the first place.

Thanks,

	tglx

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Max Filippov]

On Fri, Dec 6, 2013 at 6:02 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 6 Dec 2013, Max Filippov wrote:
>> Hi Thomas,
>
>> On Fri, Jul 5, 2013 at 6:37 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Fri, 5 Jul 2013, Thomas Gleixner wrote:
>> >> On Sat, 16 Feb 2013, Chuansheng Liu wrote:
>> >> > Currently the functions smp_call_function_many()/single() will
>> >> > give a WARN()ing only in the case of irqs_disabled(), but that
>> >> > check is not enough to guarantee execution of the SMP
>> >> > cross-calls.
>> >> >
>> >> > In many other cases such as softirq handling/interrupt handling,
>> >> > the two APIs still can not be called, just as the
>> >> > smp_call_function_many() comments say:
>> >> >
>> >> >   * You must not call this function with disabled interrupts or from a
>> >> >   * hardware interrupt handler or from a bottom half handler. Preemption
>> >> >   * must be disabled when calling this function.
>> >> >
>> >> > There is a real case for softirq DEADLOCK case:
>> >> >
>> >> > CPUA                            CPUB
>> >> >                                 spin_lock(&spinlock)
>> >> >                                 Any irq coming, call the irq handler
>> >> >                                 irq_exit()
>> >> > spin_lock_irq(&spinlock)
>> >> > <== Blocking here due to
>> >> > CPUB hold it
>> >> >                                   __do_softirq()
>> >> >                                     run_timer_softirq()
>> >> >                                       timer_cb()
>> >> >                                         call smp_call_function_many()
>> >> >                                           send IPI interrupt to CPUA
>> >> >                                             wait_csd()
>> >> >
>> >> > Then both CPUA and CPUB will be deadlocked here.
>> >>
>> >> That's not true if called with wait = 0 as we won't wait for the csd
>> >> in that case. The function will be invoked on cpuA after it reenables
>> >> interrupt. So for callers who don't care about synchronous execution
>> >> it should not warn in softirq context.
>> >
>> > Hmm, even there it matters, because of the following scenario:
>> >
>> > CPU 0
>> > smp_call_function_single(CPU 1)
>> >     csd_lock(CPU 1)
>> >     irq_enter()
>> >     irq_exit()
>> >     __do_softirq()
>> >     smp_call_function_many()
>> >       setup csd (CPU 1)
>> >         csd_lock(CPU 1) ==> CPU 0 deadlocked itself.
>> >
>> > And this is even more likely to happen than the lock issue.
>>
>> I've observed similar deadlock in a real system which has network
>> driver that uses smp_call_function_single in the softirq context.
>>
>> The proposed fix below keeps IRQs disabled on the sending CPU
>> during the period between marking csd locked and sending IPI,
>> making it possible to use smp_call_function_single from the softirq
>> context. What do you think?
>
> I'm not really exited to encourage IPIs from irq context. Just because
> some network driver uses it, is definitely not a good argument. If we
> really want to support that, then we need a proper justification why
> it is necessary in the first place.

Then there should be at least a comment for smp_call_function_single
similar to the one for smp_call_function_many, for those who still
believe it's possible?

diff --git a/kernel/smp.c b/kernel/smp.c
index 0564571..fe31b77 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -208,6 +208,10 @@ static DEFINE_PER_CPU_SHARED_ALIGNED(struct
call_single_data, csd_data);
  * @wait: If true, wait until function has completed on other CPUs.
  *
  * Returns 0 on success, else a negative status code.
+ *
+ * You must not call this function with disabled interrupts or from a
+ * hardware interrupt handler or from a bottom half handler. Preemption
+ * must be disabled when calling this function.
  */
 int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
                             int wait)

-- 
Thanks.
-- Max

Re: [PATCH V2] smp: Give WARN()ing when calling smp_call_function_many()/single() in serving irq

[Wang YanQing]

On Fri, Jul 05, 2013 at 03:50:57PM +0200, Thomas Gleixner wrote:
> On Sat, 16 Feb 2013, Chuansheng Liu wrote:
> > There is a real case for softirq DEADLOCK case:
> > 
> > CPUA                            CPUB
> >                                 spin_lock(&spinlock)
> >                                 Any irq coming, call the irq handler
> >                                 irq_exit()
> > spin_lock_irq(&spinlock)
> > <== Blocking here due to
> > CPUB hold it
> >                                   __do_softirq()
> >                                     run_timer_softirq()
> >                                       timer_cb()
> >                                         call smp_call_function_many()
> >                                           send IPI interrupt to CPUA
> >                                             wait_csd()
> > 
> > Then both CPUA and CPUB will be deadlocked here.
> 

Why can't we just use spin_lock_irq instead of spin_lock in CPUB to
prevent this to happen ?

And the according senario for kernel/smp.c is to use raw_spin_lock_irqsave
instead of raw_spin_lock in generic_smp_call_function_single_interrupt
to protect the follow one line codes:

        raw_spin_lock(&q->lock);
        list_replace_init(&q->list, &list);
        raw_spin_unlock(&q->lock);

Thanks.