* [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).