[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 <chuansh...@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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[tip-bot for Chuansheng Liu]

Commit-ID: b29f39c7c3e75a741a7da88244ec707f293ec04c
Gitweb: http://git.kernel.org/tip/b29f39c7c3e75a741a7da88244ec707f293ec04c
Author: Chuansheng Liu <chuansh...@intel.com>
AuthorDate: Wed, 6 Feb 2013 23:18:21 +0800
Committer: Ingo Molnar <mi...@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 <chuansh...@intel.com>
Cc: jun....@intel.com
Cc: pet...@infradead.org
Cc: jbeu...@suse.com
Cc: pau...@linux.vnet.ibm.com
Cc: min...@mina86.org
Cc: srivat...@linux.vnet.ibm.com
Cc: Linus Torvalds <torv...@linux-foundation.org>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Link: http://lkml.kernel.org/r/1360163901.24670.13.camel@cliu38-desktop-build
Signed-off-by: Ingo Molnar <mi...@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-bot for Ingo Molnar]

Commit-ID: 56624143151fdb84c32a43463864e6c12a5ebcfc
Gitweb: http://git.kernel.org/tip/56624143151fdb84c32a43463864e6c12a5ebcfc
Author: Ingo Molnar <mi...@kernel.org>
AuthorDate: Mon, 11 Feb 2013 10:03:29 +0100
Committer: Ingo Molnar <mi...@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 <fenggu...@intel.com>
Cc: liu chuansheng <chuansh...@intel.com>

Cc: jun....@intel.com
Cc: pet...@infradead.org
Cc: jbeu...@suse.com
Cc: pau...@linux.vnet.ibm.com
Cc: min...@mina86.org
Cc: srivat...@linux.vnet.ibm.com
Cc: Linus Torvalds <torv...@linux-foundation.org>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Link: http://lkml.kernel.org/r/1360163901.24670.13.camel@cliu38-desktop-build
Signed-off-by: Ingo Molnar <mi...@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);
--

[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 <chuansh...@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);

[Chuansheng Liu]

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,

[Liu, Chuansheng]

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‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±‘êçzX§¶ ›¡Ü¨}©ž²Æ zÚ&j:+v‰¨¾ «‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹ ®w¥¢¸?™¨è­Ú&¢)ߢ f”ù^jÇ«y§m…á@A«a¶Ú ÿ0¶ìh® å’i

[Fengguang Wu]

Chuansheng,

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

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

[Andrew Morton]

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

> Chuansheng,
>
> It works fine on tip/next. Thanks for the fix!
>
> Tested-by: Fengguang Wu <fenggu...@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?

[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 <fenggu...@intel.com> wrote:
>
> > Chuansheng,
> >
> > It works fine on tip/next. Thanks for the fix!
> >
> > Tested-by: Fengguang Wu <fenggu...@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

[Liu, Chuansheng]

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;

[Lai Jiangshan]

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 <la...@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

[Liu, Chuansheng]

> -----Original Message-----
> From: Lai Jiangshan [mailto:eag...@gmail.com]
> Sent: Wednesday, February 27, 2013 10:51 PM
> To: Liu, Chuansheng
> Cc: mi...@kernel.org; pet...@infradead.org; jbeu...@suse.com;
> pau...@linux.vnet.ibm.com; ak...@linux-foundation.org;
> min...@mina86.org; srivat...@linux.vnet.ibm.com;
> linux-...@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
>

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 <la...@cn.fujitsu.com>

[Thomas Gleixner]

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

[Thomas Gleixner]

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.

[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.

[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.

[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 :)

[Andrew Morton]

On Fri, 1 Mar 2013 03:37:11 +0000 "Liu, Chuansheng" <chuansh...@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?

Signed-off-by: liu chuansheng <chuansh...@intel.com>
Cc: Ingo Molnar <mi...@elte.hu>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Tested-by: Fengguang Wu <fenggu...@intel.com>
Cc: Lai Jiangshan <eag...@gmail.com>
Signed-off-by: Andrew Morton <ak...@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);

_

[Max Filippov]

Hi Thomas,

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 <jcmv...@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 <jcmv...@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

[Thomas Gleixner]

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

[Max Filippov]

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