| Message ID | 20181003212052.GA32371@krava (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf tools: Store ids for events with their own cpus perf_event__synthesize_event_update_cpus | expand |
>>> Hi Jirka, >>> >>> Can you please double-check your new patch, as I'm getting this now: >>> root@localhost:~# ./perf_debug record -e armv8_pmuv3_0/br_mis_pred/ sleep 1 >>> [ perf record: Woken up 1 times to write data ] >>> [ perf record: Captured and wrote 0.001 MB perf.data (6 samples) ] >>> root@localhost:~# ./perf_debug report >>> 0xe8 [0]: failed to process type: 461 >>> Error: >>> failed to process sample >>> # To display the perf.data header info, please use --header/--header-only >>> option >>> # >>> root@localhost:~# >> >> ok, I need to get a machine to test this.. but it looks like >> any sample-able events with cpumask are in arm :-\ will try >> to get some.. > > got an arm server and patch below works for me.. could you please test? > Cool, so this works ok: root@localhost:~# ./perf_debug record -e armv8_pmuv3_0/br_mis_pred/ sleep 1 [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.002 MB perf.data (6 samples) ] root@localhost:~# ./perf_debug report # To display the perf.data header info, please use --header/--header-only option # # # Total Lost Samples: 0 # # Samples: 6 of event 'armv8_pmuv3_0/br_mis_pred/' # Event count (approx.): 3369 # # Overhead Command Shared Object Symbol # ........ ....... ................. ................... # 94.81% sleep [kernel.kallsyms] [k] memcmp 4.87% sleep [kernel.kallsyms] [k] tlb_flush_mmu 0.33% perf_de [kernel.kallsyms] [k] perf_event_exec # # (Cannot load tips.txt file, please install perf!) # root@localhost:~# > thanks, > jirka > > > --- > > John reported crash when recording on an event under > pmu with cpumask defined: > > root@localhost:~# ./perf_debug_ record -e armv8_pmuv3_0/br_mis_pred/ sleep 1 > perf: Segmentation fault > Obtained 9 stack frames. > ./perf_debug_() [0x4c5ef8] > [0xffff82ba267c] > ./perf_debug_() [0x4bc5a8] > ./perf_debug_() [0x419550] > ./perf_debug_() [0x41a928] > ./perf_debug_() [0x472f58] > ./perf_debug_() [0x473210] > ./perf_debug_() [0x4070f4] > /lib/aarch64-linux-gnu/libc.so.6(__libc_start_main+0xe0) [0xffff8294c8a0] > Segmentation fault (core dumped) > > We synthesize an update event that needs to touch the evsel > id array, which is not defined at that time. Fixing this by > forcing the id allocation for events with theeir own cpus. > > Reported-by: John Garry <john.garry@huawei.com> > Link: http://lkml.kernel.org/n/tip-8x4n7o34yheigoxm1jibflm6@git.kernel.org Tested-by: John Garry <john.garry@huawei.com> In terms of adding to stable, LT v4.14 is not affected, but 4.18.x is. Thanks, John > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/perf/builtin-report.c | 1 + > tools/perf/util/evsel.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c > index c0703979c51d..257c9c18cb7e 100644 > --- a/tools/perf/builtin-report.c > +++ b/tools/perf/builtin-report.c > @@ -980,6 +980,7 @@ int cmd_report(int argc, const char **argv) > .id_index = perf_event__process_id_index, > .auxtrace_info = perf_event__process_auxtrace_info, > .auxtrace = perf_event__process_auxtrace, > + .event_update = perf_event__process_event_update, > .feature = process_feature_event, > .ordered_events = true, > .ordering_requires_timestamps = true, > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index ac6cfb8b085e..7a0d5fbaf3c1 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -1088,6 +1088,9 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts, > attr->exclude_user = 1; > } > > + if (evsel->own_cpus) > + evsel->attr.read_format |= PERF_FORMAT_ID; > + > /* > * Apply event specific term settings, > * it overloads any global configuration. >
On Thu, Oct 04, 2018 at 10:20:39AM +0100, John Garry wrote: SNIP > > We synthesize an update event that needs to touch the evsel > > id array, which is not defined at that time. Fixing this by > > forcing the id allocation for events with theeir own cpus. > > > > Reported-by: John Garry <john.garry@huawei.com> > > Link: http://lkml.kernel.org/n/tip-8x4n7o34yheigoxm1jibflm6@git.kernel.org > > Tested-by: John Garry <john.garry@huawei.com> > > In terms of adding to stable, LT v4.14 is not affected, but 4.18.x is. > > Thanks, > John Arnaldo, could you please pick up this one thanks, jirka > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > tools/perf/builtin-report.c | 1 + > > tools/perf/util/evsel.c | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c > > index c0703979c51d..257c9c18cb7e 100644 > > --- a/tools/perf/builtin-report.c > > +++ b/tools/perf/builtin-report.c > > @@ -980,6 +980,7 @@ int cmd_report(int argc, const char **argv) > > .id_index = perf_event__process_id_index, > > .auxtrace_info = perf_event__process_auxtrace_info, > > .auxtrace = perf_event__process_auxtrace, > > + .event_update = perf_event__process_event_update, > > .feature = process_feature_event, > > .ordered_events = true, > > .ordering_requires_timestamps = true, > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > > index ac6cfb8b085e..7a0d5fbaf3c1 100644 > > --- a/tools/perf/util/evsel.c > > +++ b/tools/perf/util/evsel.c > > @@ -1088,6 +1088,9 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts, > > attr->exclude_user = 1; > > } > > > > + if (evsel->own_cpus) > > + evsel->attr.read_format |= PERF_FORMAT_ID; > > + > > /* > > * Apply event specific term settings, > > * it overloads any global configuration. > > > >
On 09/10/2018 11:00, Jiri Olsa wrote: > On Thu, Oct 04, 2018 at 10:20:39AM +0100, John Garry wrote: > > SNIP > >>> We synthesize an update event that needs to touch the evsel >>> id array, which is not defined at that time. Fixing this by >>> forcing the id allocation for events with theeir own cpus. /s/theeir/their/ >>> >>> Reported-by: John Garry <john.garry@huawei.com> >>> Link: http://lkml.kernel.org/n/tip-8x4n7o34yheigoxm1jibflm6@git.kernel.org >> >> Tested-by: John Garry <john.garry@huawei.com> >> >> In terms of adding to stable, LT v4.14 is not affected, but 4.18.x is. >> >> Thanks, >> John > > Arnaldo, could you please pick up this one > Just a friendly reminder on this patch. How about re-send with an updated commit message also? Thanks, John > thanks, > jirka > >> >>> Signed-off-by: Jiri Olsa <jolsa@kernel.org> >>> --- >>> tools/perf/builtin-report.c | 1 + >>> tools/perf/util/evsel.c | 3 +++ >>> 2 files changed, 4 insertions(+) >>> >>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c >>> index c0703979c51d..257c9c18cb7e 100644 >>> --- a/tools/perf/builtin-report.c >>> +++ b/tools/perf/builtin-report.c >>> @@ -980,6 +980,7 @@ int cmd_report(int argc, const char **argv) >>> .id_index = perf_event__process_id_index, >>> .auxtrace_info = perf_event__process_auxtrace_info, >>> .auxtrace = perf_event__process_auxtrace, >>> + .event_update = perf_event__process_event_update, >>> .feature = process_feature_event, >>> .ordered_events = true, >>> .ordering_requires_timestamps = true, >>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >>> index ac6cfb8b085e..7a0d5fbaf3c1 100644 >>> --- a/tools/perf/util/evsel.c >>> +++ b/tools/perf/util/evsel.c >>> @@ -1088,6 +1088,9 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts, >>> attr->exclude_user = 1; >>> } >>> >>> + if (evsel->own_cpus) >>> + evsel->attr.read_format |= PERF_FORMAT_ID; >>> + >>> /* >>> * Apply event specific term settings, >>> * it overloads any global configuration. >>> >> >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . >
Em Fri, Oct 12, 2018 at 02:25:49PM +0100, John Garry escreveu: > On 09/10/2018 11:00, Jiri Olsa wrote: > > On Thu, Oct 04, 2018 at 10:20:39AM +0100, John Garry wrote: > > > > SNIP > > > > > > We synthesize an update event that needs to touch the evsel > > > > id array, which is not defined at that time. Fixing this by > > > > forcing the id allocation for events with theeir own cpus. > > /s/theeir/their/ > > > > > > > > > Reported-by: John Garry <john.garry@huawei.com> > > > > Link: http://lkml.kernel.org/n/tip-8x4n7o34yheigoxm1jibflm6@git.kernel.org > > > > > > Tested-by: John Garry <john.garry@huawei.com> > > > > > > In terms of adding to stable, LT v4.14 is not affected, but 4.18.x is. > > > > > > Thanks, > > > John > > > > Arnaldo, could you please pick up this one > > > > Just a friendly reminder on this patch. > > How about re-send with an updated commit message also? I'll fix up the commit message and apply, thanks. - Arnaldo > Thanks, > John > > > thanks, > > jirka > > > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > > --- > > > > tools/perf/builtin-report.c | 1 + > > > > tools/perf/util/evsel.c | 3 +++ > > > > 2 files changed, 4 insertions(+) > > > > > > > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c > > > > index c0703979c51d..257c9c18cb7e 100644 > > > > --- a/tools/perf/builtin-report.c > > > > +++ b/tools/perf/builtin-report.c > > > > @@ -980,6 +980,7 @@ int cmd_report(int argc, const char **argv) > > > > .id_index = perf_event__process_id_index, > > > > .auxtrace_info = perf_event__process_auxtrace_info, > > > > .auxtrace = perf_event__process_auxtrace, > > > > + .event_update = perf_event__process_event_update, > > > > .feature = process_feature_event, > > > > .ordered_events = true, > > > > .ordering_requires_timestamps = true, > > > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > > > > index ac6cfb8b085e..7a0d5fbaf3c1 100644 > > > > --- a/tools/perf/util/evsel.c > > > > +++ b/tools/perf/util/evsel.c > > > > @@ -1088,6 +1088,9 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts, > > > > attr->exclude_user = 1; > > > > } > > > > > > > > + if (evsel->own_cpus) > > > > + evsel->attr.read_format |= PERF_FORMAT_ID; > > > > + > > > > /* > > > > * Apply event specific term settings, > > > > * it overloads any global configuration. > > > > > > > > > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > . > > >
On 15/10/2018 20:15, Arnaldo Carvalho de Melo wrote: > Em Fri, Oct 12, 2018 at 02:25:49PM +0100, John Garry escreveu: >> On 09/10/2018 11:00, Jiri Olsa wrote: >>> On Thu, Oct 04, 2018 at 10:20:39AM +0100, John Garry wrote: >>> >>> SNIP >>> >>>>> We synthesize an update event that needs to touch the evsel >>>>> id array, which is not defined at that time. Fixing this by >>>>> forcing the id allocation for events with theeir own cpus. >> >> /s/theeir/their/ >> >>>>> >>>>> Reported-by: John Garry <john.garry@huawei.com> >>>>> Link: http://lkml.kernel.org/n/tip-8x4n7o34yheigoxm1jibflm6@git.kernel.org >>>> >>>> Tested-by: John Garry <john.garry@huawei.com> >>>> >>>> In terms of adding to stable, LT v4.14 is not affected, but 4.18.x is. >>>> >>>> Thanks, >>>> John >>> >>> Arnaldo, could you please pick up this one >>> >> >> Just a friendly reminder on this patch. >> >> How about re-send with an updated commit message also? > > I'll fix up the commit message and apply, thanks. > Much appreciated. BTW, I think that we should add a fixes tag. But I'm not sure if this patch fixes the commit I bisected to earlier (bfd8f72c2778f5bd63d), or that same commit just exposed some latent bug. Jirka, Andi, Do you know? Thanks, John > - Arnaldo > >> Thanks, >> John >> >>> thanks, >>> jirka >>> >>>> >>>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org> >>>>> --- >>>>> tools/perf/builtin-report.c | 1 + >>>>> tools/perf/util/evsel.c | 3 +++ >>>>> 2 files changed, 4 insertions(+) >>>>> >>>>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c >>>>> index c0703979c51d..257c9c18cb7e 100644 >>>>> --- a/tools/perf/builtin-report.c >>>>> +++ b/tools/perf/builtin-report.c >>>>> @@ -980,6 +980,7 @@ int cmd_report(int argc, const char **argv) >>>>> .id_index = perf_event__process_id_index, >>>>> .auxtrace_info = perf_event__process_auxtrace_info, >>>>> .auxtrace = perf_event__process_auxtrace, >>>>> + .event_update = perf_event__process_event_update, >>>>> .feature = process_feature_event, >>>>> .ordered_events = true, >>>>> .ordering_requires_timestamps = true, >>>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >>>>> index ac6cfb8b085e..7a0d5fbaf3c1 100644 >>>>> --- a/tools/perf/util/evsel.c >>>>> +++ b/tools/perf/util/evsel.c >>>>> @@ -1088,6 +1088,9 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts, >>>>> attr->exclude_user = 1; >>>>> } >>>>> >>>>> + if (evsel->own_cpus) >>>>> + evsel->attr.read_format |= PERF_FORMAT_ID; >>>>> + >>>>> /* >>>>> * Apply event specific term settings, >>>>> * it overloads any global configuration. >>>>> >>>> >>>> >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> >>> . >>> >> > > . >
On Tue, Oct 16, 2018 at 10:10:20AM +0100, John Garry wrote: > On 15/10/2018 20:15, Arnaldo Carvalho de Melo wrote: > > Em Fri, Oct 12, 2018 at 02:25:49PM +0100, John Garry escreveu: > > > On 09/10/2018 11:00, Jiri Olsa wrote: > > > > On Thu, Oct 04, 2018 at 10:20:39AM +0100, John Garry wrote: > > > > > > > > SNIP > > > > > > > > > > We synthesize an update event that needs to touch the evsel > > > > > > id array, which is not defined at that time. Fixing this by > > > > > > forcing the id allocation for events with theeir own cpus. > > > > > > /s/theeir/their/ > > > > > > > > > > > > > > > Reported-by: John Garry <john.garry@huawei.com> > > > > > > Link: http://lkml.kernel.org/n/tip-8x4n7o34yheigoxm1jibflm6@git.kernel.org > > > > > > > > > > Tested-by: John Garry <john.garry@huawei.com> > > > > > > > > > > In terms of adding to stable, LT v4.14 is not affected, but 4.18.x is. > > > > > > > > > > Thanks, > > > > > John > > > > > > > > Arnaldo, could you please pick up this one > > > > > > > > > > Just a friendly reminder on this patch. > > > > > > How about re-send with an updated commit message also? > > > > I'll fix up the commit message and apply, thanks. > > > > Much appreciated. > > BTW, I think that we should add a fixes tag. But I'm not sure if this patch > fixes the commit I bisected to earlier (bfd8f72c2778f5bd63d), or that same > commit just exposed some latent bug. > > Jirka, Andi, Do you know? yes, that commit moved it to record, which resulted in this fault Arnaldo, could you please add: Fixes: bfd8f72c2778 ("perf record: Synthesize unit/scale/... in event update") thanks, jirka
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c index c0703979c51d..257c9c18cb7e 100644 --- a/tools/perf/builtin-report.c +++ b/tools/perf/builtin-report.c @@ -980,6 +980,7 @@ int cmd_report(int argc, const char **argv) .id_index = perf_event__process_id_index, .auxtrace_info = perf_event__process_auxtrace_info, .auxtrace = perf_event__process_auxtrace, + .event_update = perf_event__process_event_update, .feature = process_feature_event, .ordered_events = true, .ordering_requires_timestamps = true, diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index ac6cfb8b085e..7a0d5fbaf3c1 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -1088,6 +1088,9 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts, attr->exclude_user = 1; } + if (evsel->own_cpus) + evsel->attr.read_format |= PERF_FORMAT_ID; + /* * Apply event specific term settings, * it overloads any global configuration.