[PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Vincent Guittot]

Please find below a new proposal for adding trace events for cpu hotplug:
-the lock/unlock of cpu_add_remove_lock mutex is now outside the trace

The goal is to measure the latency of each part (kernel, architecture)
and also to trace the cpu hotplug activity with other power events. I
have tested these traces events on an arm platform.

Subject: [PATCH 2/2] add hotplug tracepoint

this patch adds new events for cpu hotplug tracing
 * plug/unplug sequence
 * core and architecture latency measurements

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/cpu.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 156cc55..8abb84b 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -16,6 +16,9 @@
 #include <linux/mutex.h>
 #include <linux/gfp.h>

+#define CREATE_TRACE_POINTS
+#include <trace/events/cpu_hotplug.h>
+
 #ifdef CONFIG_SMP
 /* Serializes the updates to cpu_online_mask, cpu_present_mask */
 static DEFINE_MUTEX(cpu_add_remove_lock);
@@ -197,10 +200,13 @@ struct take_cpu_down_param {
 static int __ref take_cpu_down(void *_param)
 {
 	struct take_cpu_down_param *param = _param;
+	unsigned int cpu = (unsigned int)(param->hcpu);
 	int err;

 	/* Ensure this CPU doesn't handle any more interrupts. */
+	trace_cpu_hotplug_disable_start(cpu);
 	err = __cpu_disable();
+	trace_cpu_hotplug_disable_end(cpu);
 	if (err < 0)
 		return err;

@@ -256,7 +262,9 @@ static int __ref _cpu_down(unsigned int cpu, int
tasks_frozen)
 		cpu_relax();

 	/* This actually kills the CPU. */
+	trace_cpu_hotplug_die_start(cpu);
 	__cpu_die(cpu);
+	trace_cpu_hotplug_die_end(cpu);

 	/* CPU is completely dead: tell everyone.  Too late to complain. */
 	cpu_notify_nofail(CPU_DEAD | mod, hcpu);
@@ -276,6 +284,8 @@ int __ref cpu_down(unsigned int cpu)

 	cpu_maps_update_begin();

+	trace_cpu_hotplug_down_start(cpu);
+
 	if (cpu_hotplug_disabled) {
 		err = -EBUSY;
 		goto out;
@@ -284,6 +294,8 @@ int __ref cpu_down(unsigned int cpu)
 	err = _cpu_down(cpu, 0);

 out:
+	trace_cpu_hotplug_down_end(cpu);
+
 	cpu_maps_update_done();
 	return err;
 }
@@ -310,7 +322,9 @@ static int __cpuinit _cpu_up(unsigned int cpu, int
tasks_frozen)
 	}

 	/* Arch-specific enabling code. */
+	trace_cpu_hotplug_arch_up_start(cpu);
 	ret = __cpu_up(cpu);
+	trace_cpu_hotplug_arch_up_end(cpu);
 	if (ret != 0)
 		goto out_notify;
 	BUG_ON(!cpu_online(cpu));
@@ -369,6 +383,8 @@ int __cpuinit cpu_up(unsigned int cpu)

 	cpu_maps_update_begin();

+	trace_cpu_hotplug_up_start(cpu);
+
 	if (cpu_hotplug_disabled) {
 		err = -EBUSY;
 		goto out;
@@ -377,6 +393,8 @@ int __cpuinit cpu_up(unsigned int cpu)
 	err = _cpu_up(cpu, 0);

 out:
+	trace_cpu_hotplug_up_end(cpu);
+
 	cpu_maps_update_done();
 	return err;
 }
-- 
1.7.1

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

On Thu, 24 Feb 2011, Vincent Guittot wrote:

Please send full patch series not a single V5 2/2 which lacks any
references to 0/2 1/2.

> Please find below a new proposal for adding trace events for cpu hotplug:

Either it's a patch or a proposal. Darn, why think people that
proposal is such a important word? It's just useless. You don't have
to sell anything to your manager. You provide a patch which is judged
on it's technical merits and correctness. Nothing else.

> -the lock/unlock of cpu_add_remove_lock mutex is now outside the trace
> 
> The goal is to measure the latency of each part (kernel, architecture)
> and also to trace the cpu hotplug activity with other power events. I
> have tested these traces events on an arm platform.

This belongs into a cover mail [0/2] not into the patch itself
 
> Subject: [PATCH 2/2] add hotplug tracepoint

While your mail subject is correct, this is not.

If you would have sent a [0/2] cover mail with all the above blurb in
it then this extra subject line would be not needed at all.

> this patch adds new events for cpu hotplug tracing

Sentences start with an upper case letter. 

Also we already know that this is a patch. Where is the value of this
changelog? It does not tell more than the subject line.

>  * plug/unplug sequence

How surprising.

>  * core and architecture latency measurements

No it does not. It does not add latency measurements. It merily adds
tracepoints which allow you to compute the time spent in the various
steps of the hotplug state machine and the overall time.

>  #ifdef CONFIG_SMP
>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
>  static DEFINE_MUTEX(cpu_add_remove_lock);
> @@ -197,10 +200,13 @@ struct take_cpu_down_param {
>  static int __ref take_cpu_down(void *_param)
>  {
>  	struct take_cpu_down_param *param = _param;
> +	unsigned int cpu = (unsigned int)(param->hcpu);
>  	int err;
> 
>  	/* Ensure this CPU doesn't handle any more interrupts. */
> +	trace_cpu_hotplug_disable_start(cpu);
>  	err = __cpu_disable();
> +	trace_cpu_hotplug_disable_end(cpu);

How useful. What about recording the return code of __cpu_disable()?

>  	if (err < 0)
>  		return err;

> +	trace_cpu_hotplug_down_start(cpu);
> +

What's the point of this tracepoint _BEFORE_ the cpu_hotplug_disabled
check without recording cpu_hotplug_disabled ?

>  	if (cpu_hotplug_disabled) {
>  		err = -EBUSY;
>  		goto out;
> @@ -284,6 +294,8 @@ int __ref cpu_down(unsigned int cpu)
>  	err = _cpu_down(cpu, 0);
> 
>  out:
> +	trace_cpu_hotplug_down_end(cpu);

And this one is misplaced as well. It wants to be only called when we
actually called _cpu_down() and it wants to record the return code as
well.

> +
>  	cpu_maps_update_done();
>  	return err;
>  }
> @@ -310,7 +322,9 @@ static int __cpuinit _cpu_up(unsigned int cpu, int
> tasks_frozen)
>  	}
> 
>  	/* Arch-specific enabling code. */
> +	trace_cpu_hotplug_arch_up_start(cpu);
>  	ret = __cpu_up(cpu);
> +	trace_cpu_hotplug_arch_up_end(cpu);

See above.

>  	if (ret != 0)
>  		goto out_notify;
>  	BUG_ON(!cpu_online(cpu));
> @@ -369,6 +383,8 @@ int __cpuinit cpu_up(unsigned int cpu)
> 
>  	cpu_maps_update_begin();
> 
> +	trace_cpu_hotplug_up_start(cpu);
> +

Ditto

>  	if (cpu_hotplug_disabled) {
>  		err = -EBUSY;
>  		goto out;
> @@ -377,6 +393,8 @@ int __cpuinit cpu_up(unsigned int cpu)
>  	err = _cpu_up(cpu, 0);
> 
>  out:
> +	trace_cpu_hotplug_up_end(cpu);
> +

Sigh.

>  	cpu_maps_update_done();
>  	return err;

Thanks,

	tglx

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Vincent Guittot]

On 24 February 2011 19:40, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 24 Feb 2011, Vincent Guittot wrote:
>
> Please send full patch series not a single V5 2/2 which lacks any
> references to 0/2 1/2.
>

I'm going to create a complete patch series and add a 0/2 reference
with more details about the patch purpose.

This patch adds traces for 2 main goals. The 1st one is to detect the
plug and unplug of a core. As explained by Nicolas, smp arm platforms
use the cpu hotplug feature. In fact, the state of a core can modify
the cpuidle activity because some Arm SoC can't go into deep idle
state when more than one core is plugged and also because running into
mono core mode makes the cpuidle job easier and more efficient which
results in the improvement of powersaving of some use cases. That's
why it's interesting to monitor the plug state of cores and to
correlate it with cpuidle traces. The goal is not to make cpu hotplug
feature part of cpuidle one but to use cpu hotplug in a low frequency
manner (few dozens of seconds or minutes), we plug several cpus only
when needed by the system activity.
Then, the 2nd goal of these traces is to measure the duration of cpu
plug/unplug sequence across various use cases and cpu load. Cpu
hotplug is known to be an expensive operation which also takes a
variable time depending of other processes' activity (from hundreds of
ms up to few seconds). I have seen with these traces that the arch
part stays almost constant whatever the cpu load is on arm platform,
and I have also seen that the core duration depends of threads
creation when we plug a cpu.

>> Please find below a new proposal for adding trace events for cpu hotplug:
>
> Either it's a patch or a proposal. Darn, why think people that
> proposal is such a important word? It's just useless. You don't have
> to sell anything to your manager. You provide a patch which is judged
> on it's technical merits and correctness. Nothing else.
>
>> -the lock/unlock of cpu_add_remove_lock mutex is now outside the trace
>>
>> The goal is to measure the latency of each part (kernel, architecture)
>> and also to trace the cpu hotplug activity with other power events. I
>> have tested these traces events on an arm platform.
>
> This belongs into a cover mail [0/2] not into the patch itself
>
>> Subject: [PATCH 2/2] add hotplug tracepoint
>
> While your mail subject is correct, this is not.
>
> If you would have sent a [0/2] cover mail with all the above blurb in
> it then this extra subject line would be not needed at all.
>
>> this patch adds new events for cpu hotplug tracing
>
> Sentences start with an upper case letter.
>
> Also we already know that this is a patch. Where is the value of this
> changelog? It does not tell more than the subject line.
>
>>  * plug/unplug sequence
>
> How surprising.
>
>>  * core and architecture latency measurements
>
> No it does not. It does not add latency measurements. It merily adds
> tracepoints which allow you to compute the time spent in the various
> steps of the hotplug state machine and the overall time.
>
>>  #ifdef CONFIG_SMP
>>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
>>  static DEFINE_MUTEX(cpu_add_remove_lock);
>> @@ -197,10 +200,13 @@ struct take_cpu_down_param {
>>  static int __ref take_cpu_down(void *_param)
>>  {
>>       struct take_cpu_down_param *param = _param;
>> +     unsigned int cpu = (unsigned int)(param->hcpu);
>>       int err;
>>
>>       /* Ensure this CPU doesn't handle any more interrupts. */
>> +     trace_cpu_hotplug_disable_start(cpu);
>>       err = __cpu_disable();
>> +     trace_cpu_hotplug_disable_end(cpu);
>
> How useful. What about recording the return code of __cpu_disable()?
>

The goal is to monitor the cpu hotplug activity and duration. I want
to detect 2 kind of cpu_down/cpu_up call, ones which succeed to
unplug/plug a core and ones which don't. But I'm not sure that we need
to sort the failed calls into to the trace. We trace them because too
much fails could point out a bug or a wrong use of cpu hotplug.

>>       if (err < 0)
>>               return err;
>
>> +     trace_cpu_hotplug_down_start(cpu);
>> +
>
> What's the point of this tracepoint _BEFORE_ the cpu_hotplug_disabled
> check without recording cpu_hotplug_disabled ?
>

I want to trace all cpu_down call even those which returns immediately
which will be part of the failed calls.

>>       if (cpu_hotplug_disabled) {
>>               err = -EBUSY;
>>               goto out;
>> @@ -284,6 +294,8 @@ int __ref cpu_down(unsigned int cpu)
>>       err = _cpu_down(cpu, 0);
>>
>>  out:
>> +     trace_cpu_hotplug_down_end(cpu);
>
> And this one is misplaced as well. It wants to be only called when we
> actually called _cpu_down() and it wants to record the return code as
> well.
>

It has been placed here to be called each time
trace_cpu_hotplug_down_start is called.

>> +
>>       cpu_maps_update_done();
>>       return err;
>>  }
>> @@ -310,7 +322,9 @@ static int __cpuinit _cpu_up(unsigned int cpu, int
>> tasks_frozen)
>>       }
>>
>>       /* Arch-specific enabling code. */
>> +     trace_cpu_hotplug_arch_up_start(cpu);
>>       ret = __cpu_up(cpu);
>> +     trace_cpu_hotplug_arch_up_end(cpu);
>
> See above.
>
>>       if (ret != 0)
>>               goto out_notify;
>>       BUG_ON(!cpu_online(cpu));
>> @@ -369,6 +383,8 @@ int __cpuinit cpu_up(unsigned int cpu)
>>
>>       cpu_maps_update_begin();
>>
>> +     trace_cpu_hotplug_up_start(cpu);
>> +
>
> Ditto
>
>>       if (cpu_hotplug_disabled) {
>>               err = -EBUSY;
>>               goto out;
>> @@ -377,6 +393,8 @@ int __cpuinit cpu_up(unsigned int cpu)
>>       err = _cpu_up(cpu, 0);
>>
>>  out:
>> +     trace_cpu_hotplug_up_end(cpu);
>> +
>
> Sigh.
>
>>       cpu_maps_update_done();
>>       return err;
>
> Thanks,
>
>        tglx
>

Thanks,
Vincent

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2796 bytes --]

Vincent,

On Mon, 28 Feb 2011, Vincent Guittot wrote:
> On 24 February 2011 19:40, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 24 Feb 2011, Vincent Guittot wrote:

Sorry, this mail got eaten somehow, but I got it from my archive.

> >>  #ifdef CONFIG_SMP
> >>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> >>  static DEFINE_MUTEX(cpu_add_remove_lock);
> >> @@ -197,10 +200,13 @@ struct take_cpu_down_param {
> >>  static int __ref take_cpu_down(void *_param)
> >>  {
> >>       struct take_cpu_down_param *param = _param;
> >> +     unsigned int cpu = (unsigned int)(param->hcpu);
> >>       int err;
> >>
> >>       /* Ensure this CPU doesn't handle any more interrupts. */
> >> +     trace_cpu_hotplug_disable_start(cpu);
> >>       err = __cpu_disable();
> >> +     trace_cpu_hotplug_disable_end(cpu);
> >
> > How useful. What about recording the return code of __cpu_disable()?
> >
> 
> The goal is to monitor the cpu hotplug activity and duration. I want
> to detect 2 kind of cpu_down/cpu_up call, ones which succeed to
> unplug/plug a core and ones which don't. But I'm not sure that we need
> to sort the failed calls into to the trace. We trace them because too
> much fails could point out a bug or a wrong use of cpu hotplug.

This does not make sense at all. You want to see the failures, then
recording the error code makes even more sense. Your way of decoding
the error case by checking whether the next trace entry is there or
missing is just sloppy.
 
> >>       if (err < 0)
> >>               return err;
> >
> >> +     trace_cpu_hotplug_down_start(cpu);
> >> +
> >
> > What's the point of this tracepoint _BEFORE_ the cpu_hotplug_disabled
> > check without recording cpu_hotplug_disabled ?
> >
> 
> I want to trace all cpu_down call even those which returns immediately
> which will be part of the failed calls.

Decoding failure from missing entries is simply wrong.
 
> >>       if (cpu_hotplug_disabled) {
> >>               err = -EBUSY;
> >>               goto out;
> >> @@ -284,6 +294,8 @@ int __ref cpu_down(unsigned int cpu)
> >>       err = _cpu_down(cpu, 0);
> >>
> >>  out:
> >> +     trace_cpu_hotplug_down_end(cpu);
> >
> > And this one is misplaced as well. It wants to be only called when we
> > actually called _cpu_down() and it wants to record the return code as
> > well.
> >
> 
> It has been placed here to be called each time
> trace_cpu_hotplug_down_start is called.

That does not make sense at all. You cannot distinguish between
cpu_hotplug_disabled set and the _cpu_down() being called case. Or do
you want to tell me that you decode that from the time stamps? Hell
no. We want traces which are readable w/o crystal balls.
 
Thanks,

	tglx

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Vincent Guittot]

On 2 March 2011 11:08, Thomas Gleixner <tglx@linutronix.de> wrote:
> Vincent,
>
> On Mon, 28 Feb 2011, Vincent Guittot wrote:
>> On 24 February 2011 19:40, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Thu, 24 Feb 2011, Vincent Guittot wrote:
>
> Sorry, this mail got eaten somehow, but I got it from my archive.
>
>> >>  #ifdef CONFIG_SMP
>> >>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
>> >>  static DEFINE_MUTEX(cpu_add_remove_lock);
>> >> @@ -197,10 +200,13 @@ struct take_cpu_down_param {
>> >>  static int __ref take_cpu_down(void *_param)
>> >>  {
>> >>       struct take_cpu_down_param *param = _param;
>> >> +     unsigned int cpu = (unsigned int)(param->hcpu);
>> >>       int err;
>> >>
>> >>       /* Ensure this CPU doesn't handle any more interrupts. */
>> >> +     trace_cpu_hotplug_disable_start(cpu);
>> >>       err = __cpu_disable();
>> >> +     trace_cpu_hotplug_disable_end(cpu);
>> >
>> > How useful. What about recording the return code of __cpu_disable()?
>> >
>>
>> The goal is to monitor the cpu hotplug activity and duration. I want
>> to detect 2 kind of cpu_down/cpu_up call, ones which succeed to
>> unplug/plug a core and ones which don't. But I'm not sure that we need
>> to sort the failed calls into to the trace. We trace them because too
>> much fails could point out a bug or a wrong use of cpu hotplug.
>
> This does not make sense at all. You want to see the failures, then
> recording the error code makes even more sense. Your way of decoding
> the error case by checking whether the next trace entry is there or
> missing is just sloppy.
>

The 1st goal was to focus on profiling and to make trace events as
simple as possible but I agree that having all information is a better
option. We can add a parameter in the trace which gets the return code
or some test result like the value of cpu_hotplug_disabled.

>> >>       if (err < 0)
>> >>               return err;
>> >
>> >> +     trace_cpu_hotplug_down_start(cpu);
>> >> +
>> >
>> > What's the point of this tracepoint _BEFORE_ the cpu_hotplug_disabled
>> > check without recording cpu_hotplug_disabled ?
>> >
>>
>> I want to trace all cpu_down call even those which returns immediately
>> which will be part of the failed calls.
>
> Decoding failure from missing entries is simply wrong.
>
>> >>       if (cpu_hotplug_disabled) {
>> >>               err = -EBUSY;
>> >>               goto out;
>> >> @@ -284,6 +294,8 @@ int __ref cpu_down(unsigned int cpu)
>> >>       err = _cpu_down(cpu, 0);
>> >>
>> >>  out:
>> >> +     trace_cpu_hotplug_down_end(cpu);
>> >
>> > And this one is misplaced as well. It wants to be only called when we
>> > actually called _cpu_down() and it wants to record the return code as
>> > well.
>> >
>>
>> It has been placed here to be called each time
>> trace_cpu_hotplug_down_start is called.
>
> That does not make sense at all. You cannot distinguish between
> cpu_hotplug_disabled set and the _cpu_down() being called case. Or do
> you want to tell me that you decode that from the time stamps? Hell
> no. We want traces which are readable w/o crystal balls.
>
> Thanks,
>
>        tglx

Thanks,
Vincent

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

Vincent,

On Wed, 2 Mar 2011, Vincent Guittot wrote:
> On 2 March 2011 11:08, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Mon, 28 Feb 2011, Vincent Guittot wrote:
> >> The goal is to monitor the cpu hotplug activity and duration. I want
> >> to detect 2 kind of cpu_down/cpu_up call, ones which succeed to
> >> unplug/plug a core and ones which don't. But I'm not sure that we need
> >> to sort the failed calls into to the trace. We trace them because too
> >> much fails could point out a bug or a wrong use of cpu hotplug.
> >
> > This does not make sense at all. You want to see the failures, then
> > recording the error code makes even more sense. Your way of decoding
> > the error case by checking whether the next trace entry is there or
> > missing is just sloppy.
> >
> 
> The 1st goal was to focus on profiling and to make trace events as
> simple as possible but I agree that having all information is a better
> option. We can add a parameter in the trace which gets the return code
> or some test result like the value of cpu_hotplug_disabled.

That's neither a question of focus nor of better options.

The main point is correctness and usefulness. When we add new
facilities or infrastructure we want to make sure that they are
general useful and correct for all possible use cases we can imagine
at that point in time.

So yes, I understand your reasoning and your focus on your primary
interest, but I also want you to understand that this kind of review
has a very practical background (i.e. maintainability) and is not just
the annoying bullying people around conducted by grumpy old men.
 
Thanks,

	tglx

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Peter Zijlstra]

On Thu, 2011-02-24 at 18:33 +0100, Vincent Guittot wrote:

> The goal is to measure the latency of each part (kernel, architecture)
> and also to trace the cpu hotplug activity with other power events. I
> have tested these traces events on an arm platform.

> this patch adds new events for cpu hotplug tracing
>  * plug/unplug sequence
>  * core and architecture latency measurements

Aside of the points tglx made, I do have to ask _WHY_!?

Anybody who is interested in the latency of cpu hotplug is deluding
himself, also cpu hotplug is _NOT_ a power management feature, so the
rest of your justification just disappeared as well.

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Alan Cox]

> Anybody who is interested in the latency of cpu hotplug is deluding
> himself, also cpu hotplug is _NOT_ a power management feature, so the
> rest of your justification just disappeared as well.

Actually CPU hotplug is a power management feature on some devices where
you need to shutdown one of the cores to enter low power modes.
Remember we use it as part of the suspend paths and various processors
nowdays drop into a suspend to RAM type state on CPU idling.

Alan

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

On Thu, 24 Feb 2011, Alan Cox wrote:

> > Anybody who is interested in the latency of cpu hotplug is deluding
> > himself, also cpu hotplug is _NOT_ a power management feature, so the
> > rest of your justification just disappeared as well.
> 
> Actually CPU hotplug is a power management feature on some devices where
> you need to shutdown one of the cores to enter low power modes.

Right.

> Remember we use it as part of the suspend paths and various processors
> nowdays drop into a suspend to RAM type state on CPU idling.

The suspend path is using it, but the cpu idle one is hardly using the
hotplug muck.

Thanks,

	tglx

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Nicolas Pitre]

On Thu, 24 Feb 2011, Thomas Gleixner wrote:

> On Thu, 24 Feb 2011, Alan Cox wrote:
> 
> > > Anybody who is interested in the latency of cpu hotplug is deluding
> > > himself, also cpu hotplug is _NOT_ a power management feature, so the
> > > rest of your justification just disappeared as well.
> > 
> > Actually CPU hotplug is a power management feature on some devices where
> > you need to shutdown one of the cores to enter low power modes.
> 
> Right.
> 
> > Remember we use it as part of the suspend paths and various processors
> > nowdays drop into a suspend to RAM type state on CPU idling.
> 
> The suspend path is using it, but the cpu idle one is hardly using the
> hotplug muck.

Most SMP ARM processors are going to use it soon.  Powering down idle 
cores provides substantial power saving.


Nicolas

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Peter Zijlstra]

On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:

> Most SMP ARM processors are going to use it soon.  Powering down idle 
> cores provides substantial power saving.

And why can't regular idle paths be used? CPU hotplug is a massively
expensive operation.

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Alan Cox]

On Thu, 24 Feb 2011 21:30:52 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:
> 
> > Most SMP ARM processors are going to use it soon.  Powering down idle 
> > cores provides substantial power saving.

And some X86 will be doing likewise

> And why can't regular idle paths be used? CPU hotplug is a massively
> expensive operation.

They are not CPU idling on our idle path, they are dropping into suspend
to RAM, that means they need to drop the devices into S2R states as well.

And yes the paths are currently expensive in places but they need to be
sped up more not messed up. For most devices it's not going to be that
hard to sort out - the big ugly is x86 patching the instruction stream
back and forth each suspend/resume which is in "durrr..." category of
smartness.

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Nicolas Pitre]

On Thu, 24 Feb 2011, Peter Zijlstra wrote:

> On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:
> 
> > Most SMP ARM processors are going to use it soon.  Powering down idle 
> > cores provides substantial power saving.
> 
> And why can't regular idle paths be used? CPU hotplug is a massively
> expensive operation.

The idle path assumes that the CPU state is preserved.  We're talking 
about cores completely going down with power pulled beneath them and 
eventually rebooted dynamically here.


Nicolas

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Peter Zijlstra]

