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

[PATCH 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 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 1/2] add cpu hotplug tracepoints

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 |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 156cc55..f04e9cf 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);
@@ -200,7 +203,9 @@ static int __ref take_cpu_down(void *_param)
 	int err;

 	/* Ensure this CPU doesn't handle any more interrupts. */
+	trace_cpu_hotplug_arch_disable_start((unsigned int)(param->hcpu));
 	err = __cpu_disable();
+	trace_cpu_hotplug_arch_disable_end((unsigned int)(param->hcpu));
 	if (err < 0)
 		return err;

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

 	/* This actually kills the CPU. */
+	trace_cpu_hotplug_arch_die_start(cpu);
 	__cpu_die(cpu);
+	trace_cpu_hotplug_arch_die_end(cpu);

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

+	trace_cpu_hotplug_down_start(cpu);
+
 	cpu_maps_update_begin();

 	if (cpu_hotplug_disabled) {
@@ -285,6 +294,8 @@ int __ref cpu_down(unsigned int cpu)

 out:
 	cpu_maps_update_done();
+
+	trace_cpu_hotplug_down_end(cpu);
 	return err;
 }
 EXPORT_SYMBOL(cpu_down);
@@ -310,7 +321,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));
@@ -335,6 +348,8 @@ int __cpuinit cpu_up(unsigned int cpu)
 	pg_data_t	*pgdat;
 #endif

+	trace_cpu_hotplug_up_start(cpu);
+
 	if (!cpu_possible(cpu)) {
 		printk(KERN_ERR "can't online cpu %d because it is not "
 			"configured as may-hotadd at boot time\n", cpu);
@@ -376,6 +391,8 @@ int __cpuinit cpu_up(unsigned int cpu)

 	err = _cpu_up(cpu, 0);

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

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

[Frederic Weisbecker]

On Tue, Jan 25, 2011 at 09:54:59AM +0100, Vincent Guittot wrote:
> Please find below a new proposal for adding trace events for cpu hotplug.
> 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 1/2] add cpu hotplug tracepoints
> 
> 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 |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 156cc55..f04e9cf 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);
> @@ -200,7 +203,9 @@ static int __ref take_cpu_down(void *_param)
>  	int err;
> 
>  	/* Ensure this CPU doesn't handle any more interrupts. */
> +	trace_cpu_hotplug_arch_disable_start((unsigned int)(param->hcpu));
>  	err = __cpu_disable();
> +	trace_cpu_hotplug_arch_disable_end((unsigned int)(param->hcpu));

You should create a cpu var instead of doing that not very beautiful cast
twice.

Probably we should eventually rename arch_disable into just disable.

And same for die, because otherwise the name is quite long.

Other than that, looks all good to me.

Thanks.

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

[Vincent Guittot]

On 26 January 2011 18:32, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Tue, Jan 25, 2011 at 09:54:59AM +0100, Vincent Guittot wrote:
>> Please find below a new proposal for adding trace events for cpu hotplug.
>> 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 1/2] add cpu hotplug tracepoints
>>
>> 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 |   17 +++++++++++++++++
>>  1 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 156cc55..f04e9cf 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);
>> @@ -200,7 +203,9 @@ static int __ref take_cpu_down(void *_param)
>>       int err;
>>
>>       /* Ensure this CPU doesn't handle any more interrupts. */
>> +     trace_cpu_hotplug_arch_disable_start((unsigned int)(param->hcpu));
>>       err = __cpu_disable();
>> +     trace_cpu_hotplug_arch_disable_end((unsigned int)(param->hcpu));
>
> You should create a cpu var instead of doing that not very beautiful cast
> twice.
>

OK

> Probably we should eventually rename arch_disable into just disable.
>
> And same for die, because otherwise the name is quite long.
>

If we remove the arch extension of these names, we will have
trace_cpu_hotplug_arch_up for __cpu_up (trace_cpu_hotplug_up is
already used) and trace_cpu_hotplug_disable for __cpu_disable. would
that be acceptable ?

> Other than that, looks all good to me.
>
> Thanks.
>

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

[Frederic Weisbecker]

On Thu, Jan 27, 2011 at 11:04:09AM +0100, Vincent Guittot wrote:
> On 26 January 2011 18:32, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Tue, Jan 25, 2011 at 09:54:59AM +0100, Vincent Guittot wrote:
> >> Please find below a new proposal for adding trace events for cpu hotplug.
> >> 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 1/2] add cpu hotplug tracepoints
> >>
> >> 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 |   17 +++++++++++++++++
> >>  1 files changed, 17 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/kernel/cpu.c b/kernel/cpu.c
> >> index 156cc55..f04e9cf 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);
> >> @@ -200,7 +203,9 @@ static int __ref take_cpu_down(void *_param)
> >>       int err;
> >>
> >>       /* Ensure this CPU doesn't handle any more interrupts. */
> >> +     trace_cpu_hotplug_arch_disable_start((unsigned int)(param->hcpu));
> >>       err = __cpu_disable();
> >> +     trace_cpu_hotplug_arch_disable_end((unsigned int)(param->hcpu));
> >
> > You should create a cpu var instead of doing that not very beautiful cast
> > twice.
> >
> 
> OK
> 
> > Probably we should eventually rename arch_disable into just disable.
> >
> > And same for die, because otherwise the name is quite long.
> >
> 
> If we remove the arch extension of these names, we will have
> trace_cpu_hotplug_arch_up for __cpu_up (trace_cpu_hotplug_up is
> already used) and trace_cpu_hotplug_disable for __cpu_disable. would
> that be acceptable ?

You're right.

Well, we could rename cpu_hotplug_arch_up into cpu_hotplug_enable,
but the overall result makes it confusing.

Let's keep it that way.