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 <redacted>
Signed-off-by: Li Zhong <redacted>
Tested-by: Paul E. McKenney <redacted>
---
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-05-17 04:28:47
On Thu, 2012-05-17 at 12:01 +0800, Li Zhong wrote:
This patch tries to fix following lockdep complaints:
.../...
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.
So yes, it looks definitely fishy. I don't have time to study cpuidle
today to check whether that's correct or not so I'm CCing Deepthi
Dharwar who did all that cpuidle work for pseries.
Deepthi, can you check whether that patch is correct ?
quoted hunk
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 <redacted>
Signed-off-by: Li Zhong <redacted>
Tested-by: Paul E. McKenney <redacted>
---
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
On 05/17/2012 09:58 AM, Benjamin Herrenschmidt wrote:
On Thu, 2012-05-17 at 12:01 +0800, Li Zhong wrote:
quoted
This patch tries to fix following lockdep complaints:
.../...
quoted
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.
This is true when the system is booting up.
So yes, it looks definitely fishy. I don't have time to study cpuidle
today to check whether that's correct or not so I'm CCing Deepthi
Dharwar who did all that cpuidle work for pseries.
Deepthi, can you check whether that patch is correct ?
pseries_notify_cpu_idle_add_cpu() is essential to be called for
hotplug event. So by removing this call completely wouldn't
support cpus registering under cpuidle on hotplug and default idle is
executed on those with do not give much powersavings. Ideal way it to
have a notifier in pseries backend driver for hotplug notification and
then remove this function from here.
I am currently working on this patch, will post it out soon.
quoted
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.
If removed, this would not handle cpu hotplug events for cpuidle.
quoted
Reported-by: Paul E. McKenney <redacted>
Signed-off-by: Li Zhong <redacted>
Tested-by: Paul E. McKenney <redacted>
---
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
On Thu, 2012-05-17 at 15:52 +0530, Deepthi Dharwar wrote:
On 05/17/2012 09:58 AM, Benjamin Herrenschmidt wrote:
quoted
On Thu, 2012-05-17 at 12:01 +0800, Li Zhong wrote:
quoted
This patch tries to fix following lockdep complaints:
.../...
quoted
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.
This is true when the system is booting up.
quoted
So yes, it looks definitely fishy. I don't have time to study cpuidle
today to check whether that's correct or not so I'm CCing Deepthi
Dharwar who did all that cpuidle work for pseries.
Deepthi, can you check whether that patch is correct ?
pseries_notify_cpu_idle_add_cpu() is essential to be called for
hotplug event. So by removing this call completely wouldn't
support cpus registering under cpuidle on hotplug and default idle is
executed on those with do not give much powersavings.
Maybe I missed that part.. would you please give some details how
removing this would prevent powersaving cpuidle being called after
hotplug?
After rereading the codes, I think ppc_md.power_save() is the one you
mentioned that could give much powersavings?
It is registered as pSeries_idle(), which calls cpuidle_idle_call().
It seems to me that it would still be called after hotplug.
Or maybe I misunderstood your point?
Ideal way it to
have a notifier in pseries backend driver for hotplug notification and
then remove this function from here.
I am currently working on this patch, will post it out soon.
quoted
quoted
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.
If removed, this would not handle cpu hotplug events for cpuidle.
quoted
quoted
Reported-by: Paul E. McKenney <redacted>
Signed-off-by: Li Zhong <redacted>
Tested-by: Paul E. McKenney <redacted>
---
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
On Thu, 2012-05-17 at 15:52 +0530, Deepthi Dharwar wrote:
quoted
On 05/17/2012 09:58 AM, Benjamin Herrenschmidt wrote:
quoted
On Thu, 2012-05-17 at 12:01 +0800, Li Zhong wrote:
quoted
This patch tries to fix following lockdep complaints:
.../...
quoted
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.
This is true when the system is booting up.
quoted
So yes, it looks definitely fishy. I don't have time to study cpuidle
today to check whether that's correct or not so I'm CCing Deepthi
Dharwar who did all that cpuidle work for pseries.
Deepthi, can you check whether that patch is correct ?
pseries_notify_cpu_idle_add_cpu() is essential to be called for
hotplug event. So by removing this call completely wouldn't
support cpus registering under cpuidle on hotplug and default idle is
executed on those with do not give much powersavings.
Maybe I missed that part.. would you please give some details how
removing this would prevent powersaving cpuidle being called after
hotplug?
After rereading the codes, I think ppc_md.power_save() is the one you
mentioned that could give much powersavings?
It is registered as pSeries_idle(), which calls cpuidle_idle_call().
It seems to me that it would still be called after hotplug.
Or maybe I misunderstood your point?
If cpuidle_idle_call() fails, in case device is not present, off ,
not initialized and not ready to use, default idle is called.
Coming out of a hotplug event, it is good to cleanly exit out
and reallocate all the resources when needed, rather than using the
stale one to make sure this call succeeds always.
Default idle executed in pSeries_idle() :
HMT_low();
HMT_very_low();
This would not have much powersavings.
CPUIDLE subsystem needs to be informed when a hot plug event occurs
and not a good practice to mask this subsystem from this system wide
event.
I agree that putting it in xics setup is not a good thing.
Notifier would be a cleaner way of doing it.
That way, duplication of resources allocated and released at boot
time is not done.
quoted
Ideal way it to
have a notifier in pseries backend driver for hotplug notification and
then remove this function from here.
I am currently working on this patch, will post it out soon.
quoted
quoted
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.
If removed, this would not handle cpu hotplug events for cpuidle.
quoted
quoted
Reported-by: Paul E. McKenney <redacted>
Signed-off-by: Li Zhong <redacted>
Tested-by: Paul E. McKenney <redacted>
---
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
On Fri, 2012-05-18 at 16:54 +0530, Deepthi Dharwar wrote:
On 05/18/2012 08:24 AM, Li Zhong wrote:
quoted
On Thu, 2012-05-17 at 15:52 +0530, Deepthi Dharwar wrote:
quoted
On 05/17/2012 09:58 AM, Benjamin Herrenschmidt wrote:
quoted
On Thu, 2012-05-17 at 12:01 +0800, Li Zhong wrote:
quoted
This patch tries to fix following lockdep complaints:
.../...
quoted
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.
This is true when the system is booting up.
quoted
So yes, it looks definitely fishy. I don't have time to study cpuidle
today to check whether that's correct or not so I'm CCing Deepthi
Dharwar who did all that cpuidle work for pseries.
Deepthi, can you check whether that patch is correct ?
pseries_notify_cpu_idle_add_cpu() is essential to be called for
hotplug event. So by removing this call completely wouldn't
support cpus registering under cpuidle on hotplug and default idle is
executed on those with do not give much powersavings.
Maybe I missed that part.. would you please give some details how
removing this would prevent powersaving cpuidle being called after
hotplug?
After rereading the codes, I think ppc_md.power_save() is the one you
mentioned that could give much powersavings?
It is registered as pSeries_idle(), which calls cpuidle_idle_call().
It seems to me that it would still be called after hotplug.
Or maybe I misunderstood your point?
If cpuidle_idle_call() fails, in case device is not present, off ,
not initialized and not ready to use, default idle is called.
Coming out of a hotplug event, it is good to cleanly exit out
and reallocate all the resources when needed, rather than using the
stale one to make sure this call succeeds always.
Default idle executed in pSeries_idle() :
HMT_low();
HMT_very_low();
This would not have much powersavings.
From my testing, cpuidle_idle_call didn't fail after hotplug, so it
didn't fall back to the default idling. I still don't see any big
problems if we don't reallocate the resources.
CPUIDLE subsystem needs to be informed when a hot plug event occurs
and not a good practice to mask this subsystem from this system wide
event.
Ok, I agree that the CPUIDLE subsystem should be notified about the hot
plug events. Thank you.
I think this would be included in your coming patch, and I could just
stop here, hehe
I agree that putting it in xics setup is not a good thing.
Notifier would be a cleaner way of doing it.
That way, duplication of resources allocated and released at boot
time is not done.
quoted
quoted
Ideal way it to
have a notifier in pseries backend driver for hotplug notification and
then remove this function from here.
I am currently working on this patch, will post it out soon.
quoted
quoted
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.
If removed, this would not handle cpu hotplug events for cpuidle.
quoted
quoted
Reported-by: Paul E. McKenney <redacted>
Signed-off-by: Li Zhong <redacted>
Tested-by: Paul E. McKenney <redacted>
---
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