On Thu, 2011-02-24 at 15:40 -0500, Nicolas Pitre wrote:
> On Thu, 24 Feb 2011, Peter Zijlstra wrote:
> 
> > On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:
> > 
> > > Most SMP ARM processors are going to use it soon.  Powering down idle 
> > > cores provides substantial power saving.
> > 
> > And why can't regular idle paths be used? CPU hotplug is a massively
> > expensive operation.
> 
> The idle path assumes that the CPU state is preserved. 

Not much of it,

>  We're talking 
> about cores completely going down with power pulled beneath them and 
> eventually rebooted dynamically here.

I think you can get away with actually pulling the power and re-initing
the cpu to idle from the idle path and the rest of the kernel not
caring.

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

On Thu, 24 Feb 2011, Nicolas Pitre wrote:

> On Thu, 24 Feb 2011, Peter Zijlstra wrote:
> 
> > On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:
> > 
> > > Most SMP ARM processors are going to use it soon.  Powering down idle 
> > > cores provides substantial power saving.
> > 
> > And why can't regular idle paths be used? CPU hotplug is a massively
> > expensive operation.
> 
> The idle path assumes that the CPU state is preserved.  We're talking 
> about cores completely going down with power pulled beneath them and 
> eventually rebooted dynamically here.

That's the equivalent of physical hotplug, but we still have all the
memory state around, so it is possible from idle, when we have the
full isolation features in place.

