All of lore.kernel.org
 help / color / mirror / Atom feed
From: Li Zhong <zhong@linux.vnet.ibm.com>
To: LKML <linux-kernel@vger.kernel.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Paul Mackerras <paulus@samba.org>,
	PowerPC email list <linuxppc-dev@lists.ozlabs.org>
Subject: [PATCH powerpc] fix a lockdep complaint in start_secondary
Date: Thu, 17 May 2012 12:01:03 +0800	[thread overview]
Message-ID: <1337227263.24471.23.camel@ThinkPad-T420> (raw)

This patch tries to fix following lockdep complaints:

[   81.882506] =================================
[   81.882508] [ INFO: inconsistent lock state ]
[   81.882511] 3.4.0-rc4-autokern1 #1 Not tainted
[   81.882513] ---------------------------------
[   81.882516] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
[   81.882519] swapper/5/0 [HC0[0]:SC0[0]:HE1:SE1] takes:
[   81.882522]  (call_function.lock){?.....}, at:
[<c0000000000fdaa0>] .ipi_call_lock+0x20/0x40
[   81.882536] {IN-HARDIRQ-W} state was registered at:
[   81.882538]   [<c0000000000f5a9c>] .__lock_acquire+0x44c/0x9e0
[   81.882543]   [<c0000000000f60f4>] .lock_acquire+0xc4/0x260
[   81.882548]   [<c00000000063f648>] ._raw_spin_lock+0x48/0x70
[   81.882554]
[<c0000000000fede4>] .generic_smp_call_function_interrupt+0x1d4/0x320
[   81.882559]   [<c000000000037020>] .smp_ipi_demux+0x90/0x100
[   81.882565]   [<c00000000004f98c>] .icp_hv_ipi_action+0x5c/0xc0
[   81.882571]   [<c00000000013420c>] .handle_irq_event_percpu
+0xec/0x570
[   81.882577]   [<c000000000138ab4>] .handle_percpu_irq+0x84/0xd0
[   81.882582]   [<c0000000000221b4>] .call_handle_irq+0x1c/0x2c
[   81.882588]   [<c0000000000103fc>] .do_IRQ+0x16c/0x500
[   81.882593]   [<c0000000000038d0>] hardware_interrupt_common
+0x150/0x180
[   81.882598]   [<c000000000010a38>] .arch_local_irq_restore+0x38/0x90
[   81.882603]   [<c000000000017450>] .cpu_idle+0x250/0x2d0
[   81.882607]   [<c000000000651ce0>] .start_secondary+0x378/0x384
[   81.882613]   [<c00000000000936c>] .start_secondary_prolog+0x10/0x14
[   81.882618] irq event stamp: 332475
[   81.882620] hardirqs last  enabled at (332475):
[<c000000000640414>] ._raw_spin_unlock_irqrestore+0x94/0xc0
[   81.882625] hardirqs last disabled at (332474):
[<c00000000063f7c0>] ._raw_spin_lock_irqsave+0x30/0x90
[   81.882631] softirqs last  enabled at (332288):
[<c0000000000873c4>] .irq_enter+0x94/0xd0
[   81.882636] softirqs last disabled at (332287):
[<c0000000000873b4>] .irq_enter+0x84/0xd0
[   81.882640] 
[   81.882641] other info that might help us debug this:
[   81.882644]  Possible unsafe locking scenario:
[   81.882645] 
[   81.882647]        CPU0
[   81.882649]        ----
[   81.882650]   lock(call_function.lock);
[   81.882654]   <Interrupt>
[   81.882656]     lock(call_function.lock);
[   81.882660] 
[   81.882661]  *** DEADLOCK ***
[   81.882662] 
[   81.882664] no locks held by swapper/5/0.
[   81.882666] 
[   81.882667] stack backtrace:
[   81.882669] Call Trace:
[   81.882672] [c0000003c07bf860] [c0000000000146f4] .show_stack
+0x74/0x1c0 (unreliable)
[   81.882678] [c0000003c07bf910] [c0000000000f1304] .print_usage_bug
+0x1e4/0x230
[   81.882683] [c0000003c07bf9d0] [c0000000000f150c] .mark_lock_irq
+0x1bc/0x3c0
[   81.882688] [c0000003c07bfa90] [c0000000000f18a0] .mark_lock
+0x190/0x4b0
[   81.882693] [c0000003c07bfb40] [c0000000000f1d10] .mark_irqflags
+0x150/0x240
[   81.882697] [c0000003c07bfbd0] [c0000000000f5a9c] .__lock_acquire
+0x44c/0x9e0
[   81.882702] [c0000003c07bfce0] [c0000000000f60f4] .lock_acquire
+0xc4/0x260
[   81.882707] [c0000003c07bfdc0] [c00000000063f648] ._raw_spin_lock
+0x48/0x70
[   81.882712] [c0000003c07bfe50] [c0000000000fdaa0] .ipi_call_lock
+0x20/0x40
[   81.882717] [c0000003c07bfed0] [c000000000651aa0] .start_secondary
+0x138/0x384
[   81.882722] [c0000003c07bff90]
[c00000000000936c] .start_secondary_prolog+0x10/0x14


