* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.