* [PATCH v3] perf parse: Copy string to perf_evsel_config_term
@ 2020-01-07 12:03 Leo Yan
2020-01-07 21:45 ` Mathieu Poirier
0 siblings, 1 reply; 4+ messages in thread
From: Leo Yan @ 2020-01-07 12:03 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Ian Rogers,
Andi Kleen, Adrian Hunter, linux-kernel, Mathieu Poirier,
Mike Leach, Suzuki K Poulose
Cc: Leo Yan
perf with CoreSight fails to record trace data with command:
perf record -e cs_etm/@tmc_etr0/u --per-thread ls
failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a
directory)/perf/
This failure is root caused with the commit 1dc925568f01 ("perf
parse: Add a deep delete for parse event terms").
The log shows, cs_etm fails to parse the sink attribution; cs_etm event
relies on the event configuration to pass sink name, but the event
specific configuration data cannot be passed properly with flow:
get_config_terms()
ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
__t->val.drv_cfg = term->val.str;
`> __t->val.drv_cfg is assigned to term->val.str;
parse_events_terms__purge()
parse_events_term__delete()
zfree(&term->val.str);
`> term->val.str is freed and assigned to NULL pointer;
cs_etm_set_sink_attr()
sink = __t->val.drv_cfg;
`> sink string has been freed.
To fix this issue, in the function get_config_terms(), this patch
changes from directly assignment pointer value for the strings to
use strdup() for allocation a new duplicate string for the cases:
perf_evsel_config_term::val.callgraph
perf_evsel_config_term::val.branch
perf_evsel_config_term::val.drv_cfg.
In the data structure perf_evsel_config_term, this patch adds
'char *str' pointer in the val union and new field 'free_str'. When the
union is used as a string pointer, 'free_str' will be set to true;
finally it's flag to tell perf_evsel__free_config_terms() to free the
string with perf_evsel_config_term::val.str.
Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
Suggested-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
tools/perf/util/evsel.c | 2 ++
tools/perf/util/evsel_config.h | 2 ++
tools/perf/util/parse-events.c | 56 +++++++++++++++++++++-------------
3 files changed, 39 insertions(+), 21 deletions(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a69e64236120..ab9925cc1aa7 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
list_del_init(&term->list);
+ if (term->free_str)
+ free(term->val.str);
free(term);
}
}
diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
index 1f8d2fe0b66e..dfc28738e071 100644
--- a/tools/perf/util/evsel_config.h
+++ b/tools/perf/util/evsel_config.h
@@ -32,6 +32,7 @@ enum evsel_term_type {
struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
+ bool free_str;
union {
u64 period;
u64 freq;
@@ -48,6 +49,7 @@ struct perf_evsel_config_term {
bool aux_output;
u32 aux_sample_size;
u64 cfg_chg;
+ char *str;
} val;
bool weak;
};
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index ed7c008b9c8b..83fb149b9485 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1220,7 +1220,6 @@ static int get_config_terms(struct list_head *head_config,
struct list_head *head_terms __maybe_unused)
{
#define ADD_CONFIG_TERM(__type, __name, __val) \
-do { \
struct perf_evsel_config_term *__t; \
\
__t = zalloc(sizeof(*__t)); \
@@ -1229,9 +1228,24 @@ do { \
\
INIT_LIST_HEAD(&__t->list); \
__t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
- __t->val.__name = __val; \
__t->weak = term->weak; \
- list_add_tail(&__t->list, head_terms); \
+ list_add_tail(&__t->list, head_terms)
+
+#define ADD_CONFIG_TERM_VAL(__type, __name, __val) \
+do { \
+ ADD_CONFIG_TERM(__type, __name, __val); \
+ __t->val.__name = __val; \
+} while (0)
+
+#define ADD_CONFIG_TERM_STR(__type, __name, __val) \
+do { \
+ ADD_CONFIG_TERM(__type, __name, __val); \
+ __t->val.__name = strdup(__val); \
+ if (!__t->val.__name) { \
+ zfree(&__t); \
+ return -ENOMEM; \
+ } \
+ __t->free_str = true; \
} while (0)
struct parse_events_term *term;
@@ -1239,53 +1253,53 @@ do { \
list_for_each_entry(term, head_config, list) {
switch (term->type_term) {
case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
- ADD_CONFIG_TERM(PERIOD, period, term->val.num);
+ ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
- ADD_CONFIG_TERM(FREQ, freq, term->val.num);
+ ADD_CONFIG_TERM_VAL(FREQ, freq, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_TIME:
- ADD_CONFIG_TERM(TIME, time, term->val.num);
+ ADD_CONFIG_TERM_VAL(TIME, time, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
- ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
+ ADD_CONFIG_TERM_STR(CALLGRAPH, callgraph, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
- ADD_CONFIG_TERM(BRANCH, branch, term->val.str);
+ ADD_CONFIG_TERM_STR(BRANCH, branch, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
- ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
+ ADD_CONFIG_TERM_VAL(STACK_USER, stack_user, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_INHERIT:
- ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(INHERIT, inherit, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
- ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
+ ADD_CONFIG_TERM_VAL(INHERIT, inherit, term->val.num ? 0 : 1);
break;
case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
- ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
+ ADD_CONFIG_TERM_VAL(MAX_STACK, max_stack, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
- ADD_CONFIG_TERM(MAX_EVENTS, max_events, term->val.num);
+ ADD_CONFIG_TERM_VAL(MAX_EVENTS, max_events, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
- ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
- ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
+ ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, term->val.num ? 0 : 1);
break;
case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
- ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
+ ADD_CONFIG_TERM_STR(DRV_CFG, drv_cfg, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_PERCORE:
- ADD_CONFIG_TERM(PERCORE, percore,
- term->val.num ? true : false);
+ ADD_CONFIG_TERM_VAL(PERCORE, percore,
+ term->val.num ? true : false);
break;
case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
- ADD_CONFIG_TERM(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
- ADD_CONFIG_TERM(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
+ ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
break;
default:
break;
@@ -1322,7 +1336,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
}
if (bits)
- ADD_CONFIG_TERM(CFG_CHG, cfg_chg, bits);
+ ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits);
#undef ADD_CONFIG_TERM
return 0;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] perf parse: Copy string to perf_evsel_config_term
2020-01-07 12:03 [PATCH v3] perf parse: Copy string to perf_evsel_config_term Leo Yan
@ 2020-01-07 21:45 ` Mathieu Poirier
2020-01-08 10:22 ` Jiri Olsa
0 siblings, 1 reply; 4+ messages in thread
From: Mathieu Poirier @ 2020-01-07 21:45 UTC (permalink / raw)
To: Leo Yan
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Ian Rogers,
Andi Kleen, Adrian Hunter, Linux Kernel Mailing List, Mike Leach,
Suzuki K Poulose
Hi Leo,
On Tue, 7 Jan 2020 at 05:03, Leo Yan <leo.yan@linaro.org> wrote:
>
> perf with CoreSight fails to record trace data with command:
>
> perf record -e cs_etm/@tmc_etr0/u --per-thread ls
> failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a
> directory)/perf/
>
> This failure is root caused with the commit 1dc925568f01 ("perf
> parse: Add a deep delete for parse event terms").
>
> The log shows, cs_etm fails to parse the sink attribution; cs_etm event
> relies on the event configuration to pass sink name, but the event
> specific configuration data cannot be passed properly with flow:
>
> get_config_terms()
> ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> __t->val.drv_cfg = term->val.str;
> `> __t->val.drv_cfg is assigned to term->val.str;
>
> parse_events_terms__purge()
> parse_events_term__delete()
> zfree(&term->val.str);
> `> term->val.str is freed and assigned to NULL pointer;
>
> cs_etm_set_sink_attr()
> sink = __t->val.drv_cfg;
> `> sink string has been freed.
>
> To fix this issue, in the function get_config_terms(), this patch
> changes from directly assignment pointer value for the strings to
> use strdup() for allocation a new duplicate string for the cases:
>
> perf_evsel_config_term::val.callgraph
> perf_evsel_config_term::val.branch
> perf_evsel_config_term::val.drv_cfg.
>
> In the data structure perf_evsel_config_term, this patch adds
> 'char *str' pointer in the val union and new field 'free_str'. When the
> union is used as a string pointer, 'free_str' will be set to true;
> finally it's flag to tell perf_evsel__free_config_terms() to free the
> string with perf_evsel_config_term::val.str.
Many thanks for digging into this and stepping forward to provide a
solution - it is much appreciated.
>
> Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> Suggested-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/util/evsel.c | 2 ++
> tools/perf/util/evsel_config.h | 2 ++
> tools/perf/util/parse-events.c | 56 +++++++++++++++++++++-------------
> 3 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a69e64236120..ab9925cc1aa7 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
>
> list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
> list_del_init(&term->list);
> + if (term->free_str)
> + free(term->val.str);
This will do the trick but we can definitely do better.
Part of his comments on V2, Jiri hinted that we should move to a
common perf_evsel_config_term::str to replace {callgraph, drv_cfg,
branch}, something that will work because we have
perf_evsel_config_term::type. That means functions
apply_config_terms() and cs_etm_set_sink_attr() need to be modified
but the changes are quite small and well worth for the benefit they'll
carry.
With that the above becomes neat and clean.
Thanks,
Mathieu
> free(term);
> }
> }
> diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> index 1f8d2fe0b66e..dfc28738e071 100644
> --- a/tools/perf/util/evsel_config.h
> +++ b/tools/perf/util/evsel_config.h
> @@ -32,6 +32,7 @@ enum evsel_term_type {
> struct perf_evsel_config_term {
> struct list_head list;
> enum evsel_term_type type;
> + bool free_str;
> union {
> u64 period;
> u64 freq;
> @@ -48,6 +49,7 @@ struct perf_evsel_config_term {
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> + char *str;
> } val;
> bool weak;
> };
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index ed7c008b9c8b..83fb149b9485 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1220,7 +1220,6 @@ static int get_config_terms(struct list_head *head_config,
> struct list_head *head_terms __maybe_unused)
> {
> #define ADD_CONFIG_TERM(__type, __name, __val) \
> -do { \
> struct perf_evsel_config_term *__t; \
> \
> __t = zalloc(sizeof(*__t)); \
> @@ -1229,9 +1228,24 @@ do { \
> \
> INIT_LIST_HEAD(&__t->list); \
> __t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
> - __t->val.__name = __val; \
> __t->weak = term->weak; \
> - list_add_tail(&__t->list, head_terms); \
> + list_add_tail(&__t->list, head_terms)
> +
> +#define ADD_CONFIG_TERM_VAL(__type, __name, __val) \
> +do { \
> + ADD_CONFIG_TERM(__type, __name, __val); \
> + __t->val.__name = __val; \
> +} while (0)
> +
> +#define ADD_CONFIG_TERM_STR(__type, __name, __val) \
> +do { \
> + ADD_CONFIG_TERM(__type, __name, __val); \
> + __t->val.__name = strdup(__val); \
> + if (!__t->val.__name) { \
> + zfree(&__t); \
> + return -ENOMEM; \
> + } \
> + __t->free_str = true; \
> } while (0)
>
> struct parse_events_term *term;
> @@ -1239,53 +1253,53 @@ do { \
> list_for_each_entry(term, head_config, list) {
> switch (term->type_term) {
> case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> - ADD_CONFIG_TERM(PERIOD, period, term->val.num);
> + ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
> - ADD_CONFIG_TERM(FREQ, freq, term->val.num);
> + ADD_CONFIG_TERM_VAL(FREQ, freq, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_TIME:
> - ADD_CONFIG_TERM(TIME, time, term->val.num);
> + ADD_CONFIG_TERM_VAL(TIME, time, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
> - ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
> + ADD_CONFIG_TERM_STR(CALLGRAPH, callgraph, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
> - ADD_CONFIG_TERM(BRANCH, branch, term->val.str);
> + ADD_CONFIG_TERM_STR(BRANCH, branch, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
> - ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
> + ADD_CONFIG_TERM_VAL(STACK_USER, stack_user, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_INHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(INHERIT, inherit, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
> + ADD_CONFIG_TERM_VAL(INHERIT, inherit, term->val.num ? 0 : 1);
> break;
> case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> - ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
> + ADD_CONFIG_TERM_VAL(MAX_STACK, max_stack, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
> - ADD_CONFIG_TERM(MAX_EVENTS, max_events, term->val.num);
> + ADD_CONFIG_TERM_VAL(MAX_EVENTS, max_events, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
> + ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, term->val.num ? 0 : 1);
> break;
> case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
> - ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> + ADD_CONFIG_TERM_STR(DRV_CFG, drv_cfg, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_PERCORE:
> - ADD_CONFIG_TERM(PERCORE, percore,
> - term->val.num ? true : false);
> + ADD_CONFIG_TERM_VAL(PERCORE, percore,
> + term->val.num ? true : false);
> break;
> case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
> - ADD_CONFIG_TERM(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
> - ADD_CONFIG_TERM(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
> + ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
> break;
> default:
> break;
> @@ -1322,7 +1336,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
> }
>
> if (bits)
> - ADD_CONFIG_TERM(CFG_CHG, cfg_chg, bits);
> + ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits);
>
> #undef ADD_CONFIG_TERM
> return 0;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] perf parse: Copy string to perf_evsel_config_term
2020-01-07 21:45 ` Mathieu Poirier
@ 2020-01-08 10:22 ` Jiri Olsa
2020-01-08 13:20 ` Leo Yan
0 siblings, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2020-01-08 10:22 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Leo Yan, Arnaldo Carvalho de Melo, Jiri Olsa, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim,
Ian Rogers, Andi Kleen, Adrian Hunter, Linux Kernel Mailing List,
Mike Leach, Suzuki K Poulose
On Tue, Jan 07, 2020 at 02:45:27PM -0700, Mathieu Poirier wrote:
> Hi Leo,
>
> On Tue, 7 Jan 2020 at 05:03, Leo Yan <leo.yan@linaro.org> wrote:
> >
> > perf with CoreSight fails to record trace data with command:
> >
> > perf record -e cs_etm/@tmc_etr0/u --per-thread ls
> > failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a
> > directory)/perf/
> >
> > This failure is root caused with the commit 1dc925568f01 ("perf
> > parse: Add a deep delete for parse event terms").
> >
> > The log shows, cs_etm fails to parse the sink attribution; cs_etm event
> > relies on the event configuration to pass sink name, but the event
> > specific configuration data cannot be passed properly with flow:
> >
> > get_config_terms()
> > ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> > __t->val.drv_cfg = term->val.str;
> > `> __t->val.drv_cfg is assigned to term->val.str;
> >
> > parse_events_terms__purge()
> > parse_events_term__delete()
> > zfree(&term->val.str);
> > `> term->val.str is freed and assigned to NULL pointer;
> >
> > cs_etm_set_sink_attr()
> > sink = __t->val.drv_cfg;
> > `> sink string has been freed.
> >
> > To fix this issue, in the function get_config_terms(), this patch
> > changes from directly assignment pointer value for the strings to
> > use strdup() for allocation a new duplicate string for the cases:
> >
> > perf_evsel_config_term::val.callgraph
> > perf_evsel_config_term::val.branch
> > perf_evsel_config_term::val.drv_cfg.
> >
> > In the data structure perf_evsel_config_term, this patch adds
> > 'char *str' pointer in the val union and new field 'free_str'. When the
> > union is used as a string pointer, 'free_str' will be set to true;
> > finally it's flag to tell perf_evsel__free_config_terms() to free the
> > string with perf_evsel_config_term::val.str.
>
> Many thanks for digging into this and stepping forward to provide a
> solution - it is much appreciated.
>
> >
> > Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/perf/util/evsel.c | 2 ++
> > tools/perf/util/evsel_config.h | 2 ++
> > tools/perf/util/parse-events.c | 56 +++++++++++++++++++++-------------
> > 3 files changed, 39 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index a69e64236120..ab9925cc1aa7 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
> >
> > list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
> > list_del_init(&term->list);
> > + if (term->free_str)
> > + free(term->val.str);
>
> This will do the trick but we can definitely do better.
>
> Part of his comments on V2, Jiri hinted that we should move to a
> common perf_evsel_config_term::str to replace {callgraph, drv_cfg,
> branch}, something that will work because we have
> perf_evsel_config_term::type. That means functions
> apply_config_terms() and cs_etm_set_sink_attr() need to be modified
> but the changes are quite small and well worth for the benefit they'll
> carry.
>
> With that the above becomes neat and clean.
I wonder if there was some reason for keeping the variables
like that for every type and not just one per type as we did
'struct parse_events_term'
if the change is possible, the code would be cleaner, let's see ;-)
thanks,
jirka
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] perf parse: Copy string to perf_evsel_config_term
2020-01-08 10:22 ` Jiri Olsa
@ 2020-01-08 13:20 ` Leo Yan
0 siblings, 0 replies; 4+ messages in thread
From: Leo Yan @ 2020-01-08 13:20 UTC (permalink / raw)
To: Jiri Olsa
Cc: Mathieu Poirier, Arnaldo Carvalho de Melo, Jiri Olsa,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Namhyung Kim, Ian Rogers, Andi Kleen, Adrian Hunter,
Linux Kernel Mailing List, Mike Leach, Suzuki K Poulose
Hi Mathieu, Jiri,
On Wed, Jan 08, 2020 at 11:22:12AM +0100, Jiri Olsa wrote:
> On Tue, Jan 07, 2020 at 02:45:27PM -0700, Mathieu Poirier wrote:
[...]
> > Many thanks for digging into this and stepping forward to provide a
> > solution - it is much appreciated.
My pleasure!
After think again, we do need to add CoreSight test into perf test
ASAP, thus we can pay attention for the failure at the early time.
This is another topic, let's fistly focus on resolve this issue.
> > > Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
> > > Suggested-by: Jiri Olsa <jolsa@kernel.org>
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > > tools/perf/util/evsel.c | 2 ++
> > > tools/perf/util/evsel_config.h | 2 ++
> > > tools/perf/util/parse-events.c | 56 +++++++++++++++++++++-------------
> > > 3 files changed, 39 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index a69e64236120..ab9925cc1aa7 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
> > >
> > > list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
> > > list_del_init(&term->list);
> > > + if (term->free_str)
> > > + free(term->val.str);
> >
> > This will do the trick but we can definitely do better.
> >
> > Part of his comments on V2, Jiri hinted that we should move to a
> > common perf_evsel_config_term::str to replace {callgraph, drv_cfg,
> > branch}, something that will work because we have
> > perf_evsel_config_term::type. That means functions
> > apply_config_terms() and cs_etm_set_sink_attr() need to be modified
> > but the changes are quite small and well worth for the benefit they'll
> > carry.
> >
> > With that the above becomes neat and clean.
>
> I wonder if there was some reason for keeping the variables
> like that for every type and not just one per type as we did
> 'struct parse_events_term'
>
> if the change is possible, the code would be cleaner, let's see ;-)
Thanks for the suggestion, I hope to do right thing in next spin :)
I tried to only use val.num and val.str in perf_evsel_config_term in
my local code, same with the struct parse_events_term, it does work and
the effort is not big. Will send out patches soon (will use two
patches, one is for refactoring, another is for fixing regression).
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-08 13:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-07 12:03 [PATCH v3] perf parse: Copy string to perf_evsel_config_term Leo Yan
2020-01-07 21:45 ` Mathieu Poirier
2020-01-08 10:22 ` Jiri Olsa
2020-01-08 13:20 ` Leo Yan
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).