[PATCH v2 0/2] perf tools: Add PMU alias support

[PATCH v2 0/2] perf tools: Add PMU alias support

[Jin Yao]

A perf uncore PMU may have two PMU names, a real name and an alias name.
With this patch set, 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,,

v2:
---
Add test case to verify the real name and alias name having same effect.

Jin Yao (1):
  perf tests: Test for PMU alias

Kan Liang (1):
  perf pmu: Add PMU alias support

 tools/perf/arch/x86/util/pmu.c  | 109 +++++++++++++++++++++++++++++++-
 tools/perf/tests/parse-events.c |  79 +++++++++++++++++++++++
 tools/perf/util/parse-events.y  |   3 +-
 tools/perf/util/pmu.c           |  26 +++++++-
 tools/perf/util/pmu.h           |   5 ++
 5 files changed, 217 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] perf pmu: Add PMU alias support

[Jin Yao]

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;
+
+		pmu = zalloc(sizeof(*pmu));
+		if (!pmu)
+			continue;
+
+		pmu->alias = strdup(buf);
+		if (!pmu->alias) {
+			free(pmu);
+			continue;
+		}
+		pmu->name = strdup(dent->d_name);
+		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 */
-- 
2.17.1

Re: [PATCH v2 1/2] perf pmu: Add PMU alias support

[Riccardo Mancini]

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").

> +
> +               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.

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 */

Re: [PATCH v2 1/2] perf pmu: Add PMU alias support

[Jin, Yao]

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 */
> 
> 

Re: [PATCH v2 1/2] perf pmu: Add PMU alias support

[Riccardo Mancini]

Hi,

sorry for sending a separate email, but I found another problem while testing
John's patchset with ASan:
$ make DEBUG=1 EXTRA_CFLAGS='-O0 -g -fno-omit-frame-pointer -fsanitize=address'

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;
> +
> +               pmu = zalloc(sizeof(*pmu));
> +               if (!pmu)
> +                       continue;
> +
> +               pmu->alias = strdup(buf);
> +               if (!pmu->alias) {
> +                       free(pmu);
> +                       continue;
> +               }
> +               pmu->name = strdup(dent->d_name);
> +               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);
> +

name is not freed if one of the following checks fails.

	/*
	 * The pmu data we store & need consists of the pmu
	 * type value and format definitions. Load both right
	 * now.
	 */
	if (pmu_format(name, &format))
		return NULL;

	/*
	 * Check the type first to avoid unnecessary work.
	 */
	if (pmu_type(name, &type))
		return NULL;

	if (pmu_aliases(name, &aliases))
		return NULL;

Thanks,
Riccardo

> 
> @@ -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 */

[PATCH v2 2/2] perf tests: Test for PMU alias

[Jin Yao]

A perf uncore PMU may have two PMU names, a real name and an alias
name. Add one test case to verify the real name and alias name having
the same effect.

Iterate the sysfs to get one event which has an alias and create
evlist by adding two evsels. Evsel1 is created by event and evsel2
is created by alias.

Test asserts:
evsel1->core.attr.type == evsel2->core.attr.type
evsel1->core.attr.config == evsel2->core.attr.config

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
v2:
 - New in v2.

 tools/perf/tests/parse-events.c | 79 +++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 56a7b6a14195..b416851e4074 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -6,6 +6,7 @@
 #include "tests.h"
 #include "debug.h"
 #include "pmu.h"
+#include "fncache.h"
 #include <dirent.h>
 #include <errno.h>
 #include <sys/types.h>
@@ -2190,9 +2191,79 @@ static int test_pmu_events(void)
 	return ret;
 }
 