>From the log, ipi_call_lock() is called in start_secondary() with irqs
enabled. The irqs are enabled by smp_ops->setup_cpu(), in following call
chain: 
start_secondary --> smp_ops->setup_cpu --> smp_xics_setup_cpu -->
pseries_notify_cpu_idle_add_cpu --> cpuidle_disable_device -->
cpuidle_remove_state_sysfs -->  cpuidle_free_state_kobj -->
wait_for_completion --> wait_for_common

>From my understanding of the codes, I think it's not necessary to call
pseries_notify_cpu_idle_add_cpu() in the early start_secondary()
function before irqs could be enabled. 

pseries_notify_cpu_idle_add_cpu() actually does
cpuidle_disable_device(), and then cpuidle_enable_device(), which
releases and allocates the resources respectively. ( Also, all the data
are cleared and reinitialized after this cycle). The problem here is:
something like kzalloc(GFP_KERNEL), wait_for_completion() would have
problems running here where irqs are still disabled. 

Actually, cpuidle_enable_device() is called for each possible cpu when
the driver is registered. So I don't think the resources needed to be
released and allocated each time cpu becomes online. Something like
cpuidle_reset_device() would be enough to clear and reinitialize the
data.

However, after some studying of the data to be cleared, I think it's
also reasonable to keep the previous data. For example: 

/sys/devices/system/cpu/cpu#/cpuidle/state#/usage 
        the number of times this idle state has been entered
/sys/devices/system/cpu/cpu#/cpuidle/state#/time
        the amount of time spent in this idle state

So I think we could just remove the function call doing the
disable/enable cycle:

Please correct me if I missed anything. 

Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/smp.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/smp.c
b/arch/powerpc/platforms/pseries/smp.c
index e16bb8d..71706bc 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -147,7 +147,6 @@ static void __devinit smp_xics_setup_cpu(int cpu)
 	set_cpu_current_state(cpu, CPU_STATE_ONLINE);
 	set_default_offline_state(cpu);
 #endif
-	pseries_notify_cpuidle_add_cpu(cpu);
 }
 
 static int __devinit smp_pSeries_kick_cpu(int nr)
-- 
1.7.5.4

WARNING: multiple messages have this Message-ID (diff)
From: Li Zhong <zhong@linux.vnet.ibm.com>
To: LKML <linux-kernel@vger.kernel.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	PowerPC email list <linuxppc-dev@lists.ozlabs.org>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Subject: [PATCH powerpc] fix a lockdep complaint in start_secondary
Date: Thu, 17 May 2012 12:01:03 +0800	[thread overview]
Message-ID: <1337227263.24471.23.camel@ThinkPad-T420> (raw)

