Messages in this thread | | | Subject | Re: [PATCH v2 1/2] perf pmu: Add PMU alias support | From | "Jin, Yao" <> | Date | Fri, 30 Jul 2021 09:43:09 +0800 |
| |
Hi Riccardo,
On 7/29/2021 9:13 PM, Riccardo Mancini wrote: > Hi, > > On Thu, 2021-07-29 at 15:06 +0800, Jin Yao wrote: >> From: Kan Liang <kan.liang@linux.intel.com> >> >> A perf uncore PMU may have two PMU names, a real name and an alias. The >> alias is exported at /sys/bus/event_source/devices/uncore_*/alias. >> The perf tool should support the alias as well. >> >> Add alias_name in the struct perf_pmu to store the alias. For the PMU >> which doesn't have an alias. It's NULL. >> >> Introduce two X86 specific functions to retrieve the real name and the >> alias separately. >> >> Only go through the sysfs to retrieve the mapping between the real name >> and the alias once. The result is cached in a list, uncore_pmu_list. >> >> Nothing changed for the other ARCHs. >> >> With the patch, the perf tool can monitor the PMU with either the real >> name or the alias. >> >> Use the real name, >> $ perf stat -e uncore_cha_2/event=1/ -x, >> 4044879584,,uncore_cha_2/event=1/,2528059205,100.00,, >> >> Use the alias, >> $ perf stat -e uncore_type_0_2/event=1/ -x, >> 3659675336,,uncore_type_0_2/event=1/,2287306455,100.00,, >> >> Co-developed-by: Jin Yao <yao.jin@linux.intel.com> >> Signed-off-by: Jin Yao <yao.jin@linux.intel.com> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> --- >> v2: >> - No change. >> >> tools/perf/arch/x86/util/pmu.c | 109 ++++++++++++++++++++++++++++++++- >> tools/perf/util/parse-events.y | 3 +- >> tools/perf/util/pmu.c | 26 +++++++- >> tools/perf/util/pmu.h | 5 ++ >> 4 files changed, 138 insertions(+), 5 deletions(-) >> >> diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c >> index d48d608517fd..bb79b1d19b96 100644 >> --- a/tools/perf/arch/x86/util/pmu.c >> +++ b/tools/perf/arch/x86/util/pmu.c >> @@ -1,12 +1,28 @@ >> // SPDX-License-Identifier: GPL-2.0 >> #include <string.h> >> - >> +#include <stdio.h> >> +#include <sys/types.h> >> +#include <dirent.h> >> +#include <fcntl.h> >> #include <linux/stddef.h> >> #include <linux/perf_event.h> >> +#include <linux/zalloc.h> >> +#include <api/fs/fs.h> >> >> #include "../../../util/intel-pt.h" >> #include "../../../util/intel-bts.h" >> #include "../../../util/pmu.h" >> +#include "../../../util/fncache.h" >> + >> +#define TEMPLATE_ALIAS "%s/bus/event_source/devices/%s/alias" >> + >> +struct perf_pmu_alias_name { >> + char *name; >> + char *alias; >> + struct list_head list; >> +}; >> + >> +static LIST_HEAD(pmu_alias_name_list); >> >> struct perf_event_attr *perf_pmu__get_default_config(struct perf_pmu *pmu >> __maybe_unused) >> { >> @@ -18,3 +34,94 @@ struct perf_event_attr *perf_pmu__get_default_config(struct >> perf_pmu *pmu __mayb >> #endif >> return NULL; >> } >> + >> +static void setup_pmu_alias_list(void) >> +{ >> + char path[PATH_MAX]; >> + DIR *dir; >> + struct dirent *dent; >> + const char *sysfs = sysfs__mountpoint(); >> + struct perf_pmu_alias_name *pmu; >> + char buf[MAX_PMU_NAME_LEN]; >> + FILE *file; >> + >> + if (!sysfs) >> + return; >> + >> + snprintf(path, PATH_MAX, >> + "%s" EVENT_SOURCE_DEVICE_PATH, sysfs); >> + >> + dir = opendir(path); >> + if (!dir) >> + return; >> + >> + while ((dent = readdir(dir))) { >> + if (!strcmp(dent->d_name, ".") || >> + !strcmp(dent->d_name, "..")) >> + continue; >> + >> + snprintf(path, PATH_MAX, >> + TEMPLATE_ALIAS, sysfs, dent->d_name); >> + >> + if (!file_available(path)) >> + continue; >> + >> + file = fopen(path, "r"); >> + if (!file) >> + continue; >> + >> + if (fscanf(file, "%s", buf) != 1) >> + continue; > > Using fscanf with "%s" can lead to a buffer overflow. > It's better to use fgets, if possible, or to specify a maximum length (better if > through a macro, e.g. "%" STR(MAX_PMU_NAME_LEN) "s"). > Yes, fgets should be better. I will use 'fgets(buf, sizeof(buf), file)' instead in v3.
>> + >> + pmu = zalloc(sizeof(*pmu)); >> + if (!pmu) >> + continue; >> + >> + pmu->alias = strdup(buf); >> + if (!pmu->alias) { >> + free(pmu); >> + continue; >> + } >> + pmu->name = strdup(dent->d_name); > > The result of strdup is not checked. > It's also probably better to return an error in case of ENOMEM, instead of > continuing iteration, as also previous checks do. >
Yes, strictly we should check the return value, I will fix that.
Thanks Jin Yao
> Thanks, > Riccardo > >> + list_add_tail(&pmu->list, &pmu_alias_name_list); >> + fclose(file); >> + } >> + >> + closedir(dir); >> +} >> + >> +static char *__pmu_find_real_name(const char *name) >> +{ >> + struct perf_pmu_alias_name *pmu; >> + >> + list_for_each_entry(pmu, &pmu_alias_name_list, list) { >> + if (!strcmp(name, pmu->alias)) >> + return strdup(pmu->name); >> + } >> + >> + return strdup(name); >> +} >> + >> +char *pmu_find_real_name(const char *name) >> +{ >> + static bool cached_list; >> + >> + if (cached_list) >> + return __pmu_find_real_name(name); >> + >> + setup_pmu_alias_list(); >> + cached_list = true; >> + >> + return __pmu_find_real_name(name); >> +} >> + >> +char *pmu_find_alias_name(const char *name) >> +{ >> + struct perf_pmu_alias_name *pmu; >> + >> + list_for_each_entry(pmu, &pmu_alias_name_list, list) { >> + if (!strcmp(name, pmu->name)) >> + return strdup(pmu->alias); >> + } >> + return NULL; >> +} >> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y >> index 9321bd0e2f76..d94e48e1ff9b 100644 >> --- a/tools/perf/util/parse-events.y >> +++ b/tools/perf/util/parse-events.y >> @@ -316,7 +316,8 @@ event_pmu_name opt_pmu_config >> if (!strncmp(name, "uncore_", 7) && >> strncmp($1, "uncore_", 7)) >> name += 7; >> - if (!perf_pmu__match(pattern, name, $1)) { >> + if (!perf_pmu__match(pattern, name, $1) || >> + !perf_pmu__match(pattern, pmu->alias_name, $1)) { >> if (parse_events_copy_term_list(orig_terms, >> &terms)) >> CLEANUP_YYABORT; >> if (!parse_events_add_pmu(_parse_state, list, >> pmu->name, terms, true, false)) >> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c >> index 44b90d638ad5..cc9af7942e7b 100644 >> --- a/tools/perf/util/pmu.c >> +++ b/tools/perf/util/pmu.c >> @@ -944,13 +944,28 @@ static int pmu_max_precise(const char *name) >> return max_precise; >> } >> >> -static struct perf_pmu *pmu_lookup(const char *name) >> +char * __weak >> +pmu_find_real_name(const char *name) >> +{ >> + return strdup(name); >> +} >> + >> +char * __weak >> +pmu_find_alias_name(const char *name __maybe_unused) >> +{ >> + return NULL; >> +} >> + >> +static struct perf_pmu *pmu_lookup(const char *lookup_name) >> { >> struct perf_pmu *pmu; >> + char *name; >> LIST_HEAD(format); >> LIST_HEAD(aliases); >> __u32 type; >> >> + name = pmu_find_real_name(lookup_name); >> + >> /* >> * The pmu data we store & need consists of the pmu >> * type value and format definitions. Load both right >> @@ -973,7 +988,8 @@ static struct perf_pmu *pmu_lookup(const char *name) >> return NULL; >> >> pmu->cpus = pmu_cpumask(name); >> - pmu->name = strdup(name); >> + pmu->name = name; >> + pmu->alias_name = pmu_find_alias_name(name); >> pmu->type = type; >> pmu->is_uncore = pmu_is_uncore(name); >> if (pmu->is_uncore) >> @@ -1003,7 +1019,8 @@ static struct perf_pmu *pmu_find(const char *name) >> struct perf_pmu *pmu; >> >> list_for_each_entry(pmu, &pmus, list) >> - if (!strcmp(pmu->name, name)) >> + if (!strcmp(pmu->name, name) || >> + (pmu->alias_name && !strcmp(pmu->alias_name, name))) >> return pmu; >> >> return NULL; >> @@ -1898,6 +1915,9 @@ bool perf_pmu__has_hybrid(void) >> >> int perf_pmu__match(char *pattern, char *name, char *tok) >> { >> + if (!name) >> + return -1; >> + >> if (fnmatch(pattern, name, 0)) >> return -1; >> >> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h >> index 926da483a141..f6ca9f6a06ef 100644 >> --- a/tools/perf/util/pmu.h >> +++ b/tools/perf/util/pmu.h >> @@ -21,6 +21,7 @@ enum { >> #define PERF_PMU_FORMAT_BITS 64 >> #define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/" >> #define CPUS_TEMPLATE_CPU "%s/bus/event_source/devices/%s/cpus" >> +#define MAX_PMU_NAME_LEN 128 >> >> struct perf_event_attr; >> >> @@ -32,6 +33,7 @@ struct perf_pmu_caps { >> >> struct perf_pmu { >> char *name; >> + char *alias_name; /* PMU alias name */ >> char *id; >> __u32 type; >> bool selectable; >> @@ -135,4 +137,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, >> __u64 config, >> bool perf_pmu__has_hybrid(void); >> int perf_pmu__match(char *pattern, char *name, char *tok); >> >> +char *pmu_find_real_name(const char *name); >> +char *pmu_find_alias_name(const char *name); >> + >> #endif /* __PMU_H */ > >
| |