Re: [PATCH 1/4] ksym_tracer: Fix to make the tracer work

[Li Zefan <lizf@cn.fujitsu.com>]

K.Prasad wrote:
> On Wed, Dec 30, 2009 at 02:22:22PM +0800, Li Zefan wrote:
>> ksym tracer doesn't work:
>>
>>  # echo tasklist_lock:rw- > ksym_trace_filter
>>  -bash: echo: write error: No such device
>>
>> It's because we pass to perf_event_create_kernel_counter()
>> a cpu number which is not present.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  kernel/hw_breakpoint.c    |   10 +++++++---
>>  kernel/trace/trace_ksym.c |    1 -
>>  2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
>> index dbcbf6a..50dbd59 100644
>> --- a/kernel/hw_breakpoint.c
>> +++ b/kernel/hw_breakpoint.c
>> @@ -40,6 +40,7 @@
>>  #include <linux/percpu.h>
>>  #include <linux/sched.h>
>>  #include <linux/init.h>
>> +#include <linux/cpu.h>
>>  #include <linux/smp.h>
>>
>>  #include <linux/hw_breakpoint.h>
>> @@ -388,7 +389,8 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
>>  	if (!cpu_events)
>>  		return ERR_PTR(-ENOMEM);
>>
>> -	for_each_possible_cpu(cpu) {
>> +	get_online_cpus();
>> +	for_each_online_cpu(cpu) {
>>  		pevent = per_cpu_ptr(cpu_events, cpu);
>>  		bp = perf_event_create_kernel_counter(attr, cpu, -1, triggered);
>>
> 
> Frederic must have used for_each_possible_cpu() to account for CPUs that
> are offline at the time of registration, but may eventually turn online.
> Since register_wide_hw_breakpoint() interface is designed to deliver
> system-wide breakpoints, the debug registers of a new online CPU will
> should have the breakpoints populated to comprehensively notify all
> memory accesses over target address.
> 
> I'd rather wait to hear from Frederic to know why
> perf_event_create_kernel_counter() returns an error when run for an
> offline cpu and how it can be solved.
> 

See the comment in find_get_context() in kernel/perf_event.c:

                /*
                 * We could be clever and allow to attach a event to an
                 * offline CPU and activate it when the CPU comes up, but
                 * that's for later.
                 */
                if (!cpu_online(cpu))
                        return ERR_PTR(-ENODEV);

So I think we can use for_each_possible_cpu() in the future, but not now.