| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b976ef43-25ea-f24a-2c22-0976fafcf0f0@linux.intel.com>
Date: Wed, 30 Aug 2023 14:29:50 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
James Clark <james.clark@....com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/3] perf parse-events: Make common term list to strbuf
helper
On 2023-08-30 11:04 a.m., Ian Rogers wrote:
> On Wed, Aug 30, 2023 at 7:40 AM Ian Rogers <irogers@...gle.com> wrote:
>>
>> On Wed, Aug 30, 2023 at 7:15 AM Liang, Kan <kan.liang@...ux.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@...gle.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
Powered by blists - more mailing lists