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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Thu, 24 Feb 2011 17:15:21 +0100
From:	Vincent Guittot <vincent.guittot@...aro.org>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-hotplug@...r.kernel.org,
	rostedt@...dmis.org, amit.kucheria@...aro.org,
	Rusty Russell <rusty@...tcorp.com.au>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH V4 2/2] tracing, perf : add cpu hotplug trace events

On 24 February 2011 16:00, Frederic Weisbecker <fweisbec@...il.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@...aro.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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/