Re: [PATCH v2 1/7] perf: Remove duplication around EVENT_SOURCE_DEVICE_PATH

On Thu, Dec 22, 2022 at 04:03:21PM +0000, James Clark wrote:

The pattern for accessing EVENT_SOURCE_DEVICE_PATH is duplicated in a few places, so add two utility functions to cover it. Also just use perf_pmu__scan_file() instead of pmu_type() which already does the same thing.

No functional changes.

After I read the article: https://lwn.net/Articles/69419/, good to see this patch uses scnprintf() to replace snprintf().

Signed-off-by: James Clark james.clark@arm.com

tools/perf/arch/arm/util/auxtrace.c | 5 +- tools/perf/arch/x86/util/pmu.c | 12 +-- tools/perf/util/pmu.c | 110 +++++++++++----------------- tools/perf/util/pmu.h | 5 +- 4 files changed, 49 insertions(+), 83 deletions(-)

diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c index deeb163999ce..3cb4a6a16112 100644 --- a/tools/perf/arch/arm/util/auxtrace.c +++ b/tools/perf/arch/arm/util/auxtrace.c @@ -55,17 +55,16 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err) static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int *err) {

• const char *sysfs = sysfs__mountpoint(); struct perf_pmu **hisi_ptt_pmus = NULL; struct dirent *dent; char path[PATH_MAX]; DIR *dir = NULL; int idx = 0;

• snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);

• perf_pmu__event_source_devices_scnprintf(path, PATH_MAX);

Nitpick: since 'path' is an array, a good practice is to use 'sizeof(path)' rather than 'PATH_MAX' for passing the length.

dir = opendir(path); if (!dir) {

• pr_err("can't read directory '%s'\n", EVENT_SOURCE_DEVICE_PATH);
  

• pr_err("can't read directory '%s'\n", path);
  

  *err = -EINVAL; return NULL; }

diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c index 74d69db1ea99..e3456d4b9063 100644 --- a/tools/perf/arch/x86/util/pmu.c +++ b/tools/perf/arch/x86/util/pmu.c @@ -15,8 +15,6 @@ #include "../../../util/pmu.h" #include "../../../util/fncache.h" -#define TEMPLATE_ALIAS "%s/bus/event_source/devices/%s/alias"

struct pmu_alias { char *name; char *alias; @@ -72,18 +70,14 @@ static int setup_pmu_alias_list(void) char path[PATH_MAX]; DIR *dir; struct dirent *dent;

• const char *sysfs = sysfs__mountpoint(); struct pmu_alias *pmu_alias; char buf[MAX_PMU_NAME_LEN]; FILE *file; int ret = -ENOMEM;

• if (!sysfs)

• if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX)) return -1;

• snprintf(path, PATH_MAX,

•  "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
  

• dir = opendir(path); if (!dir) return -errno;

@@ -93,9 +87,7 @@ static int setup_pmu_alias_list(void) !strcmp(dent->d_name, "..")) continue;

• snprintf(path, PATH_MAX,
  

• 	 TEMPLATE_ALIAS, sysfs, dent->d_name);
  

• perf_pmu__pathname_scnprintf(path, PATH_MAX, dent->d_name, "alias");
  

  if (!file_available(path)) continue;

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 2bdeb89352e7..827c1a6bb99a 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -107,14 +107,10 @@ int perf_pmu__format_parse(char *dir, struct list_head *head) static int pmu_format(const char *name, struct list_head *format) { char path[PATH_MAX];

• const char *sysfs = sysfs__mountpoint();

• if (!sysfs)

• if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "format")) return -1;

• snprintf(path, PATH_MAX,

•  "%s" EVENT_SOURCE_DEVICE_PATH "%s/format", sysfs, name);
  

• if (!file_available(path)) return 0;

@@ -513,14 +509,10 @@ static int pmu_aliases_parse(char *dir, struct list_head *head) static int pmu_aliases(const char *name, struct list_head *head) { char path[PATH_MAX];

• const char *sysfs = sysfs__mountpoint();

• if (!sysfs)

• if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "events")) return -1;

• snprintf(path, PATH_MAX,

•  "%s/bus/event_source/devices/%s/events", sysfs, name);
  

• if (!file_available(path)) return 0;

@@ -554,52 +546,16 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias, return 0; } -/*

• • Reading/parsing the default pmu type value, which should be
• • located at:
• • /sys/bus/event_source/devices/<dev>/type as sysfs attribute.
• */

-static int pmu_type(const char *name, __u32 *type) -{

• char path[PATH_MAX];
• FILE *file;
• int ret = 0;
• const char *sysfs = sysfs__mountpoint();
• if (!sysfs)

• return -1;
  

• snprintf(path, PATH_MAX,

•  "%s" EVENT_SOURCE_DEVICE_PATH "%s/type", sysfs, name);
  

• if (access(path, R_OK) < 0)

• return -1;
  

• file = fopen(path, "r");
• if (!file)

• return -EINVAL;
  

• if (1 != fscanf(file, "%u", type))

• ret = -1;
  

• fclose(file);
• return ret;

-}

