Re: [PATCH v4 1/2] perf pmu: Add PMU alias support
From: Jin, Yao
Date: Thu Aug 12 2021 - 21:59:21 EST
Hi John,
On 8/12/2021 8:14 PM, John Garry wrote:
perf pmu: Add x86 PMU alias support
+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);
I would not expect a function which does a "find" to duplicate the name.
Same goes for all the other places which does similar.
Yes, you are right. Jiri has the similar comment for the name duplicating here. We will still keep
the 'pmu->name = strdup(name);' in pmu_lookup() in v5.
+ }
+ 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 fc683bc41715..796a4be752f4 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -946,6 +946,18 @@ perf_pmu__get_default_config(struct perf_pmu *pmu __maybe_unused)
return NULL;
}
+char * __weak
+pmu_find_real_name(const char *name)
+{
+ return strdup(name);
+}
That's not finding anything.
Will just return name in non-x86 implementation.
pmu_find_real_name(const char *name)
{
return name;
}
+
+char * __weak
+pmu_find_alias_name(const char *name __maybe_unused)
+{
+ return NULL;
> +}
+
static int pmu_max_precise(const char *name)
{
char path[PATH_MAX];
@@ -959,19 +971,25 @@ static int pmu_max_precise(const char *name)
return max_precise;
}
-static struct perf_pmu *pmu_lookup(const char *name)
+static struct perf_pmu *pmu_lookup(const char *lookup_name)
{
- struct perf_pmu *pmu;
+ struct perf_pmu *pmu = NULL;
LIST_HEAD(format);
LIST_HEAD(aliases);
__u32 type;
- bool is_hybrid = perf_pmu__hybrid_mounted(name);
+ bool is_hybrid;
+ char *name = pmu_find_real_name(lookup_name);
+
+ if (!name)
+ return NULL;
+
+ is_hybrid = perf_pmu__hybrid_mounted(name);
/*
* Check pmu name for hybrid and the pmu may be invalid in sysfs
*/
if (!strncmp(name, "cpu_", 4) && !is_hybrid)
- return NULL;
+ goto out;
/*
* The pmu data we store & need consists of the pmu
@@ -979,23 +997,24 @@ static struct perf_pmu *pmu_lookup(const char *name)
* now.
*/
if (pmu_format(name, &format))
- return NULL;
+ goto out;
/*
* Check the type first to avoid unnecessary work.
*/
if (pmu_type(name, &type))
- return NULL;
+ goto out;
if (pmu_aliases(name, &aliases))
- return NULL;
+ goto out;
pmu = zalloc(sizeof(*pmu));
if (!pmu)
- return NULL;
+ goto out;
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)
@@ -1017,6 +1036,10 @@ static struct perf_pmu *pmu_lookup(const char *name)
pmu->default_config = perf_pmu__get_default_config(pmu);
+out:
+ if (!pmu)
+ free(name);
I don't understand this. There are lots of places this function can fail , but we don't free
memories allocated previously - why just free this one?
That's because it's only introduced by my patch (duplicating name). :)
But if we keep 'pmu->name = strdup(name)' in pmu_lookup(), I'm OK to keep original code/behavior.
+
return pmu;
}
@@ -1025,7 +1048,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;
I'd be inclined to use {} for the list_for_each_entry() call
OK, I will add {}, thanks!
return NULL;
@@ -1920,6 +1944,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 */
useless comment
OK, remove it :)
Thanks
Jin Yao
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 */