Thanks,

	tglx

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Alan Cox]

> That's the equivalent of physical hotplug, but we still have all the
> memory state around, so it is possible from idle, when we have the

All sorts of other state goes walking as well though - on die device
state for example like the GPU context may well also evaporate or
disappear except for the minimum needed for scanout.

It's not just about the CPU its a system (albeit a SoC in most cases)
going to sleep lock stock and barrel except for IRQ notifications, and in
some cases things like offloaded audio playback.

Alan

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

On Thu, 24 Feb 2011, Alan Cox wrote:

> > That's the equivalent of physical hotplug, but we still have all the
> > memory state around, so it is possible from idle, when we have the
> 
> All sorts of other state goes walking as well though - on die device
> state for example like the GPU context may well also evaporate or
> disappear except for the minimum needed for scanout.
>
> It's not just about the CPU its a system (albeit a SoC in most cases)
> going to sleep lock stock and barrel except for IRQ notifications, and in
> some cases things like offloaded audio playback.

I guess we are talking about different things.

If you take down one core of a package then it does not take down the
whole SoC and does not take down devices either. Device take down is
or should be handled by explicit clock and power gating.

If you take down the whole SoC, then it's full S2RAM or some
equivilant thing. That's a different story and propably requires the
whole hotplug muck.

Thanks,

	tglx

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Peter Zijlstra]