/* Add all pmus in sysfs to pmu list: */ static void pmu_read_sysfs(void) { char path[PATH_MAX]; DIR *dir; struct dirent *dent;

• const char *sysfs = sysfs__mountpoint();

• if (!sysfs)

• if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX)) return;

• snprintf(path, PATH_MAX,

•  "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
  

• dir = opendir(path); if (!dir) return;

@@ -696,14 +652,9 @@ static char *pmu_id(const char *name) static int is_arm_pmu_core(const char *name) { char path[PATH_MAX];

• const char *sysfs = sysfs__mountpoint();

• if (!sysfs)

• if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "cpus")) return 0;

• /* Look for cpu sysfs (specific to arm) */
• scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/cpus",

• 		sysfs, name);
  

  return file_available(path);

} @@ -969,11 +920,8 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name) return NULL; /*

• * Check the type first to avoid unnecessary work.
  

• * Check the aliases first to avoid unnecessary work.
  

  */

• if (pmu_type(name, &type))

• return NULL;
  

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

@@ -983,9 +931,14 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name) pmu->cpus = pmu_cpumask(name); pmu->name = strdup(name);

• if (!pmu->name) goto err;

• /* Read type, and ensure that 1 value (type) is successfully assigned */

Maybe I don't understand well, seems to me a better comment is:

/* Read type, and ensure that type value is successfully assigned (return 1) */

The rest looks good to me. With addressing above two comments:

Reviewed-by: Leo Yan leo.yan@linaro.org

• if (perf_pmu__scan_file(pmu, "type", "%u", &type) != 1)

• goto err;
  

• alias_name = pmu_find_alias_name(name); if (alias_name) { pmu->alias_name = strdup(alias_name);

@@ -1786,16 +1739,11 @@ bool pmu_have_event(const char *pname, const char *name) static FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name) { char path[PATH_MAX];

• const char *sysfs;

• sysfs = sysfs__mountpoint();
• if (!sysfs)

• if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, pmu->name, name) ||

•    !file_available(path))
  

  return NULL;

• snprintf(path, PATH_MAX,

•  "%s" EVENT_SOURCE_DEVICE_PATH "%s/%s", sysfs, pmu->name, name);
  

• if (!file_available(path))

• return NULL;
  

  return fopen(path, "r");

} @@ -1849,7 +1797,6 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu) { struct stat st; char caps_path[PATH_MAX];

• const char *sysfs = sysfs__mountpoint(); DIR *caps_dir; struct dirent *evt_ent;

@@ -1858,12 +1805,9 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu) pmu->nr_caps = 0;

• if (!sysfs)

• if (!perf_pmu__pathname_scnprintf(caps_path, PATH_MAX, pmu->name, "caps")) return -1;

• snprintf(caps_path, PATH_MAX,

•  "%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name);
  

• if (stat(caps_path, &st) < 0) { pmu->caps_initialized = true; return 0; /* no error if caps does not exist */

@@ -1993,3 +1937,31 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus, *ucpus_ptr = unmatched_cpus; return 0; }

+int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size) +{

• const char *sysfs = sysfs__mountpoint();
• if (!sysfs)

• return 0;
  

• return scnprintf(pathname, size, "%s/bus/event_source/devices/", sysfs);

+}

+/*

• • Fill 'buf' with the path to a file or folder in 'pmu_name' in
• • sysfs. For example if pmu_name = "cs_etm" and 'filename' = "format"
• • then pathname will be filled with
• • "/sys/bus/event_source/devices/cs_etm/format"
• • Return 0 if the sysfs mountpoint couldn't be found or if no
• • characters were written.
• */

+int perf_pmu__pathname_scnprintf(char *buf, size_t size,

• 		 const char *pmu_name, const char *filename)
  

+{

• char base_path[PATH_MAX];
• if (!perf_pmu__event_source_devices_scnprintf(base_path, PATH_MAX))

• return 0;
  

• return scnprintf(buf, size, "%s%s/%s", base_path, pmu_name, filename);

+} diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h index 69ca0004f94f..2f2bb0286e2a 100644 --- a/tools/perf/util/pmu.h +++ b/tools/perf/util/pmu.h @@ -22,7 +22,6 @@ 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 @@ -259,4 +258,8 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus, char *pmu_find_real_name(const char *name); char *pmu_find_alias_name(const char *name); +int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size); +int perf_pmu__pathname_scnprintf(char *buf, size_t size,

• 		 const char *pmu_name, const char *filename);
  

#endif /* __PMU_H */

2.25.1