* [PATCH 0/2] perf tools: Fix hybrid config terms list corruption
@ 2021-09-09 12:55 Adrian Hunter
2021-09-09 12:55 ` [PATCH 1/2] perf tools: Factor out copy_config_terms() and free_config_terms() Adrian Hunter
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Adrian Hunter @ 2021-09-09 12:55 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: Jiri Olsa, Jin Yao, Kan Liang, linux-kernel
Hi
Here is a fix for an issue using perf on ADL.
Adrian Hunter (2):
perf tools: Factor out copy_config_terms() and free_config_terms()
perf tools: Fix hybrid config terms list corruption
tools/perf/util/evsel.c | 20 +++++++++++++++-----
tools/perf/util/evsel.h | 3 +++
tools/perf/util/parse-events-hybrid.c | 18 +++++++++++++++---
tools/perf/util/parse-events.c | 27 +++++++++++++--------------
4 files changed, 46 insertions(+), 22 deletions(-)
Regards
Adrian
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] perf tools: Factor out copy_config_terms() and free_config_terms() 2021-09-09 12:55 [PATCH 0/2] perf tools: Fix hybrid config terms list corruption Adrian Hunter @ 2021-09-09 12:55 ` Adrian Hunter 2021-09-09 12:55 ` [PATCH 2/2] perf tools: Fix hybrid config terms list corruption Adrian Hunter 2021-09-11 10:57 ` [PATCH 0/2] " Jiri Olsa 2 siblings, 0 replies; 7+ messages in thread From: Adrian Hunter @ 2021-09-09 12:55 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: Jiri Olsa, Jin Yao, Kan Liang, linux-kernel Factor out copy_config_terms() and free_config_terms() so that they can be reused. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- tools/perf/util/evsel.c | 20 +++++++++++++++----- tools/perf/util/evsel.h | 3 +++ tools/perf/util/parse-events.c | 9 +-------- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index 54d251327b5b..dbfeceb2546c 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -333,11 +333,11 @@ struct evsel *evsel__new_cycles(bool precise, __u32 type, __u64 config) goto out; } -static int evsel__copy_config_terms(struct evsel *dst, struct evsel *src) +int copy_config_terms(struct list_head *dst, struct list_head *src) { struct evsel_config_term *pos, *tmp; - list_for_each_entry(pos, &src->config_terms, list) { + list_for_each_entry(pos, src, list) { tmp = malloc(sizeof(*tmp)); if (tmp == NULL) return -ENOMEM; @@ -350,11 +350,16 @@ static int evsel__copy_config_terms(struct evsel *dst, struct evsel *src) return -ENOMEM; } } - list_add_tail(&tmp->list, &dst->config_terms); + list_add_tail(&tmp->list, dst); } return 0; } +static int evsel__copy_config_terms(struct evsel *dst, struct evsel *src) +{ + return copy_config_terms(&dst->config_terms, &src->config_terms); +} + /** * evsel__clone - create a new evsel copied from @orig * @orig: original evsel @@ -1385,11 +1390,11 @@ int evsel__disable(struct evsel *evsel) return err; } -static void evsel__free_config_terms(struct evsel *evsel) +void free_config_terms(struct list_head *config_terms) { struct evsel_config_term *term, *h; - list_for_each_entry_safe(term, h, &evsel->config_terms, list) { + list_for_each_entry_safe(term, h, config_terms, list) { list_del_init(&term->list); if (term->free_str) zfree(&term->val.str); @@ -1397,6 +1402,11 @@ static void evsel__free_config_terms(struct evsel *evsel) } } +static void evsel__free_config_terms(struct evsel *evsel) +{ + free_config_terms(&evsel->config_terms); +} + void evsel__exit(struct evsel *evsel) { assert(list_empty(&evsel->core.node)); diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index 1b3eeab5f188..1f7edfa8568a 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -213,6 +213,9 @@ static inline struct evsel *evsel__new(struct perf_event_attr *attr) struct evsel *evsel__clone(struct evsel *orig); struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx); +int copy_config_terms(struct list_head *dst, struct list_head *src); +void free_config_terms(struct list_head *config_terms); + /* * Returns pointer with encoded error via <linux/err.h> interface. */ diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index e5eae23cfceb..ded5808798f9 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -1608,14 +1608,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, } if (!parse_state->fake_pmu && perf_pmu__config(pmu, &attr, head_config, parse_state->error)) { - struct evsel_config_term *pos, *tmp; - - list_for_each_entry_safe(pos, tmp, &config_terms, list) { - list_del_init(&pos->list); - if (pos->free_str) - zfree(&pos->val.str); - free(pos); - } + free_config_terms(&config_terms); return -EINVAL; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] perf tools: Fix hybrid config terms list corruption 2021-09-09 12:55 [PATCH 0/2] perf tools: Fix hybrid config terms list corruption Adrian Hunter 2021-09-09 12:55 ` [PATCH 1/2] perf tools: Factor out copy_config_terms() and free_config_terms() Adrian Hunter @ 2021-09-09 12:55 ` Adrian Hunter 2021-09-10 19:32 ` Jiri Olsa 2021-09-11 10:57 ` [PATCH 0/2] " Jiri Olsa 2 siblings, 1 reply; 7+ messages in thread From: Adrian Hunter @ 2021-09-09 12:55 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: Jiri Olsa, Jin Yao, Kan Liang, linux-kernel A config terms list was spliced twice, resulting in a never-ending loop when the list was traversed. Fix by using list_splice_init() and copying and freeing the lists as necessary. This patch also depends on patch "perf tools: Factor out copy_config_terms() and free_config_terms()" Example on ADL: Before: # perf record -e '{intel_pt//,cycles/aux-sample-size=4096/pp}' uname & # jobs [1]+ Running perf record -e "{intel_pt//,cycles/aux-sample-size=4096/pp}" uname # perf top -E 10 PerfTop: 4071 irqs/sec kernel: 6.9% exact: 100.0% lost: 0/0 drop: 0/0 [4000Hz cycles], (all, 24 CPUs) --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 97.60% perf [.] __evsel__get_config_term 0.25% [kernel] [k] kallsyms_expand_symbol.constprop.13 0.24% perf [.] kallsyms__parse 0.15% [kernel] [k] _raw_spin_lock 0.14% [kernel] [k] number 0.13% [kernel] [k] advance_transaction 0.08% [kernel] [k] format_decode 0.08% perf [.] map__process_kallsym_symbol 0.08% perf [.] rb_insert_color 0.08% [kernel] [k] vsnprintf exiting. # kill %1 After: # perf record -e '{intel_pt//,cycles/aux-sample-size=4096/pp}' uname & Linux [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.060 MB perf.data ] # perf script | head perf-exec 604 [001] 1827.312293: psb: psb offs: 0 ffffffffb8415e87 pt_config_start+0x37 ([kernel.kallsyms]) perf-exec 604 1827.312293: 1 branches: ffffffffb856a3bd event_sched_in.isra.133+0xfd ([kernel.kallsyms]) => ffffffffb856a9a0 perf_pmu_nop_void+0x0 ([kernel.kallsyms]) perf-exec 604 1827.312293: 1 branches: ffffffffb856b10e merge_sched_in+0x26e ([kernel.kallsyms]) => ffffffffb856a2c0 event_sched_in.isra.133+0x0 ([kernel.kallsyms]) perf-exec 604 1827.312293: 1 branches: ffffffffb856a45d event_sched_in.isra.133+0x19d ([kernel.kallsyms]) => ffffffffb8568b80 perf_event_set_state.part.61+0x0 ([kernel.kallsyms]) perf-exec 604 1827.312293: 1 branches: ffffffffb8568b86 perf_event_set_state.part.61+0x6 ([kernel.kallsyms]) => ffffffffb85662a0 perf_event_update_time+0x0 ([kernel.kallsyms]) perf-exec 604 1827.312293: 1 branches: ffffffffb856a35c event_sched_in.isra.133+0x9c ([kernel.kallsyms]) => ffffffffb8567610 perf_log_itrace_start+0x0 ([kernel.kallsyms]) perf-exec 604 1827.312293: 1 branches: ffffffffb856a377 event_sched_in.isra.133+0xb7 ([kernel.kallsyms]) => ffffffffb8403b40 x86_pmu_add+0x0 ([kernel.kallsyms]) perf-exec 604 1827.312293: 1 branches: ffffffffb8403b86 x86_pmu_add+0x46 ([kernel.kallsyms]) => ffffffffb8403940 collect_events+0x0 ([kernel.kallsyms]) perf-exec 604 1827.312293: 1 branches: ffffffffb8403a7b collect_events+0x13b ([kernel.kallsyms]) => ffffffffb8402cd0 collect_event+0x0 ([kernel.kallsyms]) Fixes: 94da591b1c7913 ("perf parse-events: Create two hybrid raw events") Fixes: 9cbfa2f64c04d9 ("perf parse-events: Create two hybrid hardware events") Fixes: 30def61f64bac5 ("perf parse-events: Create two hybrid cache events") Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- tools/perf/util/parse-events-hybrid.c | 18 +++++++++++++++--- tools/perf/util/parse-events.c | 18 ++++++++++++------ 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c index 10160ab126f9..b234d95fb10a 100644 --- a/tools/perf/util/parse-events-hybrid.c +++ b/tools/perf/util/parse-events-hybrid.c @@ -76,12 +76,16 @@ static int add_hw_hybrid(struct parse_events_state *parse_state, int ret; perf_pmu__for_each_hybrid_pmu(pmu) { + LIST_HEAD(terms); + if (pmu_cmp(parse_state, pmu)) continue; + copy_config_terms(&terms, config_terms); ret = create_event_hybrid(PERF_TYPE_HARDWARE, &parse_state->idx, list, attr, name, - config_terms, pmu); + &terms, pmu); + free_config_terms(&terms); if (ret) return ret; } @@ -115,11 +119,15 @@ static int add_raw_hybrid(struct parse_events_state *parse_state, int ret; perf_pmu__for_each_hybrid_pmu(pmu) { + LIST_HEAD(terms); + if (pmu_cmp(parse_state, pmu)) continue; + copy_config_terms(&terms, config_terms); ret = create_raw_event_hybrid(&parse_state->idx, list, attr, - name, config_terms, pmu); + name, &terms, pmu); + free_config_terms(&terms); if (ret) return ret; } @@ -165,11 +173,15 @@ int parse_events__add_cache_hybrid(struct list_head *list, int *idx, *hybrid = true; perf_pmu__for_each_hybrid_pmu(pmu) { + LIST_HEAD(terms); + if (pmu_cmp(parse_state, pmu)) continue; + copy_config_terms(&terms, config_terms); ret = create_event_hybrid(PERF_TYPE_HW_CACHE, idx, list, - attr, name, config_terms, pmu); + attr, name, &terms, pmu); + free_config_terms(&terms); if (ret) return ret; } diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index ded5808798f9..51a2219df601 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -387,7 +387,7 @@ __add_event(struct list_head *list, int *idx, evsel->name = strdup(name); if (config_terms) - list_splice(config_terms, &evsel->config_terms); + list_splice_init(config_terms, &evsel->config_terms); if (list) list_add_tail(&evsel->core.node, list); @@ -535,9 +535,12 @@ int parse_events_add_cache(struct list_head *list, int *idx, config_name ? : name, &config_terms, &hybrid, parse_state); if (hybrid) - return ret; + goto out_free_terms; - return add_event(list, idx, &attr, config_name ? : name, &config_terms); + ret = add_event(list, idx, &attr, config_name ? : name, &config_terms); +out_free_terms: + free_config_terms(&config_terms); + return ret; } static void tracepoint_error(struct parse_events_error *e, int err, @@ -1457,10 +1460,13 @@ int parse_events_add_numeric(struct parse_events_state *parse_state, get_config_name(head_config), &config_terms, &hybrid); if (hybrid) - return ret; + goto out_free_terms; - return add_event(list, &parse_state->idx, &attr, - get_config_name(head_config), &config_terms); + ret = add_event(list, &parse_state->idx, &attr, + get_config_name(head_config), &config_terms); +out_free_terms: + free_config_terms(&config_terms); + return ret; } int parse_events_add_tool(struct parse_events_state *parse_state, -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] perf tools: Fix hybrid config terms list corruption 2021-09-09 12:55 ` [PATCH 2/2] perf tools: Fix hybrid config terms list corruption Adrian Hunter @ 2021-09-10 19:32 ` Jiri Olsa 2021-09-11 6:23 ` Adrian Hunter 0 siblings, 1 reply; 7+ messages in thread From: Jiri Olsa @ 2021-09-10 19:32 UTC (permalink / raw) To: Adrian Hunter; +Cc: Arnaldo Carvalho de Melo, Jin Yao, Kan Liang, linux-kernel On Thu, Sep 09, 2021 at 03:55:08PM +0300, Adrian Hunter wrote: > A config terms list was spliced twice, resulting in a never-ending loop > when the list was traversed. Fix by using list_splice_init() and copying > and freeing the lists as necessary. > > This patch also depends on patch "perf tools: Factor out > copy_config_terms() and free_config_terms()" > > Example on ADL: > > Before: > > # perf record -e '{intel_pt//,cycles/aux-sample-size=4096/pp}' uname & > # jobs > [1]+ Running perf record -e "{intel_pt//,cycles/aux-sample-size=4096/pp}" uname > # perf top -E 10 > PerfTop: 4071 irqs/sec kernel: 6.9% exact: 100.0% lost: 0/0 drop: 0/0 [4000Hz cycles], (all, 24 CPUs) > --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > > 97.60% perf [.] __evsel__get_config_term > 0.25% [kernel] [k] kallsyms_expand_symbol.constprop.13 > 0.24% perf [.] kallsyms__parse > 0.15% [kernel] [k] _raw_spin_lock > 0.14% [kernel] [k] number > 0.13% [kernel] [k] advance_transaction > 0.08% [kernel] [k] format_decode > 0.08% perf [.] map__process_kallsym_symbol > 0.08% perf [.] rb_insert_color > 0.08% [kernel] [k] vsnprintf > exiting. > # kill %1 > > After: > > # perf record -e '{intel_pt//,cycles/aux-sample-size=4096/pp}' uname & > Linux > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.060 MB perf.data ] > # perf script | head > perf-exec 604 [001] 1827.312293: psb: psb offs: 0 ffffffffb8415e87 pt_config_start+0x37 ([kernel.kallsyms]) > perf-exec 604 1827.312293: 1 branches: ffffffffb856a3bd event_sched_in.isra.133+0xfd ([kernel.kallsyms]) => ffffffffb856a9a0 perf_pmu_nop_void+0x0 ([kernel.kallsyms]) > perf-exec 604 1827.312293: 1 branches: ffffffffb856b10e merge_sched_in+0x26e ([kernel.kallsyms]) => ffffffffb856a2c0 event_sched_in.isra.133+0x0 ([kernel.kallsyms]) > perf-exec 604 1827.312293: 1 branches: ffffffffb856a45d event_sched_in.isra.133+0x19d ([kernel.kallsyms]) => ffffffffb8568b80 perf_event_set_state.part.61+0x0 ([kernel.kallsyms]) > perf-exec 604 1827.312293: 1 branches: ffffffffb8568b86 perf_event_set_state.part.61+0x6 ([kernel.kallsyms]) => ffffffffb85662a0 perf_event_update_time+0x0 ([kernel.kallsyms]) > perf-exec 604 1827.312293: 1 branches: ffffffffb856a35c event_sched_in.isra.133+0x9c ([kernel.kallsyms]) => ffffffffb8567610 perf_log_itrace_start+0x0 ([kernel.kallsyms]) > perf-exec 604 1827.312293: 1 branches: ffffffffb856a377 event_sched_in.isra.133+0xb7 ([kernel.kallsyms]) => ffffffffb8403b40 x86_pmu_add+0x0 ([kernel.kallsyms]) > perf-exec 604 1827.312293: 1 branches: ffffffffb8403b86 x86_pmu_add+0x46 ([kernel.kallsyms]) => ffffffffb8403940 collect_events+0x0 ([kernel.kallsyms]) > perf-exec 604 1827.312293: 1 branches: ffffffffb8403a7b collect_events+0x13b ([kernel.kallsyms]) => ffffffffb8402cd0 collect_event+0x0 ([kernel.kallsyms]) > > Fixes: 94da591b1c7913 ("perf parse-events: Create two hybrid raw events") > Fixes: 9cbfa2f64c04d9 ("perf parse-events: Create two hybrid hardware events") > Fixes: 30def61f64bac5 ("perf parse-events: Create two hybrid cache events") > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > tools/perf/util/parse-events-hybrid.c | 18 +++++++++++++++--- > tools/perf/util/parse-events.c | 18 ++++++++++++------ > 2 files changed, 27 insertions(+), 9 deletions(-) > > diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c > index 10160ab126f9..b234d95fb10a 100644 > --- a/tools/perf/util/parse-events-hybrid.c > +++ b/tools/perf/util/parse-events-hybrid.c > @@ -76,12 +76,16 @@ static int add_hw_hybrid(struct parse_events_state *parse_state, > int ret; > > perf_pmu__for_each_hybrid_pmu(pmu) { > + LIST_HEAD(terms); > + > if (pmu_cmp(parse_state, pmu)) > continue; > > + copy_config_terms(&terms, config_terms); > ret = create_event_hybrid(PERF_TYPE_HARDWARE, > &parse_state->idx, list, attr, name, > - config_terms, pmu); > + &terms, pmu); > + free_config_terms(&terms); hm, why is this needed.. there's now list_splice_init &terms within create_event_hybrid call right? so there should be nothing nothing to clean same below > if (ret) > return ret; > } > @@ -115,11 +119,15 @@ static int add_raw_hybrid(struct parse_events_state *parse_state, > int ret; > > perf_pmu__for_each_hybrid_pmu(pmu) { > + LIST_HEAD(terms); > + > if (pmu_cmp(parse_state, pmu)) > continue; > > + copy_config_terms(&terms, config_terms); > ret = create_raw_event_hybrid(&parse_state->idx, list, attr, > - name, config_terms, pmu); > + name, &terms, pmu); > + free_config_terms(&terms); > if (ret) > return ret; > } > @@ -165,11 +173,15 @@ int parse_events__add_cache_hybrid(struct list_head *list, int *idx, > > *hybrid = true; > perf_pmu__for_each_hybrid_pmu(pmu) { > + LIST_HEAD(terms); > + > if (pmu_cmp(parse_state, pmu)) > continue; > > + copy_config_terms(&terms, config_terms); > ret = create_event_hybrid(PERF_TYPE_HW_CACHE, idx, list, > - attr, name, config_terms, pmu); > + attr, name, &terms, pmu); > + free_config_terms(&terms); > if (ret) > return ret; > } > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index ded5808798f9..51a2219df601 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -387,7 +387,7 @@ __add_event(struct list_head *list, int *idx, > evsel->name = strdup(name); > > if (config_terms) > - list_splice(config_terms, &evsel->config_terms); > + list_splice_init(config_terms, &evsel->config_terms); > > if (list) > list_add_tail(&evsel->core.node, list); > @@ -535,9 +535,12 @@ int parse_events_add_cache(struct list_head *list, int *idx, > config_name ? : name, &config_terms, > &hybrid, parse_state); > if (hybrid) > - return ret; > + goto out_free_terms; > > - return add_event(list, idx, &attr, config_name ? : name, &config_terms); > + ret = add_event(list, idx, &attr, config_name ? : name, &config_terms); > +out_free_terms: > + free_config_terms(&config_terms); .. apart from this one and below, when coming from 'goto out_free_terms' we should call free_config_terms jirka > + return ret; > } > > static void tracepoint_error(struct parse_events_error *e, int err, > @@ -1457,10 +1460,13 @@ int parse_events_add_numeric(struct parse_events_state *parse_state, > get_config_name(head_config), > &config_terms, &hybrid); > if (hybrid) > - return ret; > + goto out_free_terms; > > - return add_event(list, &parse_state->idx, &attr, > - get_config_name(head_config), &config_terms); > + ret = add_event(list, &parse_state->idx, &attr, > + get_config_name(head_config), &config_terms); > +out_free_terms: > + free_config_terms(&config_terms); > + return ret; > } > > int parse_events_add_tool(struct parse_events_state *parse_state, > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] perf tools: Fix hybrid config terms list corruption 2021-09-10 19:32 ` Jiri Olsa @ 2021-09-11 6:23 ` Adrian Hunter 0 siblings, 0 replies; 7+ messages in thread From: Adrian Hunter @ 2021-09-11 6:23 UTC (permalink / raw) To: Jiri Olsa; +Cc: Arnaldo Carvalho de Melo, Jin Yao, Kan Liang, linux-kernel On 10/09/21 10:32 pm, Jiri Olsa wrote: > On Thu, Sep 09, 2021 at 03:55:08PM +0300, Adrian Hunter wrote: >> A config terms list was spliced twice, resulting in a never-ending loop >> when the list was traversed. Fix by using list_splice_init() and copying >> and freeing the lists as necessary. >> >> This patch also depends on patch "perf tools: Factor out >> copy_config_terms() and free_config_terms()" >> >> Example on ADL: >> >> Before: >> >> # perf record -e '{intel_pt//,cycles/aux-sample-size=4096/pp}' uname & >> # jobs >> [1]+ Running perf record -e "{intel_pt//,cycles/aux-sample-size=4096/pp}" uname >> # perf top -E 10 >> PerfTop: 4071 irqs/sec kernel: 6.9% exact: 100.0% lost: 0/0 drop: 0/0 [4000Hz cycles], (all, 24 CPUs) >> --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- >> >> 97.60% perf [.] __evsel__get_config_term >> 0.25% [kernel] [k] kallsyms_expand_symbol.constprop.13 >> 0.24% perf [.] kallsyms__parse >> 0.15% [kernel] [k] _raw_spin_lock >> 0.14% [kernel] [k] number >> 0.13% [kernel] [k] advance_transaction >> 0.08% [kernel] [k] format_decode >> 0.08% perf [.] map__process_kallsym_symbol >> 0.08% perf [.] rb_insert_color >> 0.08% [kernel] [k] vsnprintf >> exiting. >> # kill %1 >> >> After: >> >> # perf record -e '{intel_pt//,cycles/aux-sample-size=4096/pp}' uname & >> Linux >> [ perf record: Woken up 1 times to write data ] >> [ perf record: Captured and wrote 0.060 MB perf.data ] >> # perf script | head >> perf-exec 604 [001] 1827.312293: psb: psb offs: 0 ffffffffb8415e87 pt_config_start+0x37 ([kernel.kallsyms]) >> perf-exec 604 1827.312293: 1 branches: ffffffffb856a3bd event_sched_in.isra.133+0xfd ([kernel.kallsyms]) => ffffffffb856a9a0 perf_pmu_nop_void+0x0 ([kernel.kallsyms]) >> perf-exec 604 1827.312293: 1 branches: ffffffffb856b10e merge_sched_in+0x26e ([kernel.kallsyms]) => ffffffffb856a2c0 event_sched_in.isra.133+0x0 ([kernel.kallsyms]) >> perf-exec 604 1827.312293: 1 branches: ffffffffb856a45d event_sched_in.isra.133+0x19d ([kernel.kallsyms]) => ffffffffb8568b80 perf_event_set_state.part.61+0x0 ([kernel.kallsyms]) >> perf-exec 604 1827.312293: 1 branches: ffffffffb8568b86 perf_event_set_state.part.61+0x6 ([kernel.kallsyms]) => ffffffffb85662a0 perf_event_update_time+0x0 ([kernel.kallsyms]) >> perf-exec 604 1827.312293: 1 branches: ffffffffb856a35c event_sched_in.isra.133+0x9c ([kernel.kallsyms]) => ffffffffb8567610 perf_log_itrace_start+0x0 ([kernel.kallsyms]) >> perf-exec 604 1827.312293: 1 branches: ffffffffb856a377 event_sched_in.isra.133+0xb7 ([kernel.kallsyms]) => ffffffffb8403b40 x86_pmu_add+0x0 ([kernel.kallsyms]) >> perf-exec 604 1827.312293: 1 branches: ffffffffb8403b86 x86_pmu_add+0x46 ([kernel.kallsyms]) => ffffffffb8403940 collect_events+0x0 ([kernel.kallsyms]) >> perf-exec 604 1827.312293: 1 branches: ffffffffb8403a7b collect_events+0x13b ([kernel.kallsyms]) => ffffffffb8402cd0 collect_event+0x0 ([kernel.kallsyms]) >> >> Fixes: 94da591b1c7913 ("perf parse-events: Create two hybrid raw events") >> Fixes: 9cbfa2f64c04d9 ("perf parse-events: Create two hybrid hardware events") >> Fixes: 30def61f64bac5 ("perf parse-events: Create two hybrid cache events") >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> tools/perf/util/parse-events-hybrid.c | 18 +++++++++++++++--- >> tools/perf/util/parse-events.c | 18 ++++++++++++------ >> 2 files changed, 27 insertions(+), 9 deletions(-) >> >> diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c >> index 10160ab126f9..b234d95fb10a 100644 >> --- a/tools/perf/util/parse-events-hybrid.c >> +++ b/tools/perf/util/parse-events-hybrid.c >> @@ -76,12 +76,16 @@ static int add_hw_hybrid(struct parse_events_state *parse_state, >> int ret; >> >> perf_pmu__for_each_hybrid_pmu(pmu) { >> + LIST_HEAD(terms); >> + >> if (pmu_cmp(parse_state, pmu)) >> continue; >> >> + copy_config_terms(&terms, config_terms); >> ret = create_event_hybrid(PERF_TYPE_HARDWARE, >> &parse_state->idx, list, attr, name, >> - config_terms, pmu); >> + &terms, pmu); >> + free_config_terms(&terms); > > hm, why is this needed.. there's now list_splice_init &terms within > create_event_hybrid call right? so there should be nothing nothing > to clean create_event_hybrid() can fail and not consume 'terms'. By using list_splice_init(), free_config_terms() will only free unconsumed terms. > > same below That also has the same reasoning. > >> if (ret) >> return ret; >> } >> @@ -115,11 +119,15 @@ static int add_raw_hybrid(struct parse_events_state *parse_state, >> int ret; >> >> perf_pmu__for_each_hybrid_pmu(pmu) { >> + LIST_HEAD(terms); >> + >> if (pmu_cmp(parse_state, pmu)) >> continue; >> >> + copy_config_terms(&terms, config_terms); >> ret = create_raw_event_hybrid(&parse_state->idx, list, attr, >> - name, config_terms, pmu); >> + name, &terms, pmu); >> + free_config_terms(&terms); >> if (ret) >> return ret; >> } >> @@ -165,11 +173,15 @@ int parse_events__add_cache_hybrid(struct list_head *list, int *idx, >> >> *hybrid = true; >> perf_pmu__for_each_hybrid_pmu(pmu) { >> + LIST_HEAD(terms); >> + >> if (pmu_cmp(parse_state, pmu)) >> continue; >> >> + copy_config_terms(&terms, config_terms); >> ret = create_event_hybrid(PERF_TYPE_HW_CACHE, idx, list, >> - attr, name, config_terms, pmu); >> + attr, name, &terms, pmu); >> + free_config_terms(&terms); >> if (ret) >> return ret; >> } >> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >> index ded5808798f9..51a2219df601 100644 >> --- a/tools/perf/util/parse-events.c >> +++ b/tools/perf/util/parse-events.c >> @@ -387,7 +387,7 @@ __add_event(struct list_head *list, int *idx, >> evsel->name = strdup(name); >> >> if (config_terms) >> - list_splice(config_terms, &evsel->config_terms); >> + list_splice_init(config_terms, &evsel->config_terms); >> >> if (list) >> list_add_tail(&evsel->core.node, list); >> @@ -535,9 +535,12 @@ int parse_events_add_cache(struct list_head *list, int *idx, >> config_name ? : name, &config_terms, >> &hybrid, parse_state); >> if (hybrid) >> - return ret; >> + goto out_free_terms; >> >> - return add_event(list, idx, &attr, config_name ? : name, &config_terms); >> + ret = add_event(list, idx, &attr, config_name ? : name, &config_terms); >> +out_free_terms: >> + free_config_terms(&config_terms); > > .. apart from this one and below, when coming from 'goto out_free_terms' > we should call free_config_terms > > jirka > >> + return ret; >> } >> >> static void tracepoint_error(struct parse_events_error *e, int err, >> @@ -1457,10 +1460,13 @@ int parse_events_add_numeric(struct parse_events_state *parse_state, >> get_config_name(head_config), >> &config_terms, &hybrid); >> if (hybrid) >> - return ret; >> + goto out_free_terms; >> >> - return add_event(list, &parse_state->idx, &attr, >> - get_config_name(head_config), &config_terms); >> + ret = add_event(list, &parse_state->idx, &attr, >> + get_config_name(head_config), &config_terms); >> +out_free_terms: >> + free_config_terms(&config_terms); >> + return ret; >> } >> >> int parse_events_add_tool(struct parse_events_state *parse_state, >> -- >> 2.17.1 >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] perf tools: Fix hybrid config terms list corruption 2021-09-09 12:55 [PATCH 0/2] perf tools: Fix hybrid config terms list corruption Adrian Hunter 2021-09-09 12:55 ` [PATCH 1/2] perf tools: Factor out copy_config_terms() and free_config_terms() Adrian Hunter 2021-09-09 12:55 ` [PATCH 2/2] perf tools: Fix hybrid config terms list corruption Adrian Hunter @ 2021-09-11 10:57 ` Jiri Olsa 2021-09-11 19:01 ` Arnaldo Carvalho de Melo 2 siblings, 1 reply; 7+ messages in thread From: Jiri Olsa @ 2021-09-11 10:57 UTC (permalink / raw) To: Adrian Hunter; +Cc: Arnaldo Carvalho de Melo, Jin Yao, Kan Liang, linux-kernel On Thu, Sep 09, 2021 at 03:55:06PM +0300, Adrian Hunter wrote: > Hi > > Here is a fix for an issue using perf on ADL. > > > Adrian Hunter (2): > perf tools: Factor out copy_config_terms() and free_config_terms() > perf tools: Fix hybrid config terms list corruption Acked-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka > > tools/perf/util/evsel.c | 20 +++++++++++++++----- > tools/perf/util/evsel.h | 3 +++ > tools/perf/util/parse-events-hybrid.c | 18 +++++++++++++++--- > tools/perf/util/parse-events.c | 27 +++++++++++++-------------- > 4 files changed, 46 insertions(+), 22 deletions(-) > > > Regards > Adrian > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] perf tools: Fix hybrid config terms list corruption 2021-09-11 10:57 ` [PATCH 0/2] " Jiri Olsa @ 2021-09-11 19:01 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 7+ messages in thread From: Arnaldo Carvalho de Melo @ 2021-09-11 19:01 UTC (permalink / raw) To: Jiri Olsa; +Cc: Adrian Hunter, Jin Yao, Kan Liang, linux-kernel Em Sat, Sep 11, 2021 at 12:57:14PM +0200, Jiri Olsa escreveu: > On Thu, Sep 09, 2021 at 03:55:06PM +0300, Adrian Hunter wrote: > > Hi > > > > Here is a fix for an issue using perf on ADL. > > > > > > Adrian Hunter (2): > > perf tools: Factor out copy_config_terms() and free_config_terms() > > perf tools: Fix hybrid config terms list corruption > > Acked-by: Jiri Olsa <jolsa@redhat.com> Thanks, applied. - Arnaldo ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-11 19:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-09 12:55 [PATCH 0/2] perf tools: Fix hybrid config terms list corruption Adrian Hunter 2021-09-09 12:55 ` [PATCH 1/2] perf tools: Factor out copy_config_terms() and free_config_terms() Adrian Hunter 2021-09-09 12:55 ` [PATCH 2/2] perf tools: Fix hybrid config terms list corruption Adrian Hunter 2021-09-10 19:32 ` Jiri Olsa 2021-09-11 6:23 ` Adrian Hunter 2021-09-11 10:57 ` [PATCH 0/2] " Jiri Olsa 2021-09-11 19:01 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).