| Message ID | 20191030223448.12930-6-irogers@google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Improvements to memory usage by parse events | expand |
On Wed, Oct 30, 2019 at 03:34:43PM -0700, Ian Rogers wrote: > Make it easier to release memory associated with parse event terms by > duplicating the string for the config name and ensuring the val string > is a duplicate. > > Currently the parser may memory leak terms and this is addressed in a > later patch. > > Signed-off-by: Ian Rogers <irogers@google.com> Acked-by: Jiri Olsa <jolsa@kernel.org> thanks, jirka > --- > tools/perf/util/parse-events.c | 51 ++++++++++++++++++++++++++++------ > tools/perf/util/parse-events.y | 4 ++- > 2 files changed, 45 insertions(+), 10 deletions(-) > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index 03e54a2d8685..578288c94d2a 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -1412,7 +1412,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, > int parse_events_multi_pmu_add(struct parse_events_state *parse_state, > char *str, struct list_head **listp) > { > - struct list_head *head; > struct parse_events_term *term; > struct list_head *list; > struct perf_pmu *pmu = NULL; > @@ -1429,19 +1428,30 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, > > list_for_each_entry(alias, &pmu->aliases, list) { > if (!strcasecmp(alias->name, str)) { > + struct list_head *head; > + char *config; > + > head = malloc(sizeof(struct list_head)); > if (!head) > return -1; > INIT_LIST_HEAD(head); > - if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER, > - str, 1, false, &str, NULL) < 0) > + config = strdup(str); > + if (!config) > + return -1; > + if (parse_events_term__num(&term, > + PARSE_EVENTS__TERM_TYPE_USER, > + config, 1, false, &config, > + NULL) < 0) { > + free(list); > + free(config); > return -1; > + } > list_add_tail(&term->list, head); > > if (!parse_events_add_pmu(parse_state, list, > pmu->name, head, > true, true)) { > - pr_debug("%s -> %s/%s/\n", str, > + pr_debug("%s -> %s/%s/\n", config, > pmu->name, alias->str); > ok++; > } > @@ -1450,8 +1460,10 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, > } > } > } > - if (!ok) > + if (!ok) { > + free(list); > return -1; > + } > *listp = list; > return 0; > } > @@ -2746,30 +2758,51 @@ int parse_events_term__sym_hw(struct parse_events_term **term, > char *config, unsigned idx) > { > struct event_symbol *sym; > + char *str; > struct parse_events_term temp = { > .type_val = PARSE_EVENTS__TERM_TYPE_STR, > .type_term = PARSE_EVENTS__TERM_TYPE_USER, > - .config = config ?: (char *) "event", > + .config = config, > }; > > + if (!temp.config) { > + temp.config = strdup("event"); > + if (!temp.config) > + return -ENOMEM; > + } > BUG_ON(idx >= PERF_COUNT_HW_MAX); > sym = &event_symbols_hw[idx]; > > - return new_term(term, &temp, (char *) sym->symbol, 0); > + str = strdup(sym->symbol); > + if (!str) > + return -ENOMEM; > + return new_term(term, &temp, str, 0); > } > > int parse_events_term__clone(struct parse_events_term **new, > struct parse_events_term *term) > { > + char *str; > struct parse_events_term temp = { > .type_val = term->type_val, > .type_term = term->type_term, > - .config = term->config, > + .config = NULL, > .err_term = term->err_term, > .err_val = term->err_val, > }; > > - return new_term(new, &temp, term->val.str, term->val.num); > + if (term->config) { > + temp.config = strdup(term->config); > + if (!temp.config) > + return -ENOMEM; > + } > + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) > + return new_term(new, &temp, NULL, term->val.num); > + > + str = strdup(term->val.str); > + if (!str) > + return -ENOMEM; > + return new_term(new, &temp, str, 0); > } > > int parse_events_copy_term_list(struct list_head *old, > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > index ffa1a1b63796..545ab7cefc20 100644 > --- a/tools/perf/util/parse-events.y > +++ b/tools/perf/util/parse-events.y > @@ -665,9 +665,11 @@ PE_NAME array '=' PE_VALUE > PE_DRV_CFG_TERM > { > struct parse_events_term *term; > + char *config = strdup($1); > > + ABORT_ON(!config); > ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG, > - $1, $1, &@1, NULL)); > + config, $1, &@1, NULL)); > $$ = term; > } > > -- > 2.24.0.rc1.363.gb1bccd3e3d-goog >
Em Wed, Nov 06, 2019 at 03:25:03PM +0100, Jiri Olsa escreveu: > On Wed, Oct 30, 2019 at 03:34:43PM -0700, Ian Rogers wrote: > > Make it easier to release memory associated with parse event terms by > > duplicating the string for the config name and ensuring the val string > > is a duplicate. > > > > Currently the parser may memory leak terms and this is addressed in a > > later patch. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > Acked-by: Jiri Olsa <jolsa@kernel.org> Thanks, applied. - Arnaldo
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index 03e54a2d8685..578288c94d2a 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -1412,7 +1412,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, int parse_events_multi_pmu_add(struct parse_events_state *parse_state, char *str, struct list_head **listp) { - struct list_head *head; struct parse_events_term *term; struct list_head *list; struct perf_pmu *pmu = NULL; @@ -1429,19 +1428,30 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, list_for_each_entry(alias, &pmu->aliases, list) { if (!strcasecmp(alias->name, str)) { + struct list_head *head; + char *config; + head = malloc(sizeof(struct list_head)); if (!head) return -1; INIT_LIST_HEAD(head); - if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER, - str, 1, false, &str, NULL) < 0) + config = strdup(str); + if (!config) + return -1; + if (parse_events_term__num(&term, + PARSE_EVENTS__TERM_TYPE_USER, + config, 1, false, &config, + NULL) < 0) { + free(list); + free(config); return -1; + } list_add_tail(&term->list, head); if (!parse_events_add_pmu(parse_state, list, pmu->name, head, true, true)) { - pr_debug("%s -> %s/%s/\n", str, + pr_debug("%s -> %s/%s/\n", config, pmu->name, alias->str); ok++; } @@ -1450,8 +1460,10 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, } } } - if (!ok) + if (!ok) { + free(list); return -1; + } *listp = list; return 0; } @@ -2746,30 +2758,51 @@ int parse_events_term__sym_hw(struct parse_events_term **term, char *config, unsigned idx) { struct event_symbol *sym; + char *str; struct parse_events_term temp = { .type_val = PARSE_EVENTS__TERM_TYPE_STR, .type_term = PARSE_EVENTS__TERM_TYPE_USER, - .config = config ?: (char *) "event", + .config = config, }; + if (!temp.config) { + temp.config = strdup("event"); + if (!temp.config) + return -ENOMEM; + } BUG_ON(idx >= PERF_COUNT_HW_MAX); sym = &event_symbols_hw[idx]; - return new_term(term, &temp, (char *) sym->symbol, 0); + str = strdup(sym->symbol); + if (!str) + return -ENOMEM; + return new_term(term, &temp, str, 0); } int parse_events_term__clone(struct parse_events_term **new, struct parse_events_term *term) { + char *str; struct parse_events_term temp = { .type_val = term->type_val, .type_term = term->type_term, - .config = term->config, + .config = NULL, .err_term = term->err_term, .err_val = term->err_val, }; - return new_term(new, &temp, term->val.str, term->val.num); + if (term->config) { + temp.config = strdup(term->config); + if (!temp.config) + return -ENOMEM; + } + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) + return new_term(new, &temp, NULL, term->val.num); + + str = strdup(term->val.str); + if (!str) + return -ENOMEM; + return new_term(new, &temp, str, 0); } int parse_events_copy_term_list(struct list_head *old, diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y index ffa1a1b63796..545ab7cefc20 100644 --- a/tools/perf/util/parse-events.y +++ b/tools/perf/util/parse-events.y @@ -665,9 +665,11 @@ PE_NAME array '=' PE_VALUE PE_DRV_CFG_TERM { struct parse_events_term *term; + char *config = strdup($1); + ABORT_ON(!config); ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG, - $1, $1, &@1, NULL)); + config, $1, &@1, NULL)); $$ = term; }
Make it easier to release memory associated with parse event terms by duplicating the string for the config name and ensuring the val string is a duplicate. Currently the parser may memory leak terms and this is addressed in a later patch. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/parse-events.c | 51 ++++++++++++++++++++++++++++------ tools/perf/util/parse-events.y | 4 ++- 2 files changed, 45 insertions(+), 10 deletions(-)