On Thu, 2011-02-24 at 22:12 +0100, Thomas Gleixner wrote:

> If you take down the whole SoC, then it's full S2RAM or some
> equivilant thing. That's a different story and propably requires the
> whole hotplug muck.

Didn't we, during the whole wakelock trainwreck, that s2ram could be
done from idle as well (for hardware where the whole opportunistic
suspend was sensible to begin with)?

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

On Thu, 24 Feb 2011, Peter Zijlstra wrote:

> On Thu, 2011-02-24 at 22:12 +0100, Thomas Gleixner wrote:
> 
> > If you take down the whole SoC, then it's full S2RAM or some
> > equivilant thing. That's a different story and propably requires the
> > whole hotplug muck.
> 
> Didn't we, during the whole wakelock trainwreck, that s2ram could be
> done from idle as well (for hardware where the whole opportunistic
> suspend was sensible to begin with)?

Yes, needs some more changes to the whole infrastructure of clocks and
power gates, but it's doable.

Thanks,

	tglx

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

On Thu, 24 Feb 2011, Peter Zijlstra wrote:

> On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:
> 
> > Most SMP ARM processors are going to use it soon.  Powering down idle 
> > cores provides substantial power saving.
> 
> And why can't regular idle paths be used? CPU hotplug is a massively
> expensive operation.

