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

[PATCH V4 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 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..692e819 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);
@@ -274,6 +282,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 +295,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 +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));
@@ -335,6 +349,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 +392,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 V4 2/2] tracing, perf : add cpu hotplug trace events

[Frederic Weisbecker]

On Mon, Jan 31, 2011 at 11:23: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 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..692e819 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);
> @@ -274,6 +282,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 +295,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 +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));
> @@ -335,6 +349,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 +392,8 @@ int __cpuinit cpu_up(unsigned int cpu)
> 
>  	err = _cpu_up(cpu, 0);
> 
> +	trace_cpu_hotplug_up_end(cpu);

You should probably have this call after cpu_maps_update_done(),
because you put the start before the mutex is locked.
Just to stay symetric with lock events.

In fact I think it may be better not to include the hotplug lock/unlock
in the cpu down/up tracing, but trace start once it is locked and trace
stop before we release it.

It's just that I think you're not interested in including cpu_add_remove_lock
mutex contention in cpu hotplug traces. That's rather something to be measured
with lock events if needed.

It's a detail, for the rest I'm fine the patches. As Steve said though, it would
be nice to get feedback from cpu hotplug maintainers (who I'm adding in Cc
here again).

Thanks.


> +
>  out:
>  	cpu_maps_update_done();
>  	return err;
> -- 
> 1.7.1

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

[Vincent Guittot]

On 24 February 2011 16:00, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Mon, Jan 31, 2011 at 11:23: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 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..692e819 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);
>> @@ -274,6 +282,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 +295,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 +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));
>> @@ -335,6 +349,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 +392,8 @@ int __cpuinit cpu_up(unsigned int cpu)
>>
>>       err = _cpu_up(cpu, 0);
>>
>> +     trace_cpu_hotplug_up_end(cpu);
>
> You should probably have this call after cpu_maps_update_done(),
> because you put the start before the mutex is locked.
> Just to stay symetric with lock events.
>
> In fact I think it may be better not to include the hotplug lock/unlock
> in the cpu down/up tracing, but trace start once it is locked and trace
> stop before we release it.
>
> It's just that I think you're not interested in including cpu_add_remove_lock
> mutex contention in cpu hotplug traces. That's rather something to be measured
> with lock events if needed.
>

you're right, it's not symetric, I'm going to correct that point and
exclude the mutex from the trace

> It's a detail, for the rest I'm fine the patches. As Steve said though, it would
> be nice to get feedback from cpu hotplug maintainers (who I'm adding in Cc
> here again).
>

Thanks for your comments

> Thanks.
>
>
>> +
>>  out:
>>       cpu_maps_update_done();
>>       return err;
>> --
>> 1.7.1
>