+static bool test_alias(char **event, char **alias)
+{
+	char path[PATH_MAX];
+	DIR *dir;
+	struct dirent *dent;
+	const char *sysfs = sysfs__mountpoint();
+	char buf[128];
+	FILE *file;
+
+	if (!sysfs)
+		return false;
+
+	snprintf(path, PATH_MAX, "%s/bus/event_source/devices/", sysfs);
+	dir = opendir(path);
+	if (!dir)
+		return false;
+
+	while ((dent = readdir(dir))) {
+		if (!strcmp(dent->d_name, ".") ||
+		    !strcmp(dent->d_name, ".."))
+			continue;
+
+		snprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/alias",
+			 sysfs, dent->d_name);
+
+		if (!file_available(path))
+			continue;
+
+		file = fopen(path, "r");
+		if (!file)
+			continue;
+
+		if (fscanf(file, "%s", buf) != 1) {
+			fclose(file);
+			continue;
+		}
+
+		fclose(file);
+		*event = strdup(dent->d_name);
+		*alias = strdup(buf);
+		return true;
+	}
+
+	return false;
+}
+
+static int test__checkevent_pmu_events_alias(struct evlist *evlist)
+{
+	struct evsel *evsel1 = evlist__first(evlist);
+	struct evsel *evsel2 = evlist__last(evlist);
+
+	TEST_ASSERT_VAL("wrong type", evsel1->core.attr.type == evsel2->core.attr.type);
+	TEST_ASSERT_VAL("wrong config", evsel1->core.attr.config == evsel2->core.attr.config);
+	return 0;
+}
+
+static int test_pmu_events_alias(char *event, char *alias)
+{
+	struct evlist_test e = { .id = 0, };
+	char name[2 * NAME_MAX + 20];
+
+	snprintf(name, sizeof(name), "%s/event=1/,%s/event=1/",
+		 event, alias);
+
+	e.name  = name;
+	e.check = test__checkevent_pmu_events_alias;
+	return test_event(&e);
+}
+
 int test__parse_events(struct test *test __maybe_unused, int subtest __maybe_unused)
 {
 	int ret1, ret2 = 0;
+	char *event, *alias;
 
 #define TEST_EVENTS(tests)				\
 do {							\
@@ -2217,6 +2288,14 @@ do {							\
 			return ret;
 	}
 
+	if (test_alias(&event, &alias)) {
+		int ret = test_pmu_events_alias(event, alias);
+		free(event);
+		free(alias);
+		if (ret)
+			return ret;
+	}
+
 	ret1 = test_terms(test__terms, ARRAY_SIZE(test__terms));
 	if (!ret2)
 		ret2 = ret1;
-- 
2.17.1

Re: [PATCH v2 2/2] perf tests: Test for PMU alias

[Riccardo Mancini]

Hi,

On Thu, 2021-07-29 at 15:06 +0800, Jin Yao wrote:
> A perf uncore PMU may have two PMU names, a real name and an alias
> name. Add one test case to verify the real name and alias name having
> the same effect.
> 
> Iterate the sysfs to get one event which has an alias and create
> evlist by adding two evsels. Evsel1 is created by event and evsel2
> is created by alias.
> 
> Test asserts:
> evsel1->core.attr.type == evsel2->core.attr.type
> evsel1->core.attr.config == evsel2->core.attr.config
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
> v2:
>  - New in v2.
> 
>  tools/perf/tests/parse-events.c | 79 +++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 56a7b6a14195..b416851e4074 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -6,6 +6,7 @@
>  #include "tests.h"
>  #include "debug.h"
>  #include "pmu.h"
> +#include "fncache.h"
>  #include <dirent.h>
>  #include <errno.h>
>  #include <sys/types.h>
> @@ -2190,9 +2191,79 @@ static int test_pmu_events(void)
>         return ret;
>  }
>  
> +static bool test_alias(char **event, char **alias)
> +{
> +       char path[PATH_MAX];
> +       DIR *dir;
> +       struct dirent *dent;
> +       const char *sysfs = sysfs__mountpoint();
> +       char buf[128];
> +       FILE *file;
> +
> +       if (!sysfs)
> +               return false;
> +
> +       snprintf(path, PATH_MAX, "%s/bus/event_source/devices/", sysfs);
> +       dir = opendir(path);
> +       if (!dir)
> +               return false;
> +
> +       while ((dent = readdir(dir))) {
> +               if (!strcmp(dent->d_name, ".") ||
> +                   !strcmp(dent->d_name, ".."))
> +                       continue;
> +
> +               snprintf(path, PATH_MAX,
> "%s/bus/event_source/devices/%s/alias",
> +                        sysfs, dent->d_name);
> +
> +               if (!file_available(path))
> +                       continue;
> +
> +               file = fopen(path, "r");
> +               if (!file)
> +                       continue;
> +
> +               if (fscanf(file, "%s", buf) != 1) {
> +                       fclose(file);
> +                       continue;
> +               }

ditto as in the first patch.

> +
> +               fclose(file);
> +               *event = strdup(dent->d_name);
> +               *alias = strdup(buf);
> +               return true;
> +       }

dir is never closed.

Thanks,
Riccardo

> +
> +       return false;
> +}
> +
> +static int test__checkevent_pmu_events_alias(struct evlist *evlist)
> +{
> +       struct evsel *evsel1 = evlist__first(evlist);
> +       struct evsel *evsel2 = evlist__last(evlist);
> +
> +       TEST_ASSERT_VAL("wrong type", evsel1->core.attr.type == evsel2-
> >core.attr.type);
> +       TEST_ASSERT_VAL("wrong config", evsel1->core.attr.config == evsel2-
> >core.attr.config);
> +       return 0;
> +}
> +
> +static int test_pmu_events_alias(char *event, char *alias)
> +{
> +       struct evlist_test e = { .id = 0, };
> +       char name[2 * NAME_MAX + 20];
> +
> +       snprintf(name, sizeof(name), "%s/event=1/,%s/event=1/",
> +                event, alias);
> +
> +       e.name  = name;
> +       e.check = test__checkevent_pmu_events_alias;
> +       return test_event(&e);
> +}
> +
>  int test__parse_events(struct test *test __maybe_unused, int subtest
> __maybe_unused)
>  {
>         int ret1, ret2 = 0;
> +       char *event, *alias;
>  
>  #define TEST_EVENTS(tests)                             \
>  do {                                                   \
> @@ -2217,6 +2288,14 @@ do
> {                                                     \
>                         return ret;
>         }
>  
> +       if (test_alias(&event, &alias)) {
> +               int ret = test_pmu_events_alias(event, alias);
> +               free(event);
> +               free(alias);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         ret1 = test_terms(test__terms, ARRAY_SIZE(test__terms));
>         if (!ret2)
>                 ret2 = ret1;

Re: [PATCH v2 2/2] perf tests: Test for PMU alias

[Jin, Yao]

Hi Riccardo,

On 7/29/2021 9:15 PM, Riccardo Mancini wrote:
> Hi,
> 
> On Thu, 2021-07-29 at 15:06 +0800, Jin Yao wrote:
>> A perf uncore PMU may have two PMU names, a real name and an alias
>> name. Add one test case to verify the real name and alias name having
>> the same effect.
>>
>> Iterate the sysfs to get one event which has an alias and create
>> evlist by adding two evsels. Evsel1 is created by event and evsel2
>> is created by alias.
>>
>> Test asserts:
>> evsel1->core.attr.type == evsel2->core.attr.type
>> evsel1->core.attr.config == evsel2->core.attr.config
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>> v2:
>>   - New in v2.
>>
>>   tools/perf/tests/parse-events.c | 79 +++++++++++++++++++++++++++++++++
>>   1 file changed, 79 insertions(+)
>>
>> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
>> index 56a7b6a14195..b416851e4074 100644
>> --- a/tools/perf/tests/parse-events.c
>> +++ b/tools/perf/tests/parse-events.c
>> @@ -6,6 +6,7 @@
>>   #include "tests.h"
>>   #include "debug.h"
>>   #include "pmu.h"
>> +#include "fncache.h"
>>   #include <dirent.h>
>>   #include <errno.h>
>>   #include <sys/types.h>
>> @@ -2190,9 +2191,79 @@ static int test_pmu_events(void)
>>          return ret;
>>   }
>>   
>> +static bool test_alias(char **event, char **alias)
>> +{
>> +       char path[PATH_MAX];
>> +       DIR *dir;
>> +       struct dirent *dent;
>> +       const char *sysfs = sysfs__mountpoint();
>> +       char buf[128];
>> +       FILE *file;
>> +
>> +       if (!sysfs)
>> +               return false;
>> +
>> +       snprintf(path, PATH_MAX, "%s/bus/event_source/devices/", sysfs);
>> +       dir = opendir(path);
>> +       if (!dir)
>> +               return false;
>> +
>> +       while ((dent = readdir(dir))) {
>> +               if (!strcmp(dent->d_name, ".") ||
>> +                   !strcmp(dent->d_name, ".."))
>> +                       continue;
>> +
>> +               snprintf(path, PATH_MAX,
>> "%s/bus/event_source/devices/%s/alias",
>> +                        sysfs, dent->d_name);
>> +
>> +               if (!file_available(path))
>> +                       continue;
>> +
>> +               file = fopen(path, "r");
>> +               if (!file)
>> +                       continue;
>> +
>> +               if (fscanf(file, "%s", buf) != 1) {
>> +                       fclose(file);
>> +                       continue;
>> +               }
> 
> ditto as in the first patch.
> 

Got it, thanks!

>> +
>> +               fclose(file);
>> +               *event = strdup(dent->d_name);
>> +               *alias = strdup(buf);
>> +               return true;
>> +       }
> 
> dir is never closed.
> 

Oh yes, should add "closedir(dir);" at the end of function.

Thanks
Jin Yao

> Thanks,
> Riccardo
> 
>> +
>> +       return false;
>> +}
>> +
>> +static int test__checkevent_pmu_events_alias(struct evlist *evlist)
>> +{
>> +       struct evsel *evsel1 = evlist__first(evlist);
>> +       struct evsel *evsel2 = evlist__last(evlist);
>> +
>> +       TEST_ASSERT_VAL("wrong type", evsel1->core.attr.type == evsel2-
>>> core.attr.type);
>> +       TEST_ASSERT_VAL("wrong config", evsel1->core.attr.config == evsel2-
>>> core.attr.config);
>> +       return 0;
>> +}
>> +
>> +static int test_pmu_events_alias(char *event, char *alias)
>> +{
>> +       struct evlist_test e = { .id = 0, };
>> +       char name[2 * NAME_MAX + 20];
>> +
>> +       snprintf(name, sizeof(name), "%s/event=1/,%s/event=1/",
>> +                event, alias);
>> +
>> +       e.name  = name;
>> +       e.check = test__checkevent_pmu_events_alias;
>> +       return test_event(&e);
>> +}
>> +
>>   int test__parse_events(struct test *test __maybe_unused, int subtest
>> __maybe_unused)
>>   {
>>          int ret1, ret2 = 0;
>> +       char *event, *alias;
>>   
>>   #define TEST_EVENTS(tests)                             \
>>   do {                                                   \
>> @@ -2217,6 +2288,14 @@ do
>> {                                                     \
>>                          return ret;
>>          }
>>   
>> +       if (test_alias(&event, &alias)) {
>> +               int ret = test_pmu_events_alias(event, alias);
>> +               free(event);
>> +               free(alias);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>>          ret1 = test_terms(test__terms, ARRAY_SIZE(test__terms));
>>          if (!ret2)
>>                  ret2 = ret1;
> 
>