| Message ID | 1441630315-189525-1-git-send-email-wangnan0@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 07, 2015 at 12:51:55PM +0000, Wang Nan wrote: > Commit e1e499aba570a2ea84d29822b7ea637ac41d9a51 (perf tools: Add > processor socket info to hist_entry and addr_location) reads env->cpu > array for each sample using index al.cpu. However, al.cpu can be -1 if > sample doesn't select PERF_SAMPLE_CPU. Also, env->cpu can be invalid if > feature CPU_TOPOLOGY not selected. We should validate env->cpu and al.cpu > before setting al.socket. > > Signed-off-by: Wang Nan <wangnan0@huawei.com> > Cc: Kan Liang <kan.liang@intel.com> > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Andi Kleen <ak@linux.intel.com> > Cc: Jiri Olsa <jolsa@kernel.org> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Stephane Eranian <eranian@google.com> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > --- > > Although theoretically CPU_TOPOLOGY feature should always be selected by > 'perf record', I did generate a perf.data without that feature. It has > header like this: > > # perf report -i ./bad.perf.data --header-only > # ======== > # captured on: Thu Jan 8 09:30:15 2009 > # hostname : localhost > # os release : 3.10.49-gd672fc4 > # perf version : 4.2.gc9df > # arch : aarch64 > # nrcpus online : 8 > # nrcpus avail : 8 > # total memory : 1850768 kB > # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5 > # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1 > # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1 > # pmu mappings: cpu = 4, software = 1, tracepoint = 2 > # ======== > # > > It should be: > > # ======== > # captured on: Thu Jan 8 11:26:41 2009 > ... > # HEADER_CPU_TOPOLOGY info available, use -I to display > # pmu mappings: cpu = 4, software = 1, tracepoint = 2 > # ======== > > However, bad perf.data appears randomly. I can't stably reproduce it, so I > guess there might have another invalid memory accessing. > > --- > tools/perf/builtin-report.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c > index 4b43245..16d097d 100644 > --- a/tools/perf/builtin-report.c > +++ b/tools/perf/builtin-report.c > @@ -158,8 +158,16 @@ static int process_sample_event(struct perf_tool *tool, > return -1; > } > > - /* read socket id from perf.data for perf report */ > - al.socket = env->cpu[al.cpu].socket_id; > + /* > + * read socket id from perf.data for perf report > + * al.cpu is invalid if PERF_SAMPLE_CPU is not selected by this > + * sample. > + * env->cpu is invalid if CPU_TOPOLOGY feature is not set in > + * header. > + */ > + al.socket = -1; > + if (env->cpu && al.cpu >= 0) > + al.socket = env->cpu[al.cpu].socket_id; perf_event__preprocess_sample initializes al.socket from current system do we want to move this over there? also this change is just report specific, and we could need this in at least perf top jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 2015/9/7 21:03, Jiri Olsa wrote: > On Mon, Sep 07, 2015 at 12:51:55PM +0000, Wang Nan wrote: >> Commit e1e499aba570a2ea84d29822b7ea637ac41d9a51 (perf tools: Add >> processor socket info to hist_entry and addr_location) reads env->cpu >> array for each sample using index al.cpu. However, al.cpu can be -1 if >> sample doesn't select PERF_SAMPLE_CPU. Also, env->cpu can be invalid if >> feature CPU_TOPOLOGY not selected. We should validate env->cpu and al.cpu >> before setting al.socket. >> >> Signed-off-by: Wang Nan <wangnan0@huawei.com> >> Cc: Kan Liang <kan.liang@intel.com> >> Cc: Adrian Hunter <adrian.hunter@intel.com> >> Cc: Andi Kleen <ak@linux.intel.com> >> Cc: Jiri Olsa <jolsa@kernel.org> >> Cc: Namhyung Kim <namhyung@kernel.org> >> Cc: Stephane Eranian <eranian@google.com> >> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> >> --- >> >> Although theoretically CPU_TOPOLOGY feature should always be selected by >> 'perf record', I did generate a perf.data without that feature. It has >> header like this: >> >> # perf report -i ./bad.perf.data --header-only >> # ======== >> # captured on: Thu Jan 8 09:30:15 2009 >> # hostname : localhost >> # os release : 3.10.49-gd672fc4 >> # perf version : 4.2.gc9df >> # arch : aarch64 >> # nrcpus online : 8 >> # nrcpus avail : 8 >> # total memory : 1850768 kB >> # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5 >> # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1 >> # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1 >> # pmu mappings: cpu = 4, software = 1, tracepoint = 2 >> # ======== >> # >> >> It should be: >> >> # ======== >> # captured on: Thu Jan 8 11:26:41 2009 >> ... >> # HEADER_CPU_TOPOLOGY info available, use -I to display >> # pmu mappings: cpu = 4, software = 1, tracepoint = 2 >> # ======== >> >> However, bad perf.data appears randomly. I can't stably reproduce it, so I >> guess there might have another invalid memory accessing. >> >> --- >> tools/perf/builtin-report.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c >> index 4b43245..16d097d 100644 >> --- a/tools/perf/builtin-report.c >> +++ b/tools/perf/builtin-report.c >> @@ -158,8 +158,16 @@ static int process_sample_event(struct perf_tool *tool, >> return -1; >> } >> >> - /* read socket id from perf.data for perf report */ >> - al.socket = env->cpu[al.cpu].socket_id; >> + /* >> + * read socket id from perf.data for perf report >> + * al.cpu is invalid if PERF_SAMPLE_CPU is not selected by this >> + * sample. >> + * env->cpu is invalid if CPU_TOPOLOGY feature is not set in >> + * header. >> + */ >> + al.socket = -1; >> + if (env->cpu && al.cpu >= 0) >> + al.socket = env->cpu[al.cpu].socket_id; > perf_event__preprocess_sample initializes al.socket from current system No. For 'perf report' it initializes al.cpu from sample. Commit message of e1e499aba570a2ea84d29822b7ea637ac41d9a51: Finor 'perf report', the socket id info is from perf.data. For others, the socket id info is from current system. And at least checking of env->cpu is essential. I'm looking the problem I reported. Looks like build_cpu_topology() is possible to fail. Thank you. > do we want to move this over there? > > also this change is just report specific, and we could need > this in at least perf top > > jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 2015/9/7 20:51, Wang Nan wrote: [SNIP] > Although theoretically CPU_TOPOLOGY feature should always be selected by > 'perf record', I did generate a perf.data without that feature. It has > header like this: > > # perf report -i ./bad.perf.data --header-only > # ======== > # captured on: Thu Jan 8 09:30:15 2009 > # hostname : localhost > # os release : 3.10.49-gd672fc4 > # perf version : 4.2.gc9df > # arch : aarch64 > # nrcpus online : 8 > # nrcpus avail : 8 > # total memory : 1850768 kB > # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5 > # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1 > # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1 > # pmu mappings: cpu = 4, software = 1, tracepoint = 2 > # ======== > # > > It should be: > > # ======== > # captured on: Thu Jan 8 11:26:41 2009 > ... > # HEADER_CPU_TOPOLOGY info available, use -I to display > # pmu mappings: cpu = 4, software = 1, tracepoint = 2 > # ======== > > However, bad perf.data appears randomly. I can't stably reproduce it, so I > guess there might have another invalid memory accessing. > > I found the problem. perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. It depend on the existance of /sys/devices/system/cpu/cpu%d/topology/core_siblings_list However, CPU can be canceled by hotcpu subsystem. After that the directory of /sys/devices/system/cpu/cpu%d/topology is gone, which causes perf's write_cpu_topology() --> uild_cpu_topology() to fail, result in the above perf.data. So I think my patch is required. Thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 2015/9/8 15:37, Jiri Olsa wrote: > On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote: > > SNIP > >> I found the problem. >> >> perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. It >> depend on >> the existance of >> >> /sys/devices/system/cpu/cpu%d/topology/core_siblings_list >> >> However, CPU can be canceled by hotcpu subsystem. After that the directory >> of >> /sys/devices/system/cpu/cpu%d/topology is gone, which causes perf's >> write_cpu_topology() --> uild_cpu_topology() to fail, result in the above >> perf.data. >> >> So I think my patch is required. > no question there.. I just meant it should be placed in > perf_event__preprocess_sample function with the rest of > the 'al' initialization, like in the patch below? > > it does not compile, because there're many places calling > it and it'd need changing all callers to pass env, which > seems to require more changes.. > > also I'm not sure about removing: > - al->socket = cpu_map__get_socket_id(al->cpu); > > > Does any command actually need this initialized from current system? > > thanks, > jirka > > > --- > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c > index 0bf8c9889fc0..3339d2579bfc 100644 > --- a/tools/perf/util/event.c > +++ b/tools/perf/util/event.c > @@ -990,7 +990,8 @@ void thread__find_addr_location(struct thread *thread, > int perf_event__preprocess_sample(const union perf_event *event, > struct machine *machine, > struct addr_location *al, > - struct perf_sample *sample) > + struct perf_sample *sample, > + struct perf_env *env) > { > u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK; > struct thread *thread = machine__findnew_thread(machine, sample->pid, > @@ -1021,7 +1022,10 @@ int perf_event__preprocess_sample(const union perf_event *event, > > al->sym = NULL; > al->cpu = sample->cpu; > - al->socket = cpu_map__get_socket_id(al->cpu); > + > + al.socket = -1; > + if (env->cpu && al->cpu >= 0) > + al.socket = env->cpu[al->cpu].socket_id; > > if (al->map) { > struct dso *dso = al->map->dso; Now I understand your suggestion. You mean we can build env->cpu before processing the first sample, then init al.socket using that map instead of calling cpu_map__get_socket_id() unconditionally in an ad-hoc way. And I have another question that, since build_cpu_topo() and perf_event__preprocess_sample() are more or less doing similar things, why we need both of them? Then we need more code for this bug... Kan Liang, do you have any suggestion? Thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Sep 08, 2015 at 04:12:55PM +0800, Wangnan (F) wrote: > > > On 2015/9/8 15:37, Jiri Olsa wrote: > >On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote: > > > >SNIP > > > >>I found the problem. > >> > >>perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. It > >>depend on > >>the existance of > >> > >>/sys/devices/system/cpu/cpu%d/topology/core_siblings_list > >> > >>However, CPU can be canceled by hotcpu subsystem. After that the directory > >>of > >>/sys/devices/system/cpu/cpu%d/topology is gone, which causes perf's > >>write_cpu_topology() --> uild_cpu_topology() to fail, result in the above > >>perf.data. > >> > >>So I think my patch is required. > >no question there.. I just meant it should be placed in > >perf_event__preprocess_sample function with the rest of > >the 'al' initialization, like in the patch below? > > > >it does not compile, because there're many places calling > >it and it'd need changing all callers to pass env, which > >seems to require more changes.. > > > >also I'm not sure about removing: > >- al->socket = cpu_map__get_socket_id(al->cpu); > > > > > >Does any command actually need this initialized from current system? > > > >thanks, > >jirka > > > > > >--- > >diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c > >index 0bf8c9889fc0..3339d2579bfc 100644 > >--- a/tools/perf/util/event.c > >+++ b/tools/perf/util/event.c > >@@ -990,7 +990,8 @@ void thread__find_addr_location(struct thread *thread, > > int perf_event__preprocess_sample(const union perf_event *event, > > struct machine *machine, > > struct addr_location *al, > >- struct perf_sample *sample) > >+ struct perf_sample *sample, > >+ struct perf_env *env) > > { > > u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK; > > struct thread *thread = machine__findnew_thread(machine, sample->pid, > >@@ -1021,7 +1022,10 @@ int perf_event__preprocess_sample(const union perf_event *event, > > al->sym = NULL; > > al->cpu = sample->cpu; > >- al->socket = cpu_map__get_socket_id(al->cpu); > >+ > >+ al.socket = -1; > >+ if (env->cpu && al->cpu >= 0) > >+ al.socket = env->cpu[al->cpu].socket_id; > > if (al->map) { > > struct dso *dso = al->map->dso; > > Now I understand your suggestion. You mean we can build env->cpu before > processing the first sample, then init al.socket using that map instead hum, that should be the case anyway.. features are read before events > of calling cpu_map__get_socket_id() unconditionally in an ad-hoc way. > > And I have another question that, since build_cpu_topo() and > perf_event__preprocess_sample() are more or less doing similar things, > why we need both of them? perf_event__preprocess_sample is called for each sample, while build_cpu_topo is part of storing topology feature jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
发自我的 iPhone > 在 2015年9月8日，下午9:13，Jiri Olsa <jolsa@redhat.com> 写道： > >> On Tue, Sep 08, 2015 at 04:12:55PM +0800, Wangnan (F) wrote: >> >> >>> On 2015/9/8 15:37, Jiri Olsa wrote: >>> On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote: >>> >>> SNIP >>> >>>> I found the problem. >>>> >>>> perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. It >>>> depend on >>>> the existance of >>>> >>>> /sys/devices/system/cpu/cpu%d/topology/core_siblings_list >>>> >>>> However, CPU can be canceled by hotcpu subsystem. After that the directory >>>> of >>>> /sys/devices/system/cpu/cpu%d/topology is gone, which causes perf's >>>> write_cpu_topology() --> uild_cpu_topology() to fail, result in the above >>>> perf.data. >>>> >>>> So I think my patch is required. >>> no question there.. I just meant it should be placed in >>> perf_event__preprocess_sample function with the rest of >>> the 'al' initialization, like in the patch below? >>> >>> it does not compile, because there're many places calling >>> it and it'd need changing all callers to pass env, which >>> seems to require more changes.. >>> >>> also I'm not sure about removing: >>> - al->socket = cpu_map__get_socket_id(al->cpu); >>> >>> >>> Does any command actually need this initialized from current system? >>> >>> thanks, >>> jirka >>> >>> >>> --- >>> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c >>> index 0bf8c9889fc0..3339d2579bfc 100644 >>> --- a/tools/perf/util/event.c >>> +++ b/tools/perf/util/event.c >>> @@ -990,7 +990,8 @@ void thread__find_addr_location(struct thread *thread, >>> int perf_event__preprocess_sample(const union perf_event *event, >>> struct machine *machine, >>> struct addr_location *al, >>> - struct perf_sample *sample) >>> + struct perf_sample *sample, >>> + struct perf_env *env) >>> { >>> u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK; >>> struct thread *thread = machine__findnew_thread(machine, sample->pid, >>> @@ -1021,7 +1022,10 @@ int perf_event__preprocess_sample(const union perf_event *event, >>> al->sym = NULL; >>> al->cpu = sample->cpu; >>> - al->socket = cpu_map__get_socket_id(al->cpu); >>> + >>> + al.socket = -1; >>> + if (env->cpu && al->cpu >= 0) >>> + al.socket = env->cpu[al->cpu].socket_id; >>> if (al->map) { >>> struct dso *dso = al->map->dso; >> >> Now I understand your suggestion. You mean we can build env->cpu before >> processing the first sample, then init al.socket using that map instead > > hum, that should be the case anyway.. features are read before events > >> of calling cpu_map__get_socket_id() unconditionally in an ad-hoc way. >> >> And I have another question that, since build_cpu_topo() and >> perf_event__preprocess_sample() are more or less doing similar things, >> why we need both of them? > > perf_event__preprocess_sample is called for each sample, > while build_cpu_topo is part of storing topology feature Sorry, what I wanted to say should be: cpu_map__get_socket_id() and build_cpu_topo()... > > jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Sep 08, 2015 at 09:16:31PM +0800, pi3orama wrote: SNIP > >> Now I understand your suggestion. You mean we can build env->cpu before > >> processing the first sample, then init al.socket using that map instead > > > > hum, that should be the case anyway.. features are read before events > > > >> of calling cpu_map__get_socket_id() unconditionally in an ad-hoc way. > >> > >> And I have another question that, since build_cpu_topo() and > >> perf_event__preprocess_sample() are more or less doing similar things, > >> why we need both of them? > > > > perf_event__preprocess_sample is called for each sample, > > while build_cpu_topo is part of storing topology feature > Sorry, what I wanted to say should be: > cpu_map__get_socket_id() and build_cpu_topo()... cpu_map__get_socket_id is also used by perf stat jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> > On 2015/9/8 15:37, Jiri Olsa wrote: > > On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote: > > > > SNIP > > > >> I found the problem. > >> > >> perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. > >> It depend on the existance of > >> > >> /sys/devices/system/cpu/cpu%d/topology/core_siblings_list > >> > >> However, CPU can be canceled by hotcpu subsystem. After that the > >> directory of /sys/devices/system/cpu/cpu%d/topology is gone, which > >> causes perf's > >> write_cpu_topology() --> uild_cpu_topology() to fail, result in the > >> above perf.data. > >> > >> So I think my patch is required. > > no question there.. I just meant it should be placed in > > perf_event__preprocess_sample function with the rest of the 'al' > > initialization, like in the patch below? > > > > it does not compile, because there're many places calling it and it'd > > need changing all callers to pass env, which seems to require more > > changes.. > > > > also I'm not sure about removing: > > - al->socket = cpu_map__get_socket_id(al->cpu); > > > > > > Does any command actually need this initialized from current system? > > > > thanks, > > jirka > > > > > > --- > > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index > > 0bf8c9889fc0..3339d2579bfc 100644 > > --- a/tools/perf/util/event.c > > +++ b/tools/perf/util/event.c > > @@ -990,7 +990,8 @@ void thread__find_addr_location(struct thread > *thread, > > int perf_event__preprocess_sample(const union perf_event *event, > > struct machine *machine, > > struct addr_location *al, > > - struct perf_sample *sample) > > + struct perf_sample *sample, > > + struct perf_env *env) > > { > > u8 cpumode = event->header.misc & > PERF_RECORD_MISC_CPUMODE_MASK; > > struct thread *thread = machine__findnew_thread(machine, > > sample->pid, @@ -1021,7 +1022,10 @@ int > > perf_event__preprocess_sample(const union perf_event *event, > > > > al->sym = NULL; > > al->cpu = sample->cpu; > > - al->socket = cpu_map__get_socket_id(al->cpu); > > + > > + al.socket = -1; > > + if (env->cpu && al->cpu >= 0) > > + al.socket = env->cpu[al->cpu].socket_id; > > > > if (al->map) { > > struct dso *dso = al->map->dso; > > Now I understand your suggestion. You mean we can build env->cpu > before processing the first sample, then init al.socket using that map > instead of calling cpu_map__get_socket_id() unconditionally in an ad-hoc > way. > > And I have another question that, since build_cpu_topo() and > perf_event__preprocess_sample() are more or less doing similar things, > why we need both of them? > > Then we need more code for this bug... > > Kan Liang, do you have any suggestion? > > I think Jirka's way is better. We should handle al.socket in one place for all tools. Now we already read env from file in perf_session__new for perf report. I think we only need to update env in perf_session__new for other tools. So perf_event__preprocess_sample can use it. Thanks, Kan
Em Tue, Sep 08, 2015 at 09:37:47AM +0200, Jiri Olsa escreveu: > On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote: > > SNIP > > > > > I found the problem. > > > > perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. It > > depend on > > the existance of > > > > /sys/devices/system/cpu/cpu%d/topology/core_siblings_list > > > > However, CPU can be canceled by hotcpu subsystem. After that the directory > > of > > /sys/devices/system/cpu/cpu%d/topology is gone, which causes perf's > > write_cpu_topology() --> uild_cpu_topology() to fail, result in the above > > perf.data. > > > > So I think my patch is required. > > no question there.. I just meant it should be placed in > perf_event__preprocess_sample function with the rest of > the 'al' initialization, like in the patch below? > > it does not compile, because there're many places calling > it and it'd need changing all callers to pass env, which > seems to require more changes.. Humm, I think that we can have a pointer to the current perf_env, be it from the current machine, or from the machine environment in the perf.data file in struct machine, that way we don't need to change that function prototype, I'm prototyping this now, will post a patch. - Arnaldo > also I'm not sure about removing: > - al->socket = cpu_map__get_socket_id(al->cpu); > > > Does any command actually need this initialized from current system? > > thanks, > jirka > > > --- > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c > index 0bf8c9889fc0..3339d2579bfc 100644 > --- a/tools/perf/util/event.c > +++ b/tools/perf/util/event.c > @@ -990,7 +990,8 @@ void thread__find_addr_location(struct thread *thread, > int perf_event__preprocess_sample(const union perf_event *event, > struct machine *machine, > struct addr_location *al, > - struct perf_sample *sample) > + struct perf_sample *sample, > + struct perf_env *env) > { > u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK; > struct thread *thread = machine__findnew_thread(machine, sample->pid, > @@ -1021,7 +1022,10 @@ int perf_event__preprocess_sample(const union perf_event *event, > > al->sym = NULL; > al->cpu = sample->cpu; > - al->socket = cpu_map__get_socket_id(al->cpu); > + > + al.socket = -1; > + if (env->cpu && al->cpu >= 0) > + al.socket = env->cpu[al->cpu].socket_id; > > if (al->map) { > struct dso *dso = al->map->dso; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Sep 08, 2015 at 09:37:47AM +0200, Jiri Olsa escreveu: > > On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote: > > > > SNIP > > > > > > > > I found the problem. > > > > > > perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. It > > > depend on > > > the existance of > > > > > > /sys/devices/system/cpu/cpu%d/topology/core_siblings_list > > > > > > However, CPU can be canceled by hotcpu subsystem. After that the directory > > > of > > > /sys/devices/system/cpu/cpu%d/topology is gone, which causes perf's > > > write_cpu_topology() --> uild_cpu_topology() to fail, result in the above > > > perf.data. > > > > > > So I think my patch is required. > > > > no question there.. I just meant it should be placed in > > perf_event__preprocess_sample function with the rest of > > the 'al' initialization, like in the patch below? > > > > it does not compile, because there're many places calling > > it and it'd need changing all callers to pass env, which > > seems to require more changes.. > > Humm, I think that we can have a pointer to the current perf_env, be it > from the current machine, or from the machine environment in the > perf.data file in struct machine, that way we don't need to change that > function prototype, I'm prototyping this now, will post a patch. I was thinking of that.. but the perf_env is actualyl related to the perf.data not to the current machine.. I think it should be part of the session or perf_header jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Em Tue, Sep 08, 2015 at 12:49:10PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Sep 08, 2015 at 05:34:56PM +0200, Jiri Olsa escreveu: > > On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote: > > > Humm, I think that we can have a pointer to the current perf_env, be it > > > from the current machine, or from the machine environment in the > > > perf.data file in struct machine, that way we don't need to change that > > > function prototype, I'm prototyping this now, will post a patch. > > > > I was thinking of that.. but the perf_env is actualyl related to the > > perf.data not to the current machine.. I think it should be part of > > the session or perf_header > > But what if I want to trace only events that take place in some specific > socket, i.e. what to do when perf_session is not used at all and we are > not dealing with any header, since there are no files involved? So, this is the continuation of this patch: commit ce80d3bef9ff97638ca57a5659ef6ad356f35047 Author: Kan Liang <kan.liang@intel.com> Date: Fri Aug 28 05:48:04 2015 -0400 perf tools: Rename perf_session_env to perf_env As it is not necessarily tied to a perf.data file and needs using in places where a perf_session is not required. Suggested-by: Arnaldo Carvalho de Melo <acme@redhat.com> ----------------------------- perf_env not necessarily is related to a perf.data file, we need even to move it away from header.h. I am looking now at where to populate perf_env and set it to machine->env when no perf.data files are being accessed. I should have seen the use cpu_map__get_socket_id() in perf_event__preprocess_sample(), that is unnaceptable, as it will parse that file for each sample, right ;-\ Right now we don't have that much use for the other fields in 'perf_env', just for the CPU topology information, that we will set in addr_location for each sample, but we can have uses for that later, think about a TUI interface for 'perf trace' where we will show what was the command line, etc. - Arnaldo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Sep 08, 2015 at 12:49:10PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Tue, Sep 08, 2015 at 05:34:56PM +0200, Jiri Olsa escreveu: > > > On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote: > > > > Humm, I think that we can have a pointer to the current perf_env, be it > > > > from the current machine, or from the machine environment in the > > > > perf.data file in struct machine, that way we don't need to change that > > > > function prototype, I'm prototyping this now, will post a patch. > > > > > > I was thinking of that.. but the perf_env is actualyl related to the > > > perf.data not to the current machine.. I think it should be part of > > > the session or perf_header > > > > But what if I want to trace only events that take place in some specific > > socket, i.e. what to do when perf_session is not used at all and we are > > not dealing with any header, since there are no files involved? > > So, this is the continuation of this patch: > > commit ce80d3bef9ff97638ca57a5659ef6ad356f35047 > Author: Kan Liang <kan.liang@intel.com> > Date: Fri Aug 28 05:48:04 2015 -0400 > > perf tools: Rename perf_session_env to perf_env > > As it is not necessarily tied to a perf.data file and needs using in > places where a perf_session is not required. > > Suggested-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > ----------------------------- > > perf_env not necessarily is related to a perf.data file, we need even to > move it away from header.h. > > I am looking now at where to populate perf_env and set it to > machine->env when no perf.data files are being accessed. > > I should have seen the use cpu_map__get_socket_id() in > perf_event__preprocess_sample(), that is unnaceptable, as it will parse > that file for each sample, right ;-\ > > Right now we don't have that much use for the other fields in > 'perf_env', just for the CPU topology information, that we will set in > addr_location for each sample, but we can have uses for that later, > think about a TUI interface for 'perf trace' where we will show what was > the command line, etc. Argh, so in the patch introducing this al.socket thing it would first parse the value from the current system, reading sysfs, etc, then, in the 'report' case it would just throw this information away: - /* read socket id from perf.data for perf report */ - al.socket = env->cpu[al.cpu].socket_id; We really should do this in perf_event__preprocess_sample() and read the topology information just once, probably using the same routine that creates the perf.data file env record. - Arnaldo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Em Tue, Sep 08, 2015 at 01:13:59PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Tue, Sep 08, 2015 at 12:49:10PM -0300, Arnaldo Carvalho de Melo escreveu: > > > Em Tue, Sep 08, 2015 at 05:34:56PM +0200, Jiri Olsa escreveu: > > > > On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote: > > > > > Humm, I think that we can have a pointer to the current perf_env, be it > > > > > from the current machine, or from the machine environment in the > > > > > perf.data file in struct machine, that way we don't need to change that > > > > > function prototype, I'm prototyping this now, will post a patch. > > > > > > > > I was thinking of that.. but the perf_env is actualyl related to the > > > > perf.data not to the current machine.. I think it should be part of > > > > the session or perf_header > > > > > > But what if I want to trace only events that take place in some specific > > > socket, i.e. what to do when perf_session is not used at all and we are > > > not dealing with any header, since there are no files involved? > > > > So, this is the continuation of this patch: > > > > commit ce80d3bef9ff97638ca57a5659ef6ad356f35047 > > Author: Kan Liang <kan.liang@intel.com> > > Date: Fri Aug 28 05:48:04 2015 -0400 > > > > perf tools: Rename perf_session_env to perf_env > > > > As it is not necessarily tied to a perf.data file and needs using in > > places where a perf_session is not required. > > > > Suggested-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > > > ----------------------------- > > > > perf_env not necessarily is related to a perf.data file, we need even to > > move it away from header.h. > > > > I am looking now at where to populate perf_env and set it to > > machine->env when no perf.data files are being accessed. > > > > I should have seen the use cpu_map__get_socket_id() in > > perf_event__preprocess_sample(), that is unnaceptable, as it will parse > > that file for each sample, right ;-\ > > > > Right now we don't have that much use for the other fields in > > 'perf_env', just for the CPU topology information, that we will set in > > addr_location for each sample, but we can have uses for that later, > > think about a TUI interface for 'perf trace' where we will show what was > > the command line, etc. > > Argh, so in the patch introducing this al.socket thing it would first > parse the value from the current system, reading sysfs, etc, then, in > the 'report' case it would just throw this information away: > > - /* read socket id from perf.data for perf report */ > - al.socket = env->cpu[al.cpu].socket_id; > > We really should do this in perf_event__preprocess_sample() and read the > topology information just once, probably using the same routine that > creates the perf.data file env record. Lunch break, but I'll continue the work I started at https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core - Arnaldo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Em Tue, Sep 08, 2015 at 01:35:10PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Sep 08, 2015 at 01:13:59PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu: > > > Right now we don't have that much use for the other fields in > > > 'perf_env', just for the CPU topology information, that we will set in > > > addr_location for each sample, but we can have uses for that later, > > > think about a TUI interface for 'perf trace' where we will show what was > > > the command line, etc. > > Argh, so in the patch introducing this al.socket thing it would first > > parse the value from the current system, reading sysfs, etc, then, in > > the 'report' case it would just throw this information away: > > - /* read socket id from perf.data for perf report */ > > - al.socket = env->cpu[al.cpu].socket_id; > > We really should do this in perf_event__preprocess_sample() and read the > > topology information just once, probably using the same routine that > > creates the perf.data file env record. > Lunch break, but I'll continue the work I started at > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core Ok, I have them at my perf/env branch: https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/env 12 eefb0dbb1aa9 perf evsel: Remove forward declaration of 'struct perf_evlist' 11 8d9c09de8465 Revert "perf evsel: Add a backpointer to the evlist a evsel is in" 10 e3611f9dbced perf report: Do not blindly use env->cpu[al.cpu].socket_id 9 6f9ad1cd48e8 perf event: Use machine->env to find the cpu -> socket mapping 8 7b157d3eb7b4 perf machine: Add pointer to sample's environment 7 cc393318b0c0 perf hists browser: Fixup the "cpu" column width calculation 6 bc413472a275 perf top: Cache the cpu topology info when "-s socket" is used 5 908f1939ad52 perf sort: Set flag stating if the "socket" key is being used 4 81956a8be745 perf env: Introduce read_cpu_topology_map() method 3 7d76ec2a6f18 perf env: Adopt perf_header__set_cmdline 2 371495478108 perf env: Rename some leftovers from rename to perf_env 1 7b1a26d8cff7 perf env: Move perf_env out of header.h and session.c into separate object Now I see that one more probably is needed, the one adding the ->env backpointer to struct evlist. The ones fixing the problem are #9 and #10, the rest is infrastructure needed for those fixes to work, and some are not strictly needed but as I was working on it, couldn't resist cleaning up. I'll test this some more, add one or more cleanups and post for review, if all goes well, tomorrow I'll push it to Ingo. - Arnaldo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Em Wed, Sep 09, 2015 at 01:06:40PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Sep 08, 2015 at 01:35:10PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Tue, Sep 08, 2015 at 01:13:59PM -0300, Arnaldo Carvalho de Melo escreveu: > > > Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > Right now we don't have that much use for the other fields in > > > > 'perf_env', just for the CPU topology information, that we will set in > > > > addr_location for each sample, but we can have uses for that later, > > > > think about a TUI interface for 'perf trace' where we will show what was > > > > the command line, etc. > > > > Argh, so in the patch introducing this al.socket thing it would first > > > parse the value from the current system, reading sysfs, etc, then, in > > > the 'report' case it would just throw this information away: > > > > - /* read socket id from perf.data for perf report */ > > > - al.socket = env->cpu[al.cpu].socket_id; > > > > We really should do this in perf_event__preprocess_sample() and read the > > > topology information just once, probably using the same routine that > > > creates the perf.data file env record. > > > Lunch break, but I'll continue the work I started at > > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core > > Ok, I have them at my perf/env branch: > > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/env > > 12 eefb0dbb1aa9 perf evsel: Remove forward declaration of 'struct perf_evlist' > 11 8d9c09de8465 Revert "perf evsel: Add a backpointer to the evlist a evsel is in" > 10 e3611f9dbced perf report: Do not blindly use env->cpu[al.cpu].socket_id > 9 6f9ad1cd48e8 perf event: Use machine->env to find the cpu -> socket mapping > 8 7b157d3eb7b4 perf machine: Add pointer to sample's environment > 7 cc393318b0c0 perf hists browser: Fixup the "cpu" column width calculation > 6 bc413472a275 perf top: Cache the cpu topology info when "-s socket" is used > 5 908f1939ad52 perf sort: Set flag stating if the "socket" key is being used > 4 81956a8be745 perf env: Introduce read_cpu_topology_map() method > 3 7d76ec2a6f18 perf env: Adopt perf_header__set_cmdline > 2 371495478108 perf env: Rename some leftovers from rename to perf_env > 1 7b1a26d8cff7 perf env: Move perf_env out of header.h and session.c into separate object > > Now I see that one more probably is needed, the one adding the ->env > backpointer to struct evlist. Ok, one more: 'perf top' needs --socket-filter, will do after lunch. - Arnaldo > The ones fixing the problem are #9 and #10, the rest is infrastructure needed > for those fixes to work, and some are not strictly needed but as I was working > on it, couldn't resist cleaning up. > > I'll test this some more, add one or more cleanups and post for review, if all > goes well, tomorrow I'll push it to Ingo. > > - Arnaldo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c index 4b43245..16d097d 100644 --- a/tools/perf/builtin-report.c +++ b/tools/perf/builtin-report.c @@ -158,8 +158,16 @@ static int process_sample_event(struct perf_tool *tool, return -1; } - /* read socket id from perf.data for perf report */ - al.socket = env->cpu[al.cpu].socket_id; + /* + * read socket id from perf.data for perf report + * al.cpu is invalid if PERF_SAMPLE_CPU is not selected by this + * sample. + * env->cpu is invalid if CPU_TOPOLOGY feature is not set in + * header. + */ + al.socket = -1; + if (env->cpu && al.cpu >= 0) + al.socket = env->cpu[al.cpu].socket_id; if (rep->hide_unresolved && al.sym == NULL) goto out_put;
Commit e1e499aba570a2ea84d29822b7ea637ac41d9a51 (perf tools: Add processor socket info to hist_entry and addr_location) reads env->cpu array for each sample using index al.cpu. However, al.cpu can be -1 if sample doesn't select PERF_SAMPLE_CPU. Also, env->cpu can be invalid if feature CPU_TOPOLOGY not selected. We should validate env->cpu and al.cpu before setting al.socket. Signed-off-by: Wang Nan <wangnan0@huawei.com> Cc: Kan Liang <kan.liang@intel.com> Cc: Adrian Hunter <adrian.hunter@intel.com> Cc: Andi Kleen <ak@linux.intel.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Stephane Eranian <eranian@google.com> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> --- Although theoretically CPU_TOPOLOGY feature should always be selected by 'perf record', I did generate a perf.data without that feature. It has header like this: # perf report -i ./bad.perf.data --header-only # ======== # captured on: Thu Jan 8 09:30:15 2009 # hostname : localhost # os release : 3.10.49-gd672fc4 # perf version : 4.2.gc9df # arch : aarch64 # nrcpus online : 8 # nrcpus avail : 8 # total memory : 1850768 kB # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5 # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1 # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1 # pmu mappings: cpu = 4, software = 1, tracepoint = 2 # ======== # It should be: # ======== # captured on: Thu Jan 8 11:26:41 2009 ... # HEADER_CPU_TOPOLOGY info available, use -I to display # pmu mappings: cpu = 4, software = 1, tracepoint = 2 # ======== However, bad perf.data appears randomly. I can't stably reproduce it, so I guess there might have another invalid memory accessing. --- tools/perf/builtin-report.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)