This patch tries to fix following lockdep complaints:

[   81.882506] =================================
[   81.882508] [ INFO: inconsistent lock state ]
[   81.882511] 3.4.0-rc4-autokern1 #1 Not tainted
[   81.882513] ---------------------------------
[   81.882516] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
[   81.882519] swapper/5/0 [HC0[0]:SC0[0]:HE1:SE1] takes:
[   81.882522]  (call_function.lock){?.....}, at:
[<c0000000000fdaa0>] .ipi_call_lock+0x20/0x40
[   81.882536] {IN-HARDIRQ-W} state was registered at:
[   81.882538]   [<c0000000000f5a9c>] .__lock_acquire+0x44c/0x9e0
[   81.882543]   [<c0000000000f60f4>] .lock_acquire+0xc4/0x260
[   81.882548]   [<c00000000063f648>] ._raw_spin_lock+0x48/0x70
[   81.882554]
[<c0000000000fede4>] .generic_smp_call_function_interrupt+0x1d4/0x320
[   81.882559]   [<c000000000037020>] .smp_ipi_demux+0x90/0x100
[   81.882565]   [<c00000000004f98c>] .icp_hv_ipi_action+0x5c/0xc0
[   81.882571]   [<c00000000013420c>] .handle_irq_event_percpu
+0xec/0x570
[   81.882577]   [<c000000000138ab4>] .handle_percpu_irq+0x84/0xd0
[   81.882582]   [<c0000000000221b4>] .call_handle_irq+0x1c/0x2c
[   81.882588]   [<c0000000000103fc>] .do_IRQ+0x16c/0x500
[   81.882593]   [<c0000000000038d0>] hardware_interrupt_common
+0x150/0x180
[   81.882598]   [<c000000000010a38>] .arch_local_irq_restore+0x38/0x90
[   81.882603]   [<c000000000017450>] .cpu_idle+0x250/0x2d0
[   81.882607]   [<c000000000651ce0>] .start_secondary+0x378/0x384
[   81.882613]   [<c00000000000936c>] .start_secondary_prolog+0x10/0x14
[   81.882618] irq event stamp: 332475
[   81.882620] hardirqs last  enabled at (332475):
[<c000000000640414>] ._raw_spin_unlock_irqrestore+0x94/0xc0
[   81.882625] hardirqs last disabled at (332474):
[<c00000000063f7c0>] ._raw_spin_lock_irqsave+0x30/0x90
[   81.882631] softirqs last  enabled at (332288):
[<c0000000000873c4>] .irq_enter+0x94/0xd0
[   81.882636] softirqs last disabled at (332287):
[<c0000000000873b4>] .irq_enter+0x84/0xd0
[   81.882640] 
[   81.882641] other info that might help us debug this:
[   81.882644]  Possible unsafe locking scenario:
[   81.882645] 
[   81.882647]        CPU0
[   81.882649]        ----
[   81.882650]   lock(call_function.lock);
[   81.882654]   <Interrupt>
[   81.882656]     lock(call_function.lock);
[   81.882660] 
[   81.882661]  *** DEADLOCK ***
[   81.882662] 
[   81.882664] no locks held by swapper/5/0.
[   81.882666] 
[   81.882667] stack backtrace:
[   81.882669] Call Trace:
[   81.882672] [c0000003c07bf860] [c0000000000146f4] .show_stack
+0x74/0x1c0 (unreliable)
[   81.882678] [c0000003c07bf910] [c0000000000f1304] .print_usage_bug
+0x1e4/0x230
[   81.882683] [c0000003c07bf9d0] [c0000000000f150c] .mark_lock_irq
+0x1bc/0x3c0
[   81.882688] [c0000003c07bfa90] [c0000000000f18a0] .mark_lock
+0x190/0x4b0
[   81.882693] [c0000003c07bfb40] [c0000000000f1d10] .mark_irqflags
+0x150/0x240
[   81.882697] [c0000003c07bfbd0] [c0000000000f5a9c] .__lock_acquire
+0x44c/0x9e0
[   81.882702] [c0000003c07bfce0] [c0000000000f60f4] .lock_acquire
+0xc4/0x260
[   81.882707] [c0000003c07bfdc0] [c00000000063f648] ._raw_spin_lock
+0x48/0x70
[   81.882712] [c0000003c07bfe50] [c0000000000fdaa0] .ipi_call_lock
+0x20/0x40
[   81.882717] [c0000003c07bfed0] [c000000000651aa0] .start_secondary
+0x138/0x384
[   81.882722] [c0000003c07bff90]
[c00000000000936c] .start_secondary_prolog+0x10/0x14


