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

From: Leo Yan
Date: Thu Dec 22 2022 - 21:39:15 EST

Next message: yang.yang29: "[PATCH linux-next] of: base: use strscpy() to instead of strncpy()"
Previous message: yang.yang29: "[PATCH net-next] brcm80211: use strscpy() to instead of strncpy()"
In reply to: James Clark: "[PATCH v2 1/7] perf: Remove duplication around EVENT_SOURCE_DEVICE_PATH"
Next in thread: James Clark: "[PATCH v2 2/7] perf: Use perf_pmu__open_file() and perf_pmu__scan_file()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>

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

Next message: yang.yang29: "[PATCH linux-next] of: base: use strscpy() to instead of strncpy()"
Previous message: yang.yang29: "[PATCH net-next] brcm80211: use strscpy() to instead of strncpy()"
In reply to: James Clark: "[PATCH v2 1/7] perf: Remove duplication around EVENT_SOURCE_DEVICE_PATH"
Next in thread: James Clark: "[PATCH v2 2/7] perf: Use perf_pmu__open_file() and perf_pmu__scan_file()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]