To achieve the same result from idle, you need to exclude the core
from any unwanted wakeup. At the moment cpu unplug is the only way to
achieve that.

If you want to do the same from idle, then we need the isolation
features Frederic is working on for RT/HPC.

They allow us to isolate cores completely for totaly different
reasons, but it could be resused to provide full isolation of a core
in a very deep power state.

That would solve the problem w/o going through kstompmachine

Thanks,

	tglx

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Peter Zijlstra]

On Thu, 2011-02-24 at 21:47 +0100, Thomas Gleixner wrote:
> On Thu, 24 Feb 2011, Peter Zijlstra wrote:
> 
> > On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:
> > 
> > > Most SMP ARM processors are going to use it soon.  Powering down idle 
> > > cores provides substantial power saving.
> > 
> > And why can't regular idle paths be used? CPU hotplug is a massively
> > expensive operation.
> 
> To achieve the same result from idle, you need to exclude the core
> from any unwanted wakeup. At the moment cpu unplug is the only way to
> achieve that.

Right, everything is a nail because all we have is a hammer like.

> If you want to do the same from idle, then we need the isolation
> features Frederic is working on for RT/HPC.
> 
> They allow us to isolate cores completely for totaly different
> reasons, but it could be resused to provide full isolation of a core
> in a very deep power state.