>From the log, ipi_call_lock() is called in start_secondary() with irqs
enabled. The irqs are enabled by smp_ops->setup_cpu(), in following call
chain: 
start_secondary --> smp_ops->setup_cpu --> smp_xics_setup_cpu -->
pseries_notify_cpu_idle_add_cpu --> cpuidle_disable_device -->
cpuidle_remove_state_sysfs -->  cpuidle_free_state_kobj -->
wait_for_completion --> wait_for_common

>From my understanding of the codes, I think it's not necessary to call
pseries_notify_cpu_idle_add_cpu() in the early start_secondary()
function before irqs could be enabled. 

pseries_notify_cpu_idle_add_cpu() actually does
cpuidle_disable_device(), and then cpuidle_enable_device(), which
releases and allocates the resources respectively. ( Also, all the data
are cleared and reinitialized after this cycle). The problem here is:
something like kzalloc(GFP_KERNEL), wait_for_completion() would have
problems running here where irqs are still disabled. 

Actually, cpuidle_enable_device() is called for each possible cpu when
the driver is registered. So I don't think the resources needed to be
released and allocated each time cpu becomes online. Something like
cpuidle_reset_device() would be enough to clear and reinitialize the
data.

However, after some studying of the data to be cleared, I think it's
also reasonable to keep the previous data. For example: 

/sys/devices/system/cpu/cpu#/cpuidle/state#/usage 
        the number of times this idle state has been entered
/sys/devices/system/cpu/cpu#/cpuidle/state#/time
        the amount of time spent in this idle state

So I think we could just remove the function call doing the
disable/enable cycle:

Please correct me if I missed anything. 

Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/smp.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/smp.c
b/arch/powerpc/platforms/pseries/smp.c
index e16bb8d..71706bc 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -147,7 +147,6 @@ static void __devinit smp_xics_setup_cpu(int cpu)
 	set_cpu_current_state(cpu, CPU_STATE_ONLINE);
 	set_default_offline_state(cpu);
 #endif
-	pseries_notify_cpuidle_add_cpu(cpu);
 }
 
 static int __devinit smp_pSeries_kick_cpu(int nr)
-- 
1.7.5.4


             reply	other threads:[~2012-05-17  4:02 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-05-17  4:01 Li Zhong [this message]
2012-05-17  4:01 ` [PATCH powerpc] fix a lockdep complaint in start_secondary Li Zhong
2012-05-17  4:28 ` Benjamin Herrenschmidt
2012-05-17  4:28   ` Benjamin Herrenschmidt
2012-05-17 10:22   ` Deepthi Dharwar
2012-05-17 10:22     ` Deepthi Dharwar
2012-05-18  2:54     ` Li Zhong
2012-05-18  2:54       ` Li Zhong
2012-05-18 11:24       ` Deepthi Dharwar
2012-05-18 11:24         ` Deepthi Dharwar
2012-05-18 12:17         ` Li Zhong
2012-05-18 12:17           ` Li Zhong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1337227263.24471.23.camel@ThinkPad-T420 \
    --to=zhong@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=paulus@samba.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.