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@xxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