Exactly.

> That would solve the problem w/o going through kstompmachine

Right, kstopmachine is a large part of the problem, but cpu hotplug
really does an insane amount of work if all you want is to idle the
core.

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Thomas Gleixner]

On Thu, 24 Feb 2011, Peter Zijlstra wrote:

> On Thu, 2011-02-24 at 21:47 +0100, Thomas Gleixner wrote:
> > On Thu, 24 Feb 2011, Peter Zijlstra wrote:
> > 
> > > On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:
> > > 
> > > > Most SMP ARM processors are going to use it soon.  Powering down idle 
> > > > cores provides substantial power saving.
> > > 
> > > And why can't regular idle paths be used? CPU hotplug is a massively
> > > expensive operation.
> > 
> > To achieve the same result from idle, you need to exclude the core
> > from any unwanted wakeup. At the moment cpu unplug is the only way to
> > achieve that.
> 
> Right, everything is a nail because all we have is a hammer like.
> 
> > If you want to do the same from idle, then we need the isolation
> > features Frederic is working on for RT/HPC.
> > 
> > They allow us to isolate cores completely for totaly different
> > reasons, but it could be resused to provide full isolation of a core
> > in a very deep power state.
> 
> Exactly.
> 
> > That would solve the problem w/o going through kstompmachine
> 
> Right, kstopmachine is a large part of the problem, but cpu hotplug
> really does an insane amount of work if all you want is to idle the
> core.

