Messages in this thread Patch in this message |  | | From | Mathieu Poirier <> | Date | Tue, 7 Jan 2020 14:45:27 -0700 | Subject | Re: [PATCH v3] perf parse: Copy string to perf_evsel_config_term |
| |
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 >
|  |