* [PATCH v1 1/3] perf parse-events: Minor help message improvements
@ 2023-08-30 7:07 Ian Rogers
2023-08-30 7:07 ` [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper Ian Rogers
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Ian Rogers @ 2023-08-30 7:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Kan Liang, James Clark,
linux-perf-users, linux-kernel
Be more specific and fix a typo.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/parse-events.y | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 3a9d4e2513b5..4a370c36a0d5 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -342,7 +342,7 @@ PE_NAME opt_pmu_config
struct parse_events_error *error = parse_state->error;
char *help;
- if (asprintf(&help, "Unabled to find PMU or event on a PMU of '%s'", $1) < 0)
+ if (asprintf(&help, "Unable to find PMU or event on a PMU of '%s'", $1) < 0)
help = NULL;
parse_events_error__handle(error, @1.first_column,
strdup("Bad event or PMU"),
@@ -368,7 +368,7 @@ PE_NAME sep_dc
struct parse_events_error *error = parse_state->error;
char *help;
- if (asprintf(&help, "Unabled to find PMU or event on a PMU of '%s'", $1) < 0)
+ if (asprintf(&help, "Unable to find event on a PMU of '%s'", $1) < 0)
help = NULL;
parse_events_error__handle(error, @1.first_column, strdup("Bad event name"), help);
free($1);
--
2.42.0.rc2.253.gd59a3bf2b4-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper
2023-08-30 7:07 [PATCH v1 1/3] perf parse-events: Minor help message improvements Ian Rogers
@ 2023-08-30 7:07 ` Ian Rogers
2023-08-30 14:11 ` Liang, Kan
2023-08-30 7:07 ` [PATCH v1 3/3] perf pmu: Remove str from perf_pmu_alias Ian Rogers
2023-08-31 18:32 ` [PATCH v1 1/3] perf parse-events: Minor help message improvements Liang, Kan
2 siblings, 1 reply; 10+ messages in thread
From: Ian Rogers @ 2023-08-30 7:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Kan Liang, James Clark,
linux-perf-users, linux-kernel
A term list is turned into a string for debug output and for the str
value in the alias. Add a helper to do this based on existing code,
but then fix for situations like events being identified. Use strbuf
to manage the dynamic memory allocation and remove the 256 byte
limit. Use in various places the string of the term list is required.
Before:
```
$ sudo perf stat -vv -e inst_retired.any true
Using CPUID GenuineIntel-6-8D-1
intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
Attempting to add event pmu 'cpu' with 'inst_retired.any,' that may result in non-fatal errors
After aliases, add event pmu 'cpu' with 'event,period,' that may result in non-fatal errors
inst_retired.any -> cpu/inst_retired.any/
...
```
After:
```
$ sudo perf stat -vv -e inst_retired.any true
Using CPUID GenuineIntel-6-8D-1
intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
Attempt to add: cpu/inst_retired.any/
..after resolving event: cpu/event=0xc0,period=0x1e8483/
inst_retired.any -> cpu/event=0xc0,period=0x1e8483/
...
```
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/parse-events.c | 101 ++++++++++++++++++++++++---------
tools/perf/util/parse-events.h | 2 +
tools/perf/util/pmu.c | 19 ++-----
3 files changed, 81 insertions(+), 41 deletions(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 4c812fbe0cf9..0b941b58bdc0 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -13,7 +13,7 @@
#include <subcmd/parse-options.h>
#include "parse-events.h"
#include "string2.h"
-#include "strlist.h"
+#include "strbuf.h"
#include "debug.h"
#include <api/fs/tracing_path.h>
#include <perf/cpumap.h>
@@ -1303,19 +1303,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
- if (verbose > 1 && !(pmu && pmu->selectable)) {
- fprintf(stderr, "Attempting to add event pmu '%s' with '",
- name);
- if (head_config) {
- struct parse_events_term *term;
-
- list_for_each_entry(term, head_config, list) {
- fprintf(stderr, "%s,", term->config);
- }
- }
- fprintf(stderr, "' that may result in non-fatal errors\n");
- }
-
if (!pmu) {
char *err_str;
@@ -1325,6 +1312,21 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
parse_events_error__handle(err, loc->first_column, err_str, NULL);
return -EINVAL;
}
+
+ if (verbose > 1) {
+ struct strbuf sb;
+
+ strbuf_init(&sb, /*hint=*/ 0);
+ if (pmu->selectable && !head_config) {
+ strbuf_addf(&sb, "%s//", name);
+ } else {
+ strbuf_addf(&sb, "%s/", name);
+ parse_events_term__to_strbuf(head_config, &sb);
+ strbuf_addch(&sb, '/');
+ }
+ fprintf(stderr, "Attempt to add: %s\n", sb.buf);
+ strbuf_release(&sb);
+ }
if (head_config)
fix_raw(head_config, pmu);
@@ -1349,16 +1351,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
return -EINVAL;
if (verbose > 1) {
- fprintf(stderr, "After aliases, add event pmu '%s' with '",
- name);
- if (head_config) {
- struct parse_events_term *term;
+ struct strbuf sb;
- list_for_each_entry(term, head_config, list) {
- fprintf(stderr, "%s,", term->config);
- }
- }
- fprintf(stderr, "' that may result in non-fatal errors\n");
+ strbuf_init(&sb, /*hint=*/ 0);
+ parse_events_term__to_strbuf(head_config, &sb);
+ fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
+ strbuf_release(&sb);
}
/*
@@ -1460,7 +1458,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
parse_events_copy_term_list(head, &orig_head);
if (!parse_events_add_pmu(parse_state, list, pmu->name,
orig_head, auto_merge_stats, loc)) {
- pr_debug("%s -> %s/%s/\n", str, pmu->name, str);
+ struct strbuf sb;
+
+ strbuf_init(&sb, /*hint=*/ 0);
+ parse_events_term__to_strbuf(orig_head, &sb);
+ pr_debug("%s -> %s/%s/\n", str, pmu->name, sb.buf);
+ strbuf_release(&sb);
ok++;
}
parse_events_terms__delete(orig_head);
@@ -1469,7 +1472,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
if (parse_state->fake_pmu) {
if (!parse_events_add_pmu(parse_state, list, str, head,
/*auto_merge_stats=*/true, loc)) {
- pr_debug("%s -> %s/%s/\n", str, "fake_pmu", str);
+ struct strbuf sb;
+
+ strbuf_init(&sb, /*hint=*/ 0);
+ parse_events_term__to_strbuf(head, &sb);
+ pr_debug("%s -> %s/%s/\n", str, "fake_pmu", sb.buf);
+ strbuf_release(&sb);
ok++;
}
}
@@ -2085,7 +2093,7 @@ void parse_events_error__handle(struct parse_events_error *err, int idx,
break;
default:
pr_debug("Multiple errors dropping message: %s (%s)\n",
- err->str, err->help);
+ err->str, err->help ?: "<no help>");
free(err->str);
err->str = str;
free(err->help);
@@ -2502,6 +2510,47 @@ void parse_events_terms__delete(struct list_head *terms)
free(terms);
}
+int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
+{
+ struct parse_events_term *term;
+ bool first = true;
+
+ if (!term_list)
+ return 0;
+
+ list_for_each_entry(term, term_list, list) {
+ int ret;
+
+ if (!first) {
+ ret = strbuf_addch(sb, ',');
+ if (ret < 0)
+ return ret;
+ }
+ first = false;
+
+ if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
+ if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER && term->val.num == 1)
+ ret = strbuf_addf(sb, "%s", term->config);
+ else
+ ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
+ else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
+ if (term->config) {
+ ret = strbuf_addf(sb, "%s=", term->config);
+ if (ret < 0)
+ return ret;
+ } else if (term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) {
+ ret = strbuf_addf(sb, "%s=", config_term_names[term->type_term]);
+ if (ret < 0)
+ return ret;
+ }
+ ret = strbuf_addf(sb, "%s", term->val.str);
+ }
+ if (ret < 0)
+ return ret;
+ }
+ return 0;
+}
+
void parse_events_evlist_error(struct parse_events_state *parse_state,
int idx, const char *str)
{
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 6d75d853ce00..20bdc35d6112 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -18,6 +18,7 @@ struct parse_events_error;
struct option;
struct perf_pmu;
+struct strbuf;
const char *event_type(int type);
@@ -152,6 +153,7 @@ int parse_events_term__clone(struct parse_events_term **new,
void parse_events_term__delete(struct parse_events_term *term);
void parse_events_terms__delete(struct list_head *terms);
void parse_events_terms__purge(struct list_head *terms);
+int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb);
int parse_events__modifier_event(struct list_head *list, char *str, bool add);
int parse_events__modifier_group(struct list_head *list, char *event_mod);
int parse_events_name(struct list_head *list, const char *name);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index b3f8f3f1e900..8dbb7008877e 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -507,12 +507,11 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
const char *desc, const char *val, FILE *val_fd,
const struct pmu_event *pe)
{
- struct parse_events_term *term;
struct perf_pmu_alias *alias;
int ret;
- char newval[256];
const char *long_desc = NULL, *topic = NULL, *unit = NULL, *pmu_name = NULL;
bool deprecated = false, perpkg = false;
+ struct strbuf sb;
if (perf_pmu__find_alias(pmu, name, /*load=*/ false)) {
/* Alias was already created/loaded. */
@@ -582,20 +581,10 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
*
* Rebuild string to make alias->str member comparable.
*/
- ret = 0;
- list_for_each_entry(term, &alias->terms, list) {
- if (ret)
- ret += scnprintf(newval + ret, sizeof(newval) - ret,
- ",");
- if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
- ret += scnprintf(newval + ret, sizeof(newval) - ret,
- "%s=%#x", term->config, term->val.num);
- else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
- ret += scnprintf(newval + ret, sizeof(newval) - ret,
- "%s=%s", term->config, term->val.str);
- }
zfree(&alias->str);
- alias->str = strdup(newval);
+ strbuf_init(&sb, /*hint=*/ 0);
+ parse_events_term__to_strbuf(&alias->terms, &sb);
+ alias->str = strbuf_detach(&sb, /*sz=*/ NULL);
if (!pe)
pmu->sysfs_aliases++;
else
--
2.42.0.rc2.253.gd59a3bf2b4-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 3/3] perf pmu: Remove str from perf_pmu_alias
2023-08-30 7:07 [PATCH v1 1/3] perf parse-events: Minor help message improvements Ian Rogers
2023-08-30 7:07 ` [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper Ian Rogers
@ 2023-08-30 7:07 ` Ian Rogers
2023-08-31 18:32 ` [PATCH v1 1/3] perf parse-events: Minor help message improvements Liang, Kan
2 siblings, 0 replies; 10+ messages in thread
From: Ian Rogers @ 2023-08-30 7:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Kan Liang, James Clark,
linux-perf-users, linux-kernel
Currently the value is only used in perf list. Compute the value just
when needed to avoid unnecessary overhead. Recycle the strbuf to avoid
memory allocation overhead.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/pmu.c | 33 ++++++++++-----------------------
1 file changed, 10 insertions(+), 23 deletions(-)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 8dbb7008877e..152cda84f273 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -51,11 +51,6 @@ struct perf_pmu_alias {
* json events.
*/
char *topic;
- /**
- * @str: Comma separated parameter list like
- * "event=0xcd,umask=0x1,ldlat=0x3".
- */
- char *str;
/** @terms: Owned list of the original parsed parameters. */
struct list_head terms;
/** @list: List element of struct perf_pmu aliases. */
@@ -408,7 +403,6 @@ static void perf_pmu_free_alias(struct perf_pmu_alias *newalias)
zfree(&newalias->desc);
zfree(&newalias->long_desc);
zfree(&newalias->topic);
- zfree(&newalias->str);
zfree(&newalias->pmu_name);
parse_events_terms__purge(&newalias->terms);
free(newalias);
@@ -489,7 +483,7 @@ static int update_alias(const struct pmu_event *pe,
assign_str(pe->name, "long_desc", &data->alias->long_desc, pe->long_desc);
assign_str(pe->name, "topic", &data->alias->topic, pe->topic);
data->alias->per_pkg = pe->perpkg;
- if (assign_str(pe->name, "value", &data->alias->str, pe->event)) {
+ if (pe->event) {
parse_events_terms__purge(&data->alias->terms);
ret = parse_events_terms(&data->alias->terms, pe->event, /*input=*/NULL);
}
@@ -511,7 +505,6 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
int ret;
const char *long_desc = NULL, *topic = NULL, *unit = NULL, *pmu_name = NULL;
bool deprecated = false, perpkg = false;
- struct strbuf sb;
if (perf_pmu__find_alias(pmu, name, /*load=*/ false)) {
/* Alias was already created/loaded. */
@@ -531,7 +524,6 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
if (!alias)
return -ENOMEM;
- alias->str = NULL;
INIT_LIST_HEAD(&alias->terms);
alias->scale = 1.0;
alias->unit[0] = '\0';
@@ -574,17 +566,6 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
}
}
- /* Scan event and remove leading zeroes, spaces, newlines, some
- * platforms have terms specified as
- * event=0x0091 (read from files ../<PMU>/events/<FILE>
- * and terms specified as event=0x91 (read from JSON files).
- *
- * Rebuild string to make alias->str member comparable.
- */
- zfree(&alias->str);
- strbuf_init(&sb, /*hint=*/ 0);
- parse_events_term__to_strbuf(&alias->terms, &sb);
- alias->str = strbuf_detach(&sb, /*sz=*/ NULL);
if (!pe)
pmu->sysfs_aliases++;
else
@@ -1682,7 +1663,9 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
.pmu = pmu,
};
int ret = 0;
+ struct strbuf sb;
+ strbuf_init(&sb, /*hint=*/ 0);
pmu_add_cpu_aliases(pmu);
list_for_each_entry(event, &pmu->aliases, list) {
size_t buf_used;
@@ -1710,14 +1693,16 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
info.desc = event->desc;
info.long_desc = event->long_desc;
info.encoding_desc = buf + buf_used;
+ parse_events_term__to_strbuf(&event->terms, &sb);
buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
- "%s/%s/", info.pmu_name, event->str) + 1;
+ "%s/%s/", info.pmu_name, sb.buf) + 1;
info.topic = event->topic;
- info.str = event->str;
+ info.str = sb.buf;
info.deprecated = event->deprecated;
ret = cb(state, &info);
if (ret)
- return ret;
+ goto out;
+ strbuf_setlen(&sb, /*len=*/ 0);
}
if (pmu->selectable) {
info.name = buf;
@@ -1732,6 +1717,8 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
info.deprecated = false;
ret = cb(state, &info);
}
+out:
+ strbuf_release(&sb);
return ret;
}
--
2.42.0.rc2.253.gd59a3bf2b4-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper
2023-08-30 7:07 ` [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper Ian Rogers
@ 2023-08-30 14:11 ` Liang, Kan
2023-08-30 14:40 ` Ian Rogers
0 siblings, 1 reply; 10+ messages in thread
From: Liang, Kan @ 2023-08-30 14:11 UTC (permalink / raw)
To: Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, James Clark, linux-perf-users, linux-kernel
On 2023-08-30 3:07 a.m., Ian Rogers wrote:
> A term list is turned into a string for debug output and for the str
> value in the alias. Add a helper to do this based on existing code,
> but then fix for situations like events being identified. Use strbuf
> to manage the dynamic memory allocation and remove the 256 byte
> limit. Use in various places the string of the term list is required.
>
> Before:
> ```
> $ sudo perf stat -vv -e inst_retired.any true
> Using CPUID GenuineIntel-6-8D-1
> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> Attempting to add event pmu 'cpu' with 'inst_retired.any,' that may result in non-fatal errors
> After aliases, add event pmu 'cpu' with 'event,period,' that may result in non-fatal errors
> inst_retired.any -> cpu/inst_retired.any/
> ...
> ```
>
> After:
> ```
> $ sudo perf stat -vv -e inst_retired.any true
> Using CPUID GenuineIntel-6-8D-1
> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> Attempt to add: cpu/inst_retired.any/
> ..after resolving event: cpu/event=0xc0,period=0x1e8483/
> inst_retired.any -> cpu/event=0xc0,period=0x1e8483/
> ...
> ```
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/parse-events.c | 101 ++++++++++++++++++++++++---------
> tools/perf/util/parse-events.h | 2 +
> tools/perf/util/pmu.c | 19 ++-----
> 3 files changed, 81 insertions(+), 41 deletions(-)
Which branch should I use to apply the patch?
I tried the perf-tools-next branch, but there is some conflict.
I'd like to do some tests on a hybrid machine.
Thanks,
Kan
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 4c812fbe0cf9..0b941b58bdc0 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -13,7 +13,7 @@
> #include <subcmd/parse-options.h>
> #include "parse-events.h"
> #include "string2.h"
> -#include "strlist.h"
> +#include "strbuf.h"
> #include "debug.h"
> #include <api/fs/tracing_path.h>
> #include <perf/cpumap.h>
> @@ -1303,19 +1303,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>
> pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
>
> - if (verbose > 1 && !(pmu && pmu->selectable)) {
> - fprintf(stderr, "Attempting to add event pmu '%s' with '",
> - name);
> - if (head_config) {
> - struct parse_events_term *term;
> -
> - list_for_each_entry(term, head_config, list) {
> - fprintf(stderr, "%s,", term->config);
> - }
> - }
> - fprintf(stderr, "' that may result in non-fatal errors\n");
> - }
> -
> if (!pmu) {
> char *err_str;
>
> @@ -1325,6 +1312,21 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> parse_events_error__handle(err, loc->first_column, err_str, NULL);
> return -EINVAL;
> }
> +
> + if (verbose > 1) {
> + struct strbuf sb;
> +
> + strbuf_init(&sb, /*hint=*/ 0);
> + if (pmu->selectable && !head_config) {
> + strbuf_addf(&sb, "%s//", name);
> + } else {
> + strbuf_addf(&sb, "%s/", name);
> + parse_events_term__to_strbuf(head_config, &sb);
> + strbuf_addch(&sb, '/');
> + }
> + fprintf(stderr, "Attempt to add: %s\n", sb.buf);
> + strbuf_release(&sb);
> + }
> if (head_config)
> fix_raw(head_config, pmu);
>
> @@ -1349,16 +1351,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> return -EINVAL;
>
> if (verbose > 1) {
> - fprintf(stderr, "After aliases, add event pmu '%s' with '",
> - name);
> - if (head_config) {
> - struct parse_events_term *term;
> + struct strbuf sb;
>
> - list_for_each_entry(term, head_config, list) {
> - fprintf(stderr, "%s,", term->config);
> - }
> - }
> - fprintf(stderr, "' that may result in non-fatal errors\n");
> + strbuf_init(&sb, /*hint=*/ 0);
> + parse_events_term__to_strbuf(head_config, &sb);
> + fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
> + strbuf_release(&sb);
> }
>
> /*
> @@ -1460,7 +1458,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> parse_events_copy_term_list(head, &orig_head);
> if (!parse_events_add_pmu(parse_state, list, pmu->name,
> orig_head, auto_merge_stats, loc)) {
> - pr_debug("%s -> %s/%s/\n", str, pmu->name, str);
> + struct strbuf sb;
> +
> + strbuf_init(&sb, /*hint=*/ 0);
> + parse_events_term__to_strbuf(orig_head, &sb);
> + pr_debug("%s -> %s/%s/\n", str, pmu->name, sb.buf);
> + strbuf_release(&sb);
> ok++;
> }
> parse_events_terms__delete(orig_head);
> @@ -1469,7 +1472,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> if (parse_state->fake_pmu) {
> if (!parse_events_add_pmu(parse_state, list, str, head,
> /*auto_merge_stats=*/true, loc)) {
> - pr_debug("%s -> %s/%s/\n", str, "fake_pmu", str);
> + struct strbuf sb;
> +
> + strbuf_init(&sb, /*hint=*/ 0);
> + parse_events_term__to_strbuf(head, &sb);
> + pr_debug("%s -> %s/%s/\n", str, "fake_pmu", sb.buf);
> + strbuf_release(&sb);
> ok++;
> }
> }
> @@ -2085,7 +2093,7 @@ void parse_events_error__handle(struct parse_events_error *err, int idx,
> break;
> default:
> pr_debug("Multiple errors dropping message: %s (%s)\n",
> - err->str, err->help);
> + err->str, err->help ?: "<no help>");
> free(err->str);
> err->str = str;
> free(err->help);
> @@ -2502,6 +2510,47 @@ void parse_events_terms__delete(struct list_head *terms)
> free(terms);
> }
>
> +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
> +{
> + struct parse_events_term *term;
> + bool first = true;
> +
> + if (!term_list)
> + return 0;
> +
> + list_for_each_entry(term, term_list, list) {
> + int ret;
> +
> + if (!first) {
> + ret = strbuf_addch(sb, ',');
> + if (ret < 0)
> + return ret;
> + }
> + first = false;
> +
> + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> + if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER && term->val.num == 1)
> + ret = strbuf_addf(sb, "%s", term->config);
> + else
> + ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> + else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
> + if (term->config) {
> + ret = strbuf_addf(sb, "%s=", term->config);
> + if (ret < 0)
> + return ret;
> + } else if (term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) {
> + ret = strbuf_addf(sb, "%s=", config_term_names[term->type_term]);
> + if (ret < 0)
> + return ret;
> + }
> + ret = strbuf_addf(sb, "%s", term->val.str);
> + }
> + if (ret < 0)
> + return ret;
> + }
> + return 0;
> +}
> +
> void parse_events_evlist_error(struct parse_events_state *parse_state,
> int idx, const char *str)
> {
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 6d75d853ce00..20bdc35d6112 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -18,6 +18,7 @@ struct parse_events_error;
>
> struct option;
> struct perf_pmu;
> +struct strbuf;
>
> const char *event_type(int type);
>
> @@ -152,6 +153,7 @@ int parse_events_term__clone(struct parse_events_term **new,
> void parse_events_term__delete(struct parse_events_term *term);
> void parse_events_terms__delete(struct list_head *terms);
> void parse_events_terms__purge(struct list_head *terms);
> +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb);
> int parse_events__modifier_event(struct list_head *list, char *str, bool add);
> int parse_events__modifier_group(struct list_head *list, char *event_mod);
> int parse_events_name(struct list_head *list, const char *name);
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index b3f8f3f1e900..8dbb7008877e 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -507,12 +507,11 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
> const char *desc, const char *val, FILE *val_fd,
> const struct pmu_event *pe)
> {
> - struct parse_events_term *term;
> struct perf_pmu_alias *alias;
> int ret;
> - char newval[256];
> const char *long_desc = NULL, *topic = NULL, *unit = NULL, *pmu_name = NULL;
> bool deprecated = false, perpkg = false;
> + struct strbuf sb;
>
> if (perf_pmu__find_alias(pmu, name, /*load=*/ false)) {
> /* Alias was already created/loaded. */
> @@ -582,20 +581,10 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
> *
> * Rebuild string to make alias->str member comparable.
> */
> - ret = 0;
> - list_for_each_entry(term, &alias->terms, list) {
> - if (ret)
> - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> - ",");
> - if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> - "%s=%#x", term->config, term->val.num);
> - else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> - "%s=%s", term->config, term->val.str);
> - }
> zfree(&alias->str);
> - alias->str = strdup(newval);
> + strbuf_init(&sb, /*hint=*/ 0);
> + parse_events_term__to_strbuf(&alias->terms, &sb);
> + alias->str = strbuf_detach(&sb, /*sz=*/ NULL);
> if (!pe)
> pmu->sysfs_aliases++;
> else
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper
2023-08-30 14:11 ` Liang, Kan
@ 2023-08-30 14:40 ` Ian Rogers
2023-08-30 15:04 ` Ian Rogers
0 siblings, 1 reply; 10+ messages in thread
From: Ian Rogers @ 2023-08-30 14:40 UTC (permalink / raw)
To: Liang, Kan
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, James Clark, linux-perf-users, linux-kernel
On Wed, Aug 30, 2023 at 7:15 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2023-08-30 3:07 a.m., Ian Rogers wrote:
> > A term list is turned into a string for debug output and for the str
> > value in the alias. Add a helper to do this based on existing code,
> > but then fix for situations like events being identified. Use strbuf
> > to manage the dynamic memory allocation and remove the 256 byte
> > limit. Use in various places the string of the term list is required.
> >
> > Before:
> > ```
> > $ sudo perf stat -vv -e inst_retired.any true
> > Using CPUID GenuineIntel-6-8D-1
> > intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> > Attempting to add event pmu 'cpu' with 'inst_retired.any,' that may result in non-fatal errors
> > After aliases, add event pmu 'cpu' with 'event,period,' that may result in non-fatal errors
> > inst_retired.any -> cpu/inst_retired.any/
> > ...
> > ```
> >
> > After:
> > ```
> > $ sudo perf stat -vv -e inst_retired.any true
> > Using CPUID GenuineIntel-6-8D-1
> > intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> > Attempt to add: cpu/inst_retired.any/
> > ..after resolving event: cpu/event=0xc0,period=0x1e8483/
> > inst_retired.any -> cpu/event=0xc0,period=0x1e8483/
> > ...
> > ```
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/util/parse-events.c | 101 ++++++++++++++++++++++++---------
> > tools/perf/util/parse-events.h | 2 +
> > tools/perf/util/pmu.c | 19 ++-----
> > 3 files changed, 81 insertions(+), 41 deletions(-)
>
> Which branch should I use to apply the patch?
> I tried the perf-tools-next branch, but there is some conflict.
> I'd like to do some tests on a hybrid machine.
I was working on tmp.perf-tools-next but it is the same as
perf-tools-next currently [1]. I had this 2 line patch in place:
https://lore.kernel.org/lkml/20230830000545.1638964-1-irogers@google.com/
so that may be the conflict.
Thanks,
Ian
[1] https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=tmp.perf-tools-next
> Thanks,
> Kan
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 4c812fbe0cf9..0b941b58bdc0 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -13,7 +13,7 @@
> > #include <subcmd/parse-options.h>
> > #include "parse-events.h"
> > #include "string2.h"
> > -#include "strlist.h"
> > +#include "strbuf.h"
> > #include "debug.h"
> > #include <api/fs/tracing_path.h>
> > #include <perf/cpumap.h>
> > @@ -1303,19 +1303,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> >
> > pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
> >
> > - if (verbose > 1 && !(pmu && pmu->selectable)) {
> > - fprintf(stderr, "Attempting to add event pmu '%s' with '",
> > - name);
> > - if (head_config) {
> > - struct parse_events_term *term;
> > -
> > - list_for_each_entry(term, head_config, list) {
> > - fprintf(stderr, "%s,", term->config);
> > - }
> > - }
> > - fprintf(stderr, "' that may result in non-fatal errors\n");
> > - }
> > -
> > if (!pmu) {
> > char *err_str;
> >
> > @@ -1325,6 +1312,21 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> > parse_events_error__handle(err, loc->first_column, err_str, NULL);
> > return -EINVAL;
> > }
> > +
> > + if (verbose > 1) {
> > + struct strbuf sb;
> > +
> > + strbuf_init(&sb, /*hint=*/ 0);
> > + if (pmu->selectable && !head_config) {
> > + strbuf_addf(&sb, "%s//", name);
> > + } else {
> > + strbuf_addf(&sb, "%s/", name);
> > + parse_events_term__to_strbuf(head_config, &sb);
> > + strbuf_addch(&sb, '/');
> > + }
> > + fprintf(stderr, "Attempt to add: %s\n", sb.buf);
> > + strbuf_release(&sb);
> > + }
> > if (head_config)
> > fix_raw(head_config, pmu);
> >
> > @@ -1349,16 +1351,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> > return -EINVAL;
> >
> > if (verbose > 1) {
> > - fprintf(stderr, "After aliases, add event pmu '%s' with '",
> > - name);
> > - if (head_config) {
> > - struct parse_events_term *term;
> > + struct strbuf sb;
> >
> > - list_for_each_entry(term, head_config, list) {
> > - fprintf(stderr, "%s,", term->config);
> > - }
> > - }
> > - fprintf(stderr, "' that may result in non-fatal errors\n");
> > + strbuf_init(&sb, /*hint=*/ 0);
> > + parse_events_term__to_strbuf(head_config, &sb);
> > + fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
> > + strbuf_release(&sb);
> > }
> >
> > /*
> > @@ -1460,7 +1458,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> > parse_events_copy_term_list(head, &orig_head);
> > if (!parse_events_add_pmu(parse_state, list, pmu->name,
> > orig_head, auto_merge_stats, loc)) {
> > - pr_debug("%s -> %s/%s/\n", str, pmu->name, str);
> > + struct strbuf sb;
> > +
> > + strbuf_init(&sb, /*hint=*/ 0);
> > + parse_events_term__to_strbuf(orig_head, &sb);
> > + pr_debug("%s -> %s/%s/\n", str, pmu->name, sb.buf);
> > + strbuf_release(&sb);
> > ok++;
> > }
> > parse_events_terms__delete(orig_head);
> > @@ -1469,7 +1472,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> > if (parse_state->fake_pmu) {
> > if (!parse_events_add_pmu(parse_state, list, str, head,
> > /*auto_merge_stats=*/true, loc)) {
> > - pr_debug("%s -> %s/%s/\n", str, "fake_pmu", str);
> > + struct strbuf sb;
> > +
> > + strbuf_init(&sb, /*hint=*/ 0);
> > + parse_events_term__to_strbuf(head, &sb);
> > + pr_debug("%s -> %s/%s/\n", str, "fake_pmu", sb.buf);
> > + strbuf_release(&sb);
> > ok++;
> > }
> > }
> > @@ -2085,7 +2093,7 @@ void parse_events_error__handle(struct parse_events_error *err, int idx,
> > break;
> > default:
> > pr_debug("Multiple errors dropping message: %s (%s)\n",
> > - err->str, err->help);
> > + err->str, err->help ?: "<no help>");
> > free(err->str);
> > err->str = str;
> > free(err->help);
> > @@ -2502,6 +2510,47 @@ void parse_events_terms__delete(struct list_head *terms)
> > free(terms);
> > }
> >
> > +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
> > +{
> > + struct parse_events_term *term;
> > + bool first = true;
> > +
> > + if (!term_list)
> > + return 0;
> > +
> > + list_for_each_entry(term, term_list, list) {
> > + int ret;
> > +
> > + if (!first) {
> > + ret = strbuf_addch(sb, ',');
> > + if (ret < 0)
> > + return ret;
> > + }
> > + first = false;
> > +
> > + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> > + if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER && term->val.num == 1)
> > + ret = strbuf_addf(sb, "%s", term->config);
> > + else
> > + ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> > + else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
> > + if (term->config) {
> > + ret = strbuf_addf(sb, "%s=", term->config);
> > + if (ret < 0)
> > + return ret;
> > + } else if (term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) {
> > + ret = strbuf_addf(sb, "%s=", config_term_names[term->type_term]);
> > + if (ret < 0)
> > + return ret;
> > + }
> > + ret = strbuf_addf(sb, "%s", term->val.str);
> > + }
> > + if (ret < 0)
> > + return ret;
> > + }
> > + return 0;
> > +}
> > +
> > void parse_events_evlist_error(struct parse_events_state *parse_state,
> > int idx, const char *str)
> > {
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 6d75d853ce00..20bdc35d6112 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -18,6 +18,7 @@ struct parse_events_error;
> >
> > struct option;
> > struct perf_pmu;
> > +struct strbuf;
> >
> > const char *event_type(int type);
> >
> > @@ -152,6 +153,7 @@ int parse_events_term__clone(struct parse_events_term **new,
> > void parse_events_term__delete(struct parse_events_term *term);
> > void parse_events_terms__delete(struct list_head *terms);
> > void parse_events_terms__purge(struct list_head *terms);
> > +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb);
> > int parse_events__modifier_event(struct list_head *list, char *str, bool add);
> > int parse_events__modifier_group(struct list_head *list, char *event_mod);
> > int parse_events_name(struct list_head *list, const char *name);
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > index b3f8f3f1e900..8dbb7008877e 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -507,12 +507,11 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
> > const char *desc, const char *val, FILE *val_fd,
> > const struct pmu_event *pe)
> > {
> > - struct parse_events_term *term;
> > struct perf_pmu_alias *alias;
> > int ret;
> > - char newval[256];
> > const char *long_desc = NULL, *topic = NULL, *unit = NULL, *pmu_name = NULL;
> > bool deprecated = false, perpkg = false;
> > + struct strbuf sb;
> >
> > if (perf_pmu__find_alias(pmu, name, /*load=*/ false)) {
> > /* Alias was already created/loaded. */
> > @@ -582,20 +581,10 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
> > *
> > * Rebuild string to make alias->str member comparable.
> > */
> > - ret = 0;
> > - list_for_each_entry(term, &alias->terms, list) {
> > - if (ret)
> > - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > - ",");
> > - if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> > - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > - "%s=%#x", term->config, term->val.num);
> > - else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> > - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > - "%s=%s", term->config, term->val.str);
> > - }
> > zfree(&alias->str);
> > - alias->str = strdup(newval);
> > + strbuf_init(&sb, /*hint=*/ 0);
> > + parse_events_term__to_strbuf(&alias->terms, &sb);
> > + alias->str = strbuf_detach(&sb, /*sz=*/ NULL);
> > if (!pe)
> > pmu->sysfs_aliases++;
> > else
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper
2023-08-30 14:40 ` Ian Rogers
@ 2023-08-30 15:04 ` Ian Rogers
2023-08-30 18:29 ` Liang, Kan
0 siblings, 1 reply; 10+ messages in thread
From: Ian Rogers @ 2023-08-30 15:04 UTC (permalink / raw)
To: Liang, Kan
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, James Clark, linux-perf-users, linux-kernel
On Wed, Aug 30, 2023 at 7:40 AM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Aug 30, 2023 at 7:15 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2023-08-30 3:07 a.m., Ian Rogers wrote:
> > > A term list is turned into a string for debug output and for the str
> > > value in the alias. Add a helper to do this based on existing code,
> > > but then fix for situations like events being identified. Use strbuf
> > > to manage the dynamic memory allocation and remove the 256 byte
> > > limit. Use in various places the string of the term list is required.
> > >
> > > Before:
> > > ```
> > > $ sudo perf stat -vv -e inst_retired.any true
> > > Using CPUID GenuineIntel-6-8D-1
> > > intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> > > Attempting to add event pmu 'cpu' with 'inst_retired.any,' that may result in non-fatal errors
> > > After aliases, add event pmu 'cpu' with 'event,period,' that may result in non-fatal errors
> > > inst_retired.any -> cpu/inst_retired.any/
> > > ...
> > > ```
> > >
> > > After:
> > > ```
> > > $ sudo perf stat -vv -e inst_retired.any true
> > > Using CPUID GenuineIntel-6-8D-1
> > > intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> > > Attempt to add: cpu/inst_retired.any/
> > > ..after resolving event: cpu/event=0xc0,period=0x1e8483/
> > > inst_retired.any -> cpu/event=0xc0,period=0x1e8483/
> > > ...
> > > ```
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > > tools/perf/util/parse-events.c | 101 ++++++++++++++++++++++++---------
> > > tools/perf/util/parse-events.h | 2 +
> > > tools/perf/util/pmu.c | 19 ++-----
> > > 3 files changed, 81 insertions(+), 41 deletions(-)
> >
> > Which branch should I use to apply the patch?
> > I tried the perf-tools-next branch, but there is some conflict.
> > I'd like to do some tests on a hybrid machine.
>
> I was working on tmp.perf-tools-next but it is the same as
> perf-tools-next currently [1]. I had this 2 line patch in place:
> https://lore.kernel.org/lkml/20230830000545.1638964-1-irogers@google.com/
> so that may be the conflict.
>
> Thanks,
> Ian
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=tmp.perf-tools-next
Hmm.. for some reason I'm not seeing the message on LKML and so b4
fails. Anyway, applying the patches and running on an Alderlake I see:
```
$ perf stat -vv -e inst_retired.any true
Using CPUID GenuineIntel-6-97-2
Attempt to add: cpu_atom/inst_retired.any/
..after resolving event: cpu_atom/event=0xc0,period=0x1e8483/
inst_retired.any -> cpu_atom/event=0xc0,period=0x1e8483/
Attempt to add: cpu_core/inst_retired.any/
..after resolving event: cpu_core/event=0xc0,period=0x1e8483/
inst_retired.any -> cpu_core/event=0xc0,period=0x1e8483/
...
```
The previous output was like:
```
$ perf stat -vv -e inst_retired.any true
Using CPUID GenuineIntel-6-97-2
Attempting to add event pmu 'cpu_core' with 'inst_retired.any,' that
may result in non-fatal errors
After aliases, add event pmu 'cpu_core' with 'event,period,' that may
result in non-fatal errors
inst_retired.any -> cpu_core/event=0xc0,period=0x1e8483/
Attempting to add event pmu 'cpu_atom' with 'inst_retired.any,' that
may result in non-fatal errors
After aliases, add event pmu 'cpu_atom' with 'event,period,' that may
result in non-fatal errors
inst_retired.any -> cpu_atom/event=0xc0,period=0x1e8483/
...
```
Perhaps a more interesting example is:
```
$ perf stat -vv -e UOPS_RETIRED.MS true
Using CPUID GenuineIntel-6-97-2
Attempt to add: cpu_atom/UOPS_RETIRED.MS/
..after resolving event: cpu_atom/event=0xc2,period=0x1e8483,umask/
UOPS_RETIRED.MS -> cpu_atom/event=0xc2,period=0x1e8483,umask/
Attempt to add: cpu_core/UOPS_RETIRED.MS/
..after resolving event:
cpu_core/event=0xc2,period=0x1e8483,umask=0x4,frontend=0x8/
UOPS_RETIRED.MS -> cpu_core/event=0xc2,period=0x1e8483,umask=0x4,frontend=0x8/
...
```
The umask not being printed for the cpu_atom is an issue. The problem
is how we encode terms of an event name, it is indistinguishable when
the of the user field is 1. I'll probably add something to fix this
later, but it only impacts debug output and perf list, so I'm not
overly worried. We could go in the other direction and instead of
printing say cpu_atom/UOPS_RETIRED.MS/ print
cpu_atom/UOPS_RETIRED.MS=1/, but I thought that was uglier and more
confusing.
Thanks,
Ian
> > Thanks,
> > Kan
> > >
> > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > > index 4c812fbe0cf9..0b941b58bdc0 100644
> > > --- a/tools/perf/util/parse-events.c
> > > +++ b/tools/perf/util/parse-events.c
> > > @@ -13,7 +13,7 @@
> > > #include <subcmd/parse-options.h>
> > > #include "parse-events.h"
> > > #include "string2.h"
> > > -#include "strlist.h"
> > > +#include "strbuf.h"
> > > #include "debug.h"
> > > #include <api/fs/tracing_path.h>
> > > #include <perf/cpumap.h>
> > > @@ -1303,19 +1303,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> > >
> > > pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
> > >
> > > - if (verbose > 1 && !(pmu && pmu->selectable)) {
> > > - fprintf(stderr, "Attempting to add event pmu '%s' with '",
> > > - name);
> > > - if (head_config) {
> > > - struct parse_events_term *term;
> > > -
> > > - list_for_each_entry(term, head_config, list) {
> > > - fprintf(stderr, "%s,", term->config);
> > > - }
> > > - }
> > > - fprintf(stderr, "' that may result in non-fatal errors\n");
> > > - }
> > > -
> > > if (!pmu) {
> > > char *err_str;
> > >
> > > @@ -1325,6 +1312,21 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> > > parse_events_error__handle(err, loc->first_column, err_str, NULL);
> > > return -EINVAL;
> > > }
> > > +
> > > + if (verbose > 1) {
> > > + struct strbuf sb;
> > > +
> > > + strbuf_init(&sb, /*hint=*/ 0);
> > > + if (pmu->selectable && !head_config) {
> > > + strbuf_addf(&sb, "%s//", name);
> > > + } else {
> > > + strbuf_addf(&sb, "%s/", name);
> > > + parse_events_term__to_strbuf(head_config, &sb);
> > > + strbuf_addch(&sb, '/');
> > > + }
> > > + fprintf(stderr, "Attempt to add: %s\n", sb.buf);
> > > + strbuf_release(&sb);
> > > + }
> > > if (head_config)
> > > fix_raw(head_config, pmu);
> > >
> > > @@ -1349,16 +1351,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> > > return -EINVAL;
> > >
> > > if (verbose > 1) {
> > > - fprintf(stderr, "After aliases, add event pmu '%s' with '",
> > > - name);
> > > - if (head_config) {
> > > - struct parse_events_term *term;
> > > + struct strbuf sb;
> > >
> > > - list_for_each_entry(term, head_config, list) {
> > > - fprintf(stderr, "%s,", term->config);
> > > - }
> > > - }
> > > - fprintf(stderr, "' that may result in non-fatal errors\n");
> > > + strbuf_init(&sb, /*hint=*/ 0);
> > > + parse_events_term__to_strbuf(head_config, &sb);
> > > + fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
> > > + strbuf_release(&sb);
> > > }
> > >
> > > /*
> > > @@ -1460,7 +1458,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> > > parse_events_copy_term_list(head, &orig_head);
> > > if (!parse_events_add_pmu(parse_state, list, pmu->name,
> > > orig_head, auto_merge_stats, loc)) {
> > > - pr_debug("%s -> %s/%s/\n", str, pmu->name, str);
> > > + struct strbuf sb;
> > > +
> > > + strbuf_init(&sb, /*hint=*/ 0);
> > > + parse_events_term__to_strbuf(orig_head, &sb);
> > > + pr_debug("%s -> %s/%s/\n", str, pmu->name, sb.buf);
> > > + strbuf_release(&sb);
> > > ok++;
> > > }
> > > parse_events_terms__delete(orig_head);
> > > @@ -1469,7 +1472,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> > > if (parse_state->fake_pmu) {
> > > if (!parse_events_add_pmu(parse_state, list, str, head,
> > > /*auto_merge_stats=*/true, loc)) {
> > > - pr_debug("%s -> %s/%s/\n", str, "fake_pmu", str);
> > > + struct strbuf sb;
> > > +
> > > + strbuf_init(&sb, /*hint=*/ 0);
> > > + parse_events_term__to_strbuf(head, &sb);
> > > + pr_debug("%s -> %s/%s/\n", str, "fake_pmu", sb.buf);
> > > + strbuf_release(&sb);
> > > ok++;
> > > }
> > > }
> > > @@ -2085,7 +2093,7 @@ void parse_events_error__handle(struct parse_events_error *err, int idx,
> > > break;
> > > default:
> > > pr_debug("Multiple errors dropping message: %s (%s)\n",
> > > - err->str, err->help);
> > > + err->str, err->help ?: "<no help>");
> > > free(err->str);
> > > err->str = str;
> > > free(err->help);
> > > @@ -2502,6 +2510,47 @@ void parse_events_terms__delete(struct list_head *terms)
> > > free(terms);
> > > }
> > >
> > > +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
> > > +{
> > > + struct parse_events_term *term;
> > > + bool first = true;
> > > +
> > > + if (!term_list)
> > > + return 0;
> > > +
> > > + list_for_each_entry(term, term_list, list) {
> > > + int ret;
> > > +
> > > + if (!first) {
> > > + ret = strbuf_addch(sb, ',');
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > + first = false;
> > > +
> > > + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> > > + if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER && term->val.num == 1)
> > > + ret = strbuf_addf(sb, "%s", term->config);
> > > + else
> > > + ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> > > + else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
> > > + if (term->config) {
> > > + ret = strbuf_addf(sb, "%s=", term->config);
> > > + if (ret < 0)
> > > + return ret;
> > > + } else if (term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) {
> > > + ret = strbuf_addf(sb, "%s=", config_term_names[term->type_term]);
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > + ret = strbuf_addf(sb, "%s", term->val.str);
> > > + }
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > void parse_events_evlist_error(struct parse_events_state *parse_state,
> > > int idx, const char *str)
> > > {
> > > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > > index 6d75d853ce00..20bdc35d6112 100644
> > > --- a/tools/perf/util/parse-events.h
> > > +++ b/tools/perf/util/parse-events.h
> > > @@ -18,6 +18,7 @@ struct parse_events_error;
> > >
> > > struct option;
> > > struct perf_pmu;
> > > +struct strbuf;
> > >
> > > const char *event_type(int type);
> > >
> > > @@ -152,6 +153,7 @@ int parse_events_term__clone(struct parse_events_term **new,
> > > void parse_events_term__delete(struct parse_events_term *term);
> > > void parse_events_terms__delete(struct list_head *terms);
> > > void parse_events_terms__purge(struct list_head *terms);
> > > +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb);
> > > int parse_events__modifier_event(struct list_head *list, char *str, bool add);
> > > int parse_events__modifier_group(struct list_head *list, char *event_mod);
> > > int parse_events_name(struct list_head *list, const char *name);
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > index b3f8f3f1e900..8dbb7008877e 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -507,12 +507,11 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
> > > const char *desc, const char *val, FILE *val_fd,
> > > const struct pmu_event *pe)
> > > {
> > > - struct parse_events_term *term;
> > > struct perf_pmu_alias *alias;
> > > int ret;
> > > - char newval[256];
> > > const char *long_desc = NULL, *topic = NULL, *unit = NULL, *pmu_name = NULL;
> > > bool deprecated = false, perpkg = false;
> > > + struct strbuf sb;
> > >
> > > if (perf_pmu__find_alias(pmu, name, /*load=*/ false)) {
> > > /* Alias was already created/loaded. */
> > > @@ -582,20 +581,10 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
> > > *
> > > * Rebuild string to make alias->str member comparable.
> > > */
> > > - ret = 0;
> > > - list_for_each_entry(term, &alias->terms, list) {
> > > - if (ret)
> > > - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > > - ",");
> > > - if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> > > - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > > - "%s=%#x", term->config, term->val.num);
> > > - else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> > > - ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > > - "%s=%s", term->config, term->val.str);
> > > - }
> > > zfree(&alias->str);
> > > - alias->str = strdup(newval);
> > > + strbuf_init(&sb, /*hint=*/ 0);
> > > + parse_events_term__to_strbuf(&alias->terms, &sb);
> > > + alias->str = strbuf_detach(&sb, /*sz=*/ NULL);
> > > if (!pe)
> > > pmu->sysfs_aliases++;
> > > else
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper
2023-08-30 15:04 ` Ian Rogers
@ 2023-08-30 18:29 ` Liang, Kan
2023-08-30 19:34 ` Liang, Kan
0 siblings, 1 reply; 10+ messages in thread
From: Liang, Kan @ 2023-08-30 18:29 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, James Clark, linux-perf-users, linux-kernel
On 2023-08-30 11:04 a.m., Ian Rogers wrote:
> On Wed, Aug 30, 2023 at 7:40 AM Ian Rogers <irogers@google.com> wrote:
>>
>> On Wed, Aug 30, 2023 at 7:15 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>>
>>>
>>>
>>> On 2023-08-30 3:07 a.m., Ian Rogers wrote:
>>>> A term list is turned into a string for debug output and for the str
>>>> value in the alias. Add a helper to do this based on existing code,
>>>> but then fix for situations like events being identified. Use strbuf
>>>> to manage the dynamic memory allocation and remove the 256 byte
>>>> limit. Use in various places the string of the term list is required.
>>>>
>>>> Before:
>>>> ```
>>>> $ sudo perf stat -vv -e inst_retired.any true
>>>> Using CPUID GenuineIntel-6-8D-1
>>>> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
>>>> Attempting to add event pmu 'cpu' with 'inst_retired.any,' that may result in non-fatal errors
>>>> After aliases, add event pmu 'cpu' with 'event,period,' that may result in non-fatal errors
>>>> inst_retired.any -> cpu/inst_retired.any/
>>>> ...
>>>> ```
>>>>
>>>> After:
>>>> ```
>>>> $ sudo perf stat -vv -e inst_retired.any true
>>>> Using CPUID GenuineIntel-6-8D-1
>>>> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
>>>> Attempt to add: cpu/inst_retired.any/
>>>> ..after resolving event: cpu/event=0xc0,period=0x1e8483/
>>>> inst_retired.any -> cpu/event=0xc0,period=0x1e8483/
>>>> ...
>>>> ```
>>>>
>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>> ---
>>>> tools/perf/util/parse-events.c | 101 ++++++++++++++++++++++++---------
>>>> tools/perf/util/parse-events.h | 2 +
>>>> tools/perf/util/pmu.c | 19 ++-----
>>>> 3 files changed, 81 insertions(+), 41 deletions(-)
>>>
>>> Which branch should I use to apply the patch?
>>> I tried the perf-tools-next branch, but there is some conflict.
>>> I'd like to do some tests on a hybrid machine.
>>
>> I was working on tmp.perf-tools-next but it is the same as
>> perf-tools-next currently [1]. I had this 2 line patch in place:
>> https://lore.kernel.org/lkml/20230830000545.1638964-1-irogers@google.com/
Yes, on top of this patch, I don't see the conflict anymore.
>> so that may be the conflict.
>>
>> Thanks,
>> Ian
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=tmp.perf-tools-next
>
> Hmm.. for some reason I'm not seeing the message on LKML and so b4
> fails. Anyway, applying the patches and running on an Alderlake I see:
> ```
> $ perf stat -vv -e inst_retired.any true
> Using CPUID GenuineIntel-6-97-2
> Attempt to add: cpu_atom/inst_retired.any/
> ..after resolving event: cpu_atom/event=0xc0,period=0x1e8483/
> inst_retired.any -> cpu_atom/event=0xc0,period=0x1e8483/
> Attempt to add: cpu_core/inst_retired.any/
> ..after resolving event: cpu_core/event=0xc0,period=0x1e8483/
> inst_retired.any -> cpu_core/event=0xc0,period=0x1e8483/
> ...
> ```
> The previous output was like:
> ```
> $ perf stat -vv -e inst_retired.any true
> Using CPUID GenuineIntel-6-97-2
> Attempting to add event pmu 'cpu_core' with 'inst_retired.any,' that
> may result in non-fatal errors
> After aliases, add event pmu 'cpu_core' with 'event,period,' that may
> result in non-fatal errors
> inst_retired.any -> cpu_core/event=0xc0,period=0x1e8483/
> Attempting to add event pmu 'cpu_atom' with 'inst_retired.any,' that
> may result in non-fatal errors
> After aliases, add event pmu 'cpu_atom' with 'event,period,' that may
> result in non-fatal errors
> inst_retired.any -> cpu_atom/event=0xc0,period=0x1e8483/
> ...
> ```
> Perhaps a more interesting example is:
> ```
> $ perf stat -vv -e UOPS_RETIRED.MS true
> Using CPUID GenuineIntel-6-97-2
> Attempt to add: cpu_atom/UOPS_RETIRED.MS/
> ..after resolving event: cpu_atom/event=0xc2,period=0x1e8483,umask/
> UOPS_RETIRED.MS -> cpu_atom/event=0xc2,period=0x1e8483,umask/
> Attempt to add: cpu_core/UOPS_RETIRED.MS/
> ..after resolving event:
> cpu_core/event=0xc2,period=0x1e8483,umask=0x4,frontend=0x8/
> UOPS_RETIRED.MS -> cpu_core/event=0xc2,period=0x1e8483,umask=0x4,frontend=0x8/
> ...
> ```
> The umask not being printed for the cpu_atom is an issue.
Yes, I observed it as well.
> The problem
> is how we encode terms of an event name, it is indistinguishable when
> the of the user field is 1. I'll probably add something to fix this
> later, but it only impacts debug output and perf list, so I'm not
> overly worried.
It should be OK for the debug output.
> We could go in the other direction and instead of
> printing say cpu_atom/UOPS_RETIRED.MS/ print
> cpu_atom/UOPS_RETIRED.MS=1/, but I thought that was uglier and more
> confusing.
If there is no plan to fix it for now, it's better to keep it as is. The
UOPS_RETIRED.MS=1 is confusing.
Thanks,
Kan
>
> Thanks,
> Ian
>
>>> Thanks,
>>> Kan
>>>>
>>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>>> index 4c812fbe0cf9..0b941b58bdc0 100644
>>>> --- a/tools/perf/util/parse-events.c
>>>> +++ b/tools/perf/util/parse-events.c
>>>> @@ -13,7 +13,7 @@
>>>> #include <subcmd/parse-options.h>
>>>> #include "parse-events.h"
>>>> #include "string2.h"
>>>> -#include "strlist.h"
>>>> +#include "strbuf.h"
>>>> #include "debug.h"
>>>> #include <api/fs/tracing_path.h>
>>>> #include <perf/cpumap.h>
>>>> @@ -1303,19 +1303,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>>>>
>>>> pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
>>>>
>>>> - if (verbose > 1 && !(pmu && pmu->selectable)) {
>>>> - fprintf(stderr, "Attempting to add event pmu '%s' with '",
>>>> - name);
>>>> - if (head_config) {
>>>> - struct parse_events_term *term;
>>>> -
>>>> - list_for_each_entry(term, head_config, list) {
>>>> - fprintf(stderr, "%s,", term->config);
>>>> - }
>>>> - }
>>>> - fprintf(stderr, "' that may result in non-fatal errors\n");
>>>> - }
>>>> -
>>>> if (!pmu) {
>>>> char *err_str;
>>>>
>>>> @@ -1325,6 +1312,21 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>>>> parse_events_error__handle(err, loc->first_column, err_str, NULL);
>>>> return -EINVAL;
>>>> }
>>>> +
>>>> + if (verbose > 1) {
>>>> + struct strbuf sb;
>>>> +
>>>> + strbuf_init(&sb, /*hint=*/ 0);
>>>> + if (pmu->selectable && !head_config) {
>>>> + strbuf_addf(&sb, "%s//", name);
>>>> + } else {
>>>> + strbuf_addf(&sb, "%s/", name);
>>>> + parse_events_term__to_strbuf(head_config, &sb);
>>>> + strbuf_addch(&sb, '/');
>>>> + }
>>>> + fprintf(stderr, "Attempt to add: %s\n", sb.buf);
>>>> + strbuf_release(&sb);
>>>> + }
>>>> if (head_config)
>>>> fix_raw(head_config, pmu);
>>>>
>>>> @@ -1349,16 +1351,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>>>> return -EINVAL;
>>>>
>>>> if (verbose > 1) {
>>>> - fprintf(stderr, "After aliases, add event pmu '%s' with '",
>>>> - name);
>>>> - if (head_config) {
>>>> - struct parse_events_term *term;
>>>> + struct strbuf sb;
>>>>
>>>> - list_for_each_entry(term, head_config, list) {
>>>> - fprintf(stderr, "%s,", term->config);
>>>> - }
>>>> - }
>>>> - fprintf(stderr, "' that may result in non-fatal errors\n");
>>>> + strbuf_init(&sb, /*hint=*/ 0);
>>>> + parse_events_term__to_strbuf(head_config, &sb);
>>>> + fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
>>>> + strbuf_release(&sb);
>>>> }
>>>>
>>>> /*
>>>> @@ -1460,7 +1458,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>>>> parse_events_copy_term_list(head, &orig_head);
>>>> if (!parse_events_add_pmu(parse_state, list, pmu->name,
>>>> orig_head, auto_merge_stats, loc)) {
>>>> - pr_debug("%s -> %s/%s/\n", str, pmu->name, str);
>>>> + struct strbuf sb;
>>>> +
>>>> + strbuf_init(&sb, /*hint=*/ 0);
>>>> + parse_events_term__to_strbuf(orig_head, &sb);
>>>> + pr_debug("%s -> %s/%s/\n", str, pmu->name, sb.buf);
>>>> + strbuf_release(&sb);
>>>> ok++;
>>>> }
>>>> parse_events_terms__delete(orig_head);
>>>> @@ -1469,7 +1472,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>>>> if (parse_state->fake_pmu) {
>>>> if (!parse_events_add_pmu(parse_state, list, str, head,
>>>> /*auto_merge_stats=*/true, loc)) {
>>>> - pr_debug("%s -> %s/%s/\n", str, "fake_pmu", str);
>>>> + struct strbuf sb;
>>>> +
>>>> + strbuf_init(&sb, /*hint=*/ 0);
>>>> + parse_events_term__to_strbuf(head, &sb);
>>>> + pr_debug("%s -> %s/%s/\n", str, "fake_pmu", sb.buf);
>>>> + strbuf_release(&sb);
>>>> ok++;
>>>> }
>>>> }
>>>> @@ -2085,7 +2093,7 @@ void parse_events_error__handle(struct parse_events_error *err, int idx,
>>>> break;
>>>> default:
>>>> pr_debug("Multiple errors dropping message: %s (%s)\n",
>>>> - err->str, err->help);
>>>> + err->str, err->help ?: "<no help>");
>>>> free(err->str);
>>>> err->str = str;
>>>> free(err->help);
>>>> @@ -2502,6 +2510,47 @@ void parse_events_terms__delete(struct list_head *terms)
>>>> free(terms);
>>>> }
>>>>
>>>> +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
>>>> +{
>>>> + struct parse_events_term *term;
>>>> + bool first = true;
>>>> +
>>>> + if (!term_list)
>>>> + return 0;
>>>> +
>>>> + list_for_each_entry(term, term_list, list) {
>>>> + int ret;
>>>> +
>>>> + if (!first) {
>>>> + ret = strbuf_addch(sb, ',');
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + }
>>>> + first = false;
>>>> +
>>>> + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
>>>> + if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER && term->val.num == 1)
>>>> + ret = strbuf_addf(sb, "%s", term->config);
>>>> + else
>>>> + ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
>>>> + else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
>>>> + if (term->config) {
>>>> + ret = strbuf_addf(sb, "%s=", term->config);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + } else if (term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) {
>>>> + ret = strbuf_addf(sb, "%s=", config_term_names[term->type_term]);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + }
>>>> + ret = strbuf_addf(sb, "%s", term->val.str);
>>>> + }
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> void parse_events_evlist_error(struct parse_events_state *parse_state,
>>>> int idx, const char *str)
>>>> {
>>>> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
>>>> index 6d75d853ce00..20bdc35d6112 100644
>>>> --- a/tools/perf/util/parse-events.h
>>>> +++ b/tools/perf/util/parse-events.h
>>>> @@ -18,6 +18,7 @@ struct parse_events_error;
>>>>
>>>> struct option;
>>>> struct perf_pmu;
>>>> +struct strbuf;
>>>>
>>>> const char *event_type(int type);
>>>>
>>>> @@ -152,6 +153,7 @@ int parse_events_term__clone(struct parse_events_term **new,
>>>> void parse_events_term__delete(struct parse_events_term *term);
>>>> void parse_events_terms__delete(struct list_head *terms);
>>>> void parse_events_terms__purge(struct list_head *terms);
>>>> +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb);
>>>> int parse_events__modifier_event(struct list_head *list, char *str, bool add);
>>>> int parse_events__modifier_group(struct list_head *list, char *event_mod);
>>>> int parse_events_name(struct list_head *list, const char *name);
>>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>>> index b3f8f3f1e900..8dbb7008877e 100644
>>>> --- a/tools/perf/util/pmu.c
>>>> +++ b/tools/perf/util/pmu.c
>>>> @@ -507,12 +507,11 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
>>>> const char *desc, const char *val, FILE *val_fd,
>>>> const struct pmu_event *pe)
>>>> {
>>>> - struct parse_events_term *term;
>>>> struct perf_pmu_alias *alias;
>>>> int ret;
>>>> - char newval[256];
>>>> const char *long_desc = NULL, *topic = NULL, *unit = NULL, *pmu_name = NULL;
>>>> bool deprecated = false, perpkg = false;
>>>> + struct strbuf sb;
>>>>
>>>> if (perf_pmu__find_alias(pmu, name, /*load=*/ false)) {
>>>> /* Alias was already created/loaded. */
>>>> @@ -582,20 +581,10 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
>>>> *
>>>> * Rebuild string to make alias->str member comparable.
>>>> */
>>>> - ret = 0;
>>>> - list_for_each_entry(term, &alias->terms, list) {
>>>> - if (ret)
>>>> - ret += scnprintf(newval + ret, sizeof(newval) - ret,
>>>> - ",");
>>>> - if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
>>>> - ret += scnprintf(newval + ret, sizeof(newval) - ret,
>>>> - "%s=%#x", term->config, term->val.num);
>>>> - else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
>>>> - ret += scnprintf(newval + ret, sizeof(newval) - ret,
>>>> - "%s=%s", term->config, term->val.str);
>>>> - }
>>>> zfree(&alias->str);
>>>> - alias->str = strdup(newval);
>>>> + strbuf_init(&sb, /*hint=*/ 0);
>>>> + parse_events_term__to_strbuf(&alias->terms, &sb);
>>>> + alias->str = strbuf_detach(&sb, /*sz=*/ NULL);
>>>> if (!pe)
>>>> pmu->sysfs_aliases++;
>>>> else
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper
2023-08-30 18:29 ` Liang, Kan
@ 2023-08-30 19:34 ` Liang, Kan
2023-08-30 21:12 ` Ian Rogers
0 siblings, 1 reply; 10+ messages in thread
From: Liang, Kan @ 2023-08-30 19:34 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, James Clark, linux-perf-users, linux-kernel
On 2023-08-30 2:29 p.m., Liang, Kan wrote:
>> The problem
>> is how we encode terms of an event name, it is indistinguishable when
>> the of the user field is 1. I'll probably add something to fix this
>> later, but it only impacts debug output and perf list, so I'm not
>> overly worried.
> It should be OK for the debug output.
>
Not just the debug output. It also impacts the perf list --detail.
With the patch,
perf list --detail | grep uops_retired.heavy -A 2
uops_retired.heavy
[Retired uops except the last uop of each instruction. Unit:
cpu_core]
cpu_core/event=0xc2,period=0x1e8483,umask/
Without the patch,
perf list --detail | grep uops_retired.heavy -A 2
uops_retired.heavy
[Retired uops except the last uop of each instruction. Unit:
cpu_core]
cpu_core/event=0xc2,period=0x1e8483,umask=0x1/
Thanks,
Kan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper
2023-08-30 19:34 ` Liang, Kan
@ 2023-08-30 21:12 ` Ian Rogers
0 siblings, 0 replies; 10+ messages in thread
From: Ian Rogers @ 2023-08-30 21:12 UTC (permalink / raw)
To: Liang, Kan
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, James Clark, linux-perf-users, linux-kernel
On Wed, Aug 30, 2023 at 12:35 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2023-08-30 2:29 p.m., Liang, Kan wrote:
> >> The problem
> >> is how we encode terms of an event name, it is indistinguishable when
> >> the of the user field is 1. I'll probably add something to fix this
> >> later, but it only impacts debug output and perf list, so I'm not
> >> overly worried.
> > It should be OK for the debug output.
> >
>
> Not just the debug output. It also impacts the perf list --detail.
>
> With the patch,
> perf list --detail | grep uops_retired.heavy -A 2
> uops_retired.heavy
> [Retired uops except the last uop of each instruction. Unit:
> cpu_core]
> cpu_core/event=0xc2,period=0x1e8483,umask/
>
> Without the patch,
> perf list --detail | grep uops_retired.heavy -A 2
> uops_retired.heavy
> [Retired uops except the last uop of each instruction. Unit:
> cpu_core]
> cpu_core/event=0xc2,period=0x1e8483,umask=0x1/
Right, nobody uses --detail ;-) I'll write the fix.
Thanks,
Ian
> Thanks,
> Kan
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/3] perf parse-events: Minor help message improvements
2023-08-30 7:07 [PATCH v1 1/3] perf parse-events: Minor help message improvements Ian Rogers
2023-08-30 7:07 ` [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper Ian Rogers
2023-08-30 7:07 ` [PATCH v1 3/3] perf pmu: Remove str from perf_pmu_alias Ian Rogers
@ 2023-08-31 18:32 ` Liang, Kan
2 siblings, 0 replies; 10+ messages in thread
From: Liang, Kan @ 2023-08-31 18:32 UTC (permalink / raw)
To: Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, James Clark, linux-perf-users, linux-kernel
The other thread address the only issue I found when I tested the patch
series on a hybrid machine.
https://lore.kernel.org/lkml/c2affcc9-468f-bf4c-a080-65b31e05a83f@linux.intel.com/
The patch series looks good.
Tested-by: Kan Liang <kan.liang@linux.intel.com>
Thanks,
Kan
On 2023-08-30 3:07 a.m., Ian Rogers wrote:
> Be more specific and fix a typo.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/parse-events.y | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 3a9d4e2513b5..4a370c36a0d5 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -342,7 +342,7 @@ PE_NAME opt_pmu_config
> struct parse_events_error *error = parse_state->error;
> char *help;
>
> - if (asprintf(&help, "Unabled to find PMU or event on a PMU of '%s'", $1) < 0)
> + if (asprintf(&help, "Unable to find PMU or event on a PMU of '%s'", $1) < 0)
> help = NULL;
> parse_events_error__handle(error, @1.first_column,
> strdup("Bad event or PMU"),
> @@ -368,7 +368,7 @@ PE_NAME sep_dc
> struct parse_events_error *error = parse_state->error;
> char *help;
>
> - if (asprintf(&help, "Unabled to find PMU or event on a PMU of '%s'", $1) < 0)
> + if (asprintf(&help, "Unable to find event on a PMU of '%s'", $1) < 0)
> help = NULL;
> parse_events_error__handle(error, @1.first_column, strdup("Bad event name"), help);
> free($1);
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-08-31 18:33 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-30 7:07 [PATCH v1 1/3] perf parse-events: Minor help message improvements Ian Rogers
2023-08-30 7:07 ` [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper Ian Rogers
2023-08-30 14:11 ` Liang, Kan
2023-08-30 14:40 ` Ian Rogers
2023-08-30 15:04 ` Ian Rogers
2023-08-30 18:29 ` Liang, Kan
2023-08-30 19:34 ` Liang, Kan
2023-08-30 21:12 ` Ian Rogers
2023-08-30 7:07 ` [PATCH v1 3/3] perf pmu: Remove str from perf_pmu_alias Ian Rogers
2023-08-31 18:32 ` [PATCH v1 1/3] perf parse-events: Minor help message improvements Liang, Kan
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).