As far as I can tell the isolation stuff covers lot's of the problems
including RCU, but there is still a way to go. And the good new is
that, when a cpu is idle it has not much state to preserve if its
properly isolated and the memory is not going away.

Thanks,

	tglx

 

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Paul E. McKenney]

On Thu, Feb 24, 2011 at 09:58:32PM +0100, Peter Zijlstra wrote:
> On Thu, 2011-02-24 at 21:47 +0100, Thomas Gleixner wrote:
> > On Thu, 24 Feb 2011, Peter Zijlstra wrote:
> > 
> > > On Thu, 2011-02-24 at 15:24 -0500, Nicolas Pitre wrote:
> > > 
> > > > Most SMP ARM processors are going to use it soon.  Powering down idle 
> > > > cores provides substantial power saving.
> > > 
> > > And why can't regular idle paths be used? CPU hotplug is a massively
> > > expensive operation.
> > 
> > To achieve the same result from idle, you need to exclude the core
> > from any unwanted wakeup. At the moment cpu unplug is the only way to
> > achieve that.
> 
> Right, everything is a nail because all we have is a hammer like.
> 
> > If you want to do the same from idle, then we need the isolation
> > features Frederic is working on for RT/HPC.
> > 
> > They allow us to isolate cores completely for totaly different
> > reasons, but it could be resused to provide full isolation of a core
> > in a very deep power state.
> 
> Exactly.
> 
> > That would solve the problem w/o going through kstompmachine
> 
> Right, kstopmachine is a large part of the problem, but cpu hotplug
> really does an insane amount of work if all you want is to idle the
> core.

