From: Ian Rogers <irogers@google.com>
To: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
Ian Rogers <irogers@google.com>,
Adrian Hunter <adrian.hunter@intel.com>,
James Clark <james.clark@arm.com>,
Kan Liang <kan.liang@linux.intel.com>,
Rob Herring <robh@kernel.org>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH v1 5/6] perf parse-events: Copy fewer term lists
Date: Fri, 1 Sep 2023 16:39:48 -0700 [thread overview]
Message-ID: <20230901233949.2930562-5-irogers@google.com> (raw)
In-Reply-To: <20230901233949.2930562-1-irogers@google.com>
When trying to add events to multiple PMUs the term list is copied
first as adding the event will rewrite the event's name term into the
sysfs and/or json encoding terms (see perf_pmu__check_alias). Change
the parse events add API so the passed in term list is const, then
copy the list when modification is necessary.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/parse-events.c | 108 ++++++++++++++++++---------------
tools/perf/util/parse-events.h | 7 +--
tools/perf/util/parse-events.y | 17 +-----
3 files changed, 65 insertions(+), 67 deletions(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 283c559a35b4..06a844bcce4a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -35,6 +35,7 @@
extern int parse_events_debug;
#endif
static int get_config_terms(struct list_head *head_config, struct list_head *head_terms);
+static int parse_events_terms__copy(const struct list_head *src, struct list_head *dest);
struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
[PERF_COUNT_HW_CPU_CYCLES] = {
@@ -1367,7 +1368,7 @@ static bool config_term_percore(struct list_head *config_terms)
int parse_events_add_pmu(struct parse_events_state *parse_state,
struct list_head *list, const char *name,
- struct list_head *head_config,
+ const struct list_head *const_head_terms,
bool auto_merge_stats, void *loc_)
{
struct perf_event_attr attr;
@@ -1377,6 +1378,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
struct parse_events_error *err = parse_state->error;
YYLTYPE *loc = loc_;
LIST_HEAD(config_terms);
+ LIST_HEAD(head_terms);
pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
@@ -1390,32 +1392,37 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
return -EINVAL;
}
+ if (const_head_terms) {
+ int ret = parse_events_terms__copy(const_head_terms, &head_terms);
+
+ if (ret)
+ return ret;
+ }
+
if (verbose > 1) {
struct strbuf sb;
strbuf_init(&sb, /*hint=*/ 0);
- if (pmu->selectable && !head_config) {
+ if (pmu->selectable && list_empty(&head_terms)) {
strbuf_addf(&sb, "%s//", name);
} else {
strbuf_addf(&sb, "%s/", name);
- parse_events_term__to_strbuf(head_config, &sb);
+ parse_events_term__to_strbuf(&head_terms, &sb);
strbuf_addch(&sb, '/');
}
fprintf(stderr, "Attempt to add: %s\n", sb.buf);
strbuf_release(&sb);
}
- if (head_config)
- fix_raw(head_config, pmu);
+ fix_raw(&head_terms, pmu);
if (pmu->default_config) {
- memcpy(&attr, pmu->default_config,
- sizeof(struct perf_event_attr));
+ memcpy(&attr, pmu->default_config, sizeof(struct perf_event_attr));
} else {
memset(&attr, 0, sizeof(attr));
}
attr.type = pmu->type;
- if (!head_config) {
+ if (list_empty(&head_terms)) {
evsel = __add_event(list, &parse_state->idx, &attr,
/*init_attr=*/true, /*name=*/NULL,
/*metric_id=*/NULL, pmu,
@@ -1424,14 +1431,16 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
return evsel ? 0 : -ENOMEM;
}
- if (!parse_state->fake_pmu && perf_pmu__check_alias(pmu, head_config, &info, err))
+ if (!parse_state->fake_pmu && perf_pmu__check_alias(pmu, &head_terms, &info, err)) {
+ parse_events_terms__purge(&head_terms);
return -EINVAL;
+ }
if (verbose > 1) {
struct strbuf sb;
strbuf_init(&sb, /*hint=*/ 0);
- parse_events_term__to_strbuf(head_config, &sb);
+ parse_events_term__to_strbuf(&head_terms, &sb);
fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
strbuf_release(&sb);
}
@@ -1440,39 +1449,52 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
* Configure hardcoded terms first, no need to check
* return value when called with fail == 0 ;)
*/
- if (config_attr(&attr, head_config, parse_state->error, config_term_pmu))
+ if (config_attr(&attr, &head_terms, parse_state->error, config_term_pmu)) {
+ parse_events_terms__purge(&head_terms);
return -EINVAL;
+ }
- if (get_config_terms(head_config, &config_terms))
+ if (get_config_terms(&head_terms, &config_terms)) {
+ parse_events_terms__purge(&head_terms);
return -ENOMEM;
+ }
/*
* When using default config, record which bits of attr->config were
* changed by the user.
*/
- if (pmu->default_config && get_config_chgs(pmu, head_config, &config_terms))
+ if (pmu->default_config && get_config_chgs(pmu, &head_terms, &config_terms)) {
+ parse_events_terms__purge(&head_terms);
return -ENOMEM;
+ }
- if (!parse_state->fake_pmu && perf_pmu__config(pmu, &attr, head_config, parse_state->error)) {
+ if (!parse_state->fake_pmu &&
+ perf_pmu__config(pmu, &attr, &head_terms, parse_state->error)) {
free_config_terms(&config_terms);
+ parse_events_terms__purge(&head_terms);
return -EINVAL;
}
evsel = __add_event(list, &parse_state->idx, &attr, /*init_attr=*/true,
- get_config_name(head_config),
- get_config_metric_id(head_config), pmu,
+ get_config_name(&head_terms),
+ get_config_metric_id(&head_terms), pmu,
&config_terms, auto_merge_stats, /*cpu_list=*/NULL);
- if (!evsel)
+ if (!evsel) {
+ parse_events_terms__purge(&head_terms);
return -ENOMEM;
+ }
if (evsel->name)
evsel->use_config_name = true;
evsel->percore = config_term_percore(&evsel->config_terms);
- if (parse_state->fake_pmu)
+ if (parse_state->fake_pmu) {
+ parse_events_terms__purge(&head_terms);
return 0;
+ }
+ parse_events_terms__purge(&head_terms);
free((char *)evsel->unit);
evsel->unit = strdup(info.unit);
evsel->scale = info.scale;
@@ -1482,25 +1504,25 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
}
int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
- const char *event_name, struct list_head *head,
+ const char *event_name,
+ const struct list_head *const_head_terms,
struct list_head **listp, void *loc_)
{
struct parse_events_term *term;
struct list_head *list = NULL;
- struct list_head *orig_head = NULL;
struct perf_pmu *pmu = NULL;
YYLTYPE *loc = loc_;
int ok = 0;
const char *config;
+ LIST_HEAD(head_terms);
*listp = NULL;
- if (!head) {
- head = malloc(sizeof(struct list_head));
- if (!head)
- goto out_err;
+ if (const_head_terms) {
+ int ret = parse_events_terms__copy(const_head_terms, &head_terms);
- INIT_LIST_HEAD(head);
+ if (ret)
+ return ret;
}
config = strdup(event_name);
@@ -1514,7 +1536,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
zfree(&config);
goto out_err;
}
- list_add_tail(&term->list, head);
+ list_add_tail(&term->list, &head_terms);
/* Add it for all PMUs that support the alias */
list = malloc(sizeof(struct list_head));
@@ -1533,27 +1555,25 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
continue;
auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
- parse_events_copy_term_list(head, &orig_head);
if (!parse_events_add_pmu(parse_state, list, pmu->name,
- orig_head, auto_merge_stats, loc)) {
+ &head_terms, auto_merge_stats, loc)) {
struct strbuf sb;
strbuf_init(&sb, /*hint=*/ 0);
- parse_events_term__to_strbuf(orig_head, &sb);
+ parse_events_term__to_strbuf(&head_terms, &sb);
pr_debug("%s -> %s/%s/\n", event_name, pmu->name, sb.buf);
strbuf_release(&sb);
ok++;
}
- parse_events_terms__delete(orig_head);
}
if (parse_state->fake_pmu) {
- if (!parse_events_add_pmu(parse_state, list, event_name, head,
+ if (!parse_events_add_pmu(parse_state, list, event_name, &head_terms,
/*auto_merge_stats=*/true, loc)) {
struct strbuf sb;
strbuf_init(&sb, /*hint=*/ 0);
- parse_events_term__to_strbuf(head, &sb);
+ parse_events_term__to_strbuf(&head_terms, &sb);
pr_debug("%s -> %s/%s/\n", event_name, "fake_pmu", sb.buf);
strbuf_release(&sb);
ok++;
@@ -1561,12 +1581,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
}
out_err:
+ parse_events_terms__purge(&head_terms);
if (ok)
*listp = list;
else
free(list);
- parse_events_terms__delete(head);
return ok ? 0 : -1;
}
@@ -2543,27 +2563,19 @@ void parse_events_term__delete(struct parse_events_term *term)
free(term);
}
-int parse_events_copy_term_list(struct list_head *old,
- struct list_head **new)
+static int parse_events_terms__copy(const struct list_head *src, struct list_head *dest)
{
- struct parse_events_term *term, *n;
- int ret;
-
- if (!old) {
- *new = NULL;
- return 0;
- }
+ struct parse_events_term *term;
- *new = malloc(sizeof(struct list_head));
- if (!*new)
- return -ENOMEM;
- INIT_LIST_HEAD(*new);
+ list_for_each_entry (term, src, list) {
+ struct parse_events_term *n;
+ int ret;
- list_for_each_entry (term, old, list) {
ret = parse_events_term__clone(&n, term);
if (ret)
return ret;
- list_add_tail(&n->list, *new);
+
+ list_add_tail(&n->list, dest);
}
return 0;
}
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 36a67ef7b35a..e6612856e881 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -209,7 +209,7 @@ int parse_events_add_breakpoint(struct parse_events_state *parse_state,
struct list_head *head_config);
int parse_events_add_pmu(struct parse_events_state *parse_state,
struct list_head *list, const char *name,
- struct list_head *head_config,
+ const struct list_head *const_head_terms,
bool auto_merge_stats, void *loc);
struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
@@ -218,12 +218,9 @@ struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
const char *event_name,
- struct list_head *head_config,
+ const struct list_head *head_terms,
struct list_head **listp, void *loc);
-int parse_events_copy_term_list(struct list_head *old,
- struct list_head **new);
-
void parse_events__set_leader(char *name, struct list_head *list);
void parse_events_update_lists(struct list_head *list_event,
struct list_head *list_all);
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 4fae7847d13b..1ee65f36762c 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -274,23 +274,18 @@ event_pmu:
PE_NAME opt_pmu_config
{
struct parse_events_state *parse_state = _parse_state;
- struct list_head *list = NULL, *orig_terms = NULL, *terms= NULL;
+ /* List of created evsels. */
+ struct list_head *list = NULL;
char *pattern = NULL;
#define CLEANUP \
do { \
parse_events_terms__delete($2); \
- parse_events_terms__delete(orig_terms); \
free(list); \
free($1); \
free(pattern); \
} while(0)
- if (parse_events_copy_term_list($2, &orig_terms)) {
- CLEANUP;
- YYNOMEM;
- }
-
list = alloc_list();
if (!list) {
CLEANUP;
@@ -320,16 +315,11 @@ PE_NAME opt_pmu_config
!perf_pmu__match(pattern, pmu->alias_name, $1)) {
bool auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
- if (parse_events_copy_term_list(orig_terms, &terms)) {
- CLEANUP;
- YYNOMEM;
- }
- if (!parse_events_add_pmu(parse_state, list, pmu->name, terms,
+ if (!parse_events_add_pmu(parse_state, list, pmu->name, $2,
auto_merge_stats, &@1)) {
ok++;
parse_state->wild_card_pmus = true;
}
- parse_events_terms__delete(terms);
}
}
@@ -337,7 +327,6 @@ PE_NAME opt_pmu_config
/* Failure to add, assume $1 is an event name. */
zfree(&list);
ok = !parse_events_multi_pmu_add(parse_state, $1, $2, &list, &@1);
- $2 = NULL;
}
if (!ok) {
struct parse_events_error *error = parse_state->error;
--
2.42.0.283.g2d96d420d3-goog
next prev parent reply other threads:[~2023-09-01 23:40 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-01 23:39 [PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms Ian Rogers
2023-09-01 23:39 ` [PATCH v1 2/6] perf parse-events: Remove unnecessary __maybe_unused Ian Rogers
2023-09-01 23:39 ` [PATCH v1 3/6] perf parse-events: Tidy up str parameter Ian Rogers
2023-09-01 23:39 ` [PATCH v1 4/6] perf parse-events: Avoid enum casts Ian Rogers
2023-09-01 23:39 ` Ian Rogers [this message]
2023-09-01 23:39 ` [PATCH v1 6/6] perf parse-events: Add struct parse_events_terms Ian Rogers
2023-09-02 11:11 ` [PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms Arnaldo Carvalho de Melo
2023-09-02 16:28 ` Ian Rogers
2023-09-05 9:39 ` James Clark
2023-09-06 15:48 ` Arnaldo Carvalho de Melo
2023-09-06 15:51 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230901233949.2930562-5-irogers@google.com \
--to=irogers@google.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=james.clark@arm.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).