You do indeed need to know that the CPU will be powered off for quite
some time to be worth the extra work.  And a number of embedded devices
do have this level of foreknowledge -- for example, they might use the
second CPU only when the user is doing some computationally intensive
task, so that the device would normally be using only a single CPU.

							Thanx, Paul

Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events

[Peter Zijlstra]

On Thu, 2011-02-24 at 20:11 +0000, Alan Cox wrote:
> > Anybody who is interested in the latency of cpu hotplug is deluding
> > himself, also cpu hotplug is _NOT_ a power management feature, so the
> > rest of your justification just disappeared as well.
> 
> Actually CPU hotplug is a power management feature on some devices where
> you need to shutdown one of the cores to enter low power modes.

Aren't we confusing things here? Surely simply idling a core is good
enough? Why would we want to go through the whole CPU hotplug dance
simply to enter a low power state?

> Remember we use it as part of the suspend paths and various processors
> nowdays drop into a suspend to RAM type state on CPU idling.

Which would illustrate the above point. CPU hotplug is a terribly
expensive op, and doing so from idle is really utterly ridiculous (nor
can we, idle is not supposed to schedule and cpu-hotplug needs to
schedule)

Why can't we do these things from the normal idle path, presumably these
state transitions are 'fast', so we can implement them as normal idle
modes. 

The scheduler has (due to power7 support) the ability to favour lower
cpu nrs when placing tasks, so idle !bsp (assuming cpu0 is the bsp) can
drop into their special state, and then when the bsp goes idle it can do
whatever it needs to do.

All that needs is to make sure smp_send_reschedule() can wake !bsp cores
from their special sleep state, but that's all arch code anyway.

I really see no reason to conflate cpu-hotplug and idle/power-states.