[5/9] perf pmu: Use relative path for sysfs scan

  The PMU information is in the kernel sysfs so it needs to scan the
directory to get the whole information like event aliases, formats
and so on.  During the traversal, it opens a lot of files and
directories like below:

  dir = opendir("/sys/bus/event_source/devices");
  while (dentry = readdir(dir)) {
    char buf[PATH_MAX];

    snprintf(buf, sizeof(buf), "%s/%s",
             "/sys/bus/event_source/devices", dentry->d_name);
    fd = open(buf, O_RDONLY);
    ...
  }

But this is not good since it needs to copy the string to build the
absolute pathname, and it makes redundant pathname walk (from the /sys)
unnecessarily.  We can use openat(2) to open the file in the given
directory.  While it's not a problem ususally, it can be a problem
when the kernel has contentions on the sysfs.

Add a couple of new helper to return the file descriptor of PMU
directory so that it can use it with relative paths.

 * perf_pmu__event_source_devices_fd()
   - returns a fd for the PMU root ("/sys/bus/event_source/devices")

 * perf_pmu__pathname_fd()
   - returns a fd for "<pmu>/<file>" under the PMU root

Now the above code can be converted something like below:

  dirfd = perf_pmu__event_source_devices_fd();
  dir = fdopendir(dirfd);
  while (dentry = readdir(dir)) {
    fd = openat(dirfd, dentry->d_name, O_RDONLY);
    ...
  }

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/pmu.c |   9 ++-
 tools/perf/util/pmu.c  | 161 +++++++++++++++++++++++++----------------
 tools/perf/util/pmu.h  |   4 +-
 3 files changed, 111 insertions(+), 63 deletions(-)
  

On Fri, Mar 31, 2023 at 1:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The PMU information is in the kernel sysfs so it needs to scan the
> directory to get the whole information like event aliases, formats
> and so on.  During the traversal, it opens a lot of files and
> directories like below:
>
>   dir = opendir("/sys/bus/event_source/devices");
>   while (dentry = readdir(dir)) {
>     char buf[PATH_MAX];
>
>     snprintf(buf, sizeof(buf), "%s/%s",
>              "/sys/bus/event_source/devices", dentry->d_name);
>     fd = open(buf, O_RDONLY);
>     ...
>   }
>
> But this is not good since it needs to copy the string to build the
> absolute pathname, and it makes redundant pathname walk (from the /sys)
> unnecessarily.  We can use openat(2) to open the file in the given
> directory.  While it's not a problem ususally, it can be a problem
> when the kernel has contentions on the sysfs.
>
> Add a couple of new helper to return the file descriptor of PMU
> directory so that it can use it with relative paths.
>
>  * perf_pmu__event_source_devices_fd()
>    - returns a fd for the PMU root ("/sys/bus/event_source/devices")
>
>  * perf_pmu__pathname_fd()
>    - returns a fd for "<pmu>/<file>" under the PMU root
>
> Now the above code can be converted something like below:
>
>   dirfd = perf_pmu__event_source_devices_fd();
>   dir = fdopendir(dirfd);
>   while (dentry = readdir(dir)) {
>     fd = openat(dirfd, dentry->d_name, O_RDONLY);
>     ...
>   }
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/tests/pmu.c |   9 ++-
>  tools/perf/util/pmu.c  | 161 +++++++++++++++++++++++++----------------
>  tools/perf/util/pmu.h  |   4 +-
>  3 files changed, 111 insertions(+), 63 deletions(-)
>
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index 8507bd615e97..3cf25f883df7 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -3,6 +3,7 @@
>  #include "pmu.h"
>  #include "tests.h"
>  #include <errno.h>
> +#include <fcntl.h>
>  #include <stdio.h>
>  #include <linux/kernel.h>
>  #include <linux/limits.h>
> @@ -149,10 +150,16 @@ static int test__pmu(struct test_suite *test __maybe_unused, int subtest __maybe
>
>         do {
>                 struct perf_event_attr attr;
> +               int fd;
>
>                 memset(&attr, 0, sizeof(attr));
>
> -               ret = perf_pmu__format_parse(format, &formats);
> +               fd = open(format, O_DIRECTORY);
> +               if (fd < 0) {
> +                       ret = fd;
> +                       break;
> +               }
> +               ret = perf_pmu__format_parse(fd, &formats);
>                 if (ret)
>                         break;
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index b112606f36ec..9fc6b8b5732b 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -62,38 +62,38 @@ extern FILE *perf_pmu_in;
>
>  static bool hybrid_scanned;
>
> +static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name);
> +
>  /*
>   * Parse & process all the sysfs attributes located under
>   * the directory specified in 'dir' parameter.
>   */
> -int perf_pmu__format_parse(char *dir, struct list_head *head)
> +int perf_pmu__format_parse(int dirfd, struct list_head *head)
>  {
>         struct dirent *evt_ent;
>         DIR *format_dir;
>         int ret = 0;
>
> -       format_dir = opendir(dir);
> +       format_dir = fdopendir(dirfd);
>         if (!format_dir)
>                 return -EINVAL;
>
>         while (!ret && (evt_ent = readdir(format_dir))) {
> -               char path[PATH_MAX];
>                 char *name = evt_ent->d_name;
> -               FILE *file;
> +               int fd;
>
>                 if (!strcmp(name, ".") || !strcmp(name, ".."))
>                         continue;
>
> -               snprintf(path, PATH_MAX, "%s/%s", dir, name);
>
>                 ret = -EINVAL;
> -               file = fopen(path, "r");
> -               if (!file)
> +               fd = openat(dirfd, name, O_RDONLY);
> +               if (fd < 0)
>                         break;
>
> -               perf_pmu_in = file;
> +               perf_pmu_in = fdopen(fd, "r");
>                 ret = perf_pmu_parse(head, name);
> -               fclose(file);
> +               fclose(perf_pmu_in);

Eek, I hadn't realized that the pmu parser wasn't reentrant. I sent out:
https://lore.kernel.org/lkml/20230403172031.1759781-1-irogers@google.com/
to fix this.

Thanks,
Ian

>         }
>
>         closedir(format_dir);
> @@ -105,17 +105,16 @@ int perf_pmu__format_parse(char *dir, struct list_head *head)
>   * located at:
>   * /sys/bus/event_source/devices/<dev>/format as sysfs group attributes.
>   */
> -static int pmu_format(const char *name, struct list_head *format)
> +static int pmu_format(int dirfd, const char *name, struct list_head *format)
>  {
> -       char path[PATH_MAX];
> -
> -       if (!perf_pmu__pathname_scnprintf(path, sizeof(path), name, "format"))
> -               return -1;
> +       int fd;
>
> -       if (!file_available(path))
> +       fd = perf_pmu__pathname_fd(dirfd, name, "format", O_DIRECTORY);
> +       if (fd < 0)
>                 return 0;
>
> -       if (perf_pmu__format_parse(path, format))
> +       /* it'll close the fd */
> +       if (perf_pmu__format_parse(fd, format))
>                 return -1;
>
>         return 0;
> @@ -158,7 +157,7 @@ int perf_pmu__convert_scale(const char *scale, char **end, double *sval)
>         return ret;
>  }
>
> -static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *name)
> +static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, int dirfd, char *name)
>  {
>         struct stat st;
>         ssize_t sret;
> @@ -166,9 +165,9 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
>         int fd, ret = -1;
>         char path[PATH_MAX];
>
> -       scnprintf(path, PATH_MAX, "%s/%s.scale", dir, name);
> +       scnprintf(path, PATH_MAX, "%s.scale", name);
>
> -       fd = open(path, O_RDONLY);
> +       fd = openat(dirfd, path, O_RDONLY);
>         if (fd == -1)
>                 return -1;
>
> @@ -190,15 +189,15 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
>         return ret;
>  }
>
> -static int perf_pmu__parse_unit(struct perf_pmu_alias *alias, char *dir, char *name)
> +static int perf_pmu__parse_unit(struct perf_pmu_alias *alias, int dirfd, char *name)
>  {
>         char path[PATH_MAX];
>         ssize_t sret;
>         int fd;
>
> -       scnprintf(path, PATH_MAX, "%s/%s.unit", dir, name);
> +       scnprintf(path, PATH_MAX, "%s.unit", name);
>
> -       fd = open(path, O_RDONLY);
> +       fd = openat(dirfd, path, O_RDONLY);
>         if (fd == -1)
>                 return -1;
>
> @@ -221,14 +220,14 @@ static int perf_pmu__parse_unit(struct perf_pmu_alias *alias, char *dir, char *n
>  }
>
>  static int
> -perf_pmu__parse_per_pkg(struct perf_pmu_alias *alias, char *dir, char *name)
> +perf_pmu__parse_per_pkg(struct perf_pmu_alias *alias, int dirfd, char *name)
>  {
>         char path[PATH_MAX];
>         int fd;
>
> -       scnprintf(path, PATH_MAX, "%s/%s.per-pkg", dir, name);
> +       scnprintf(path, PATH_MAX, "%s.per-pkg", name);
>
> -       fd = open(path, O_RDONLY);
> +       fd = openat(dirfd, path, O_RDONLY);
>         if (fd == -1)
>                 return -1;
>
> @@ -239,14 +238,14 @@ perf_pmu__parse_per_pkg(struct perf_pmu_alias *alias, char *dir, char *name)
>  }
>
>  static int perf_pmu__parse_snapshot(struct perf_pmu_alias *alias,
> -                                   char *dir, char *name)
> +                                   int dirfd, char *name)
>  {
>         char path[PATH_MAX];
>         int fd;
>
> -       scnprintf(path, PATH_MAX, "%s/%s.snapshot", dir, name);
> +       scnprintf(path, PATH_MAX, "%s.snapshot", name);
>
> -       fd = open(path, O_RDONLY);
> +       fd = openat(dirfd, path, O_RDONLY);
>         if (fd == -1)
>                 return -1;
>
> @@ -332,7 +331,7 @@ static bool perf_pmu_merge_alias(struct perf_pmu_alias *newalias,
>         return false;
>  }
>
> -static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
> +static int __perf_pmu__new_alias(struct list_head *list, int dirfd, char *name,
>                                  char *desc, char *val, const struct pmu_event *pe)
>  {
>         struct parse_events_term *term;
> @@ -391,14 +390,14 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
>         }
>
>         alias->name = strdup(name);
> -       if (dir) {
> +       if (dirfd >= 0) {
>                 /*
>                  * load unit name and scale if available
>                  */
> -               perf_pmu__parse_unit(alias, dir, name);
> -               perf_pmu__parse_scale(alias, dir, name);
> -               perf_pmu__parse_per_pkg(alias, dir, name);
> -               perf_pmu__parse_snapshot(alias, dir, name);
> +               perf_pmu__parse_unit(alias, dirfd, name);
> +               perf_pmu__parse_scale(alias, dirfd, name);
> +               perf_pmu__parse_per_pkg(alias, dirfd, name);
> +               perf_pmu__parse_snapshot(alias, dirfd, name);
>         }
>
>         alias->desc = desc ? strdup(desc) : NULL;
> @@ -419,7 +418,7 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
>         return 0;
>  }
>
> -static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FILE *file)
> +static int perf_pmu__new_alias(struct list_head *list, int dirfd, char *name, FILE *file)
>  {
>         char buf[256];
>         int ret;
> @@ -433,7 +432,7 @@ static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FI
>         /* Remove trailing newline from sysfs file */
>         strim(buf);
>
> -       return __perf_pmu__new_alias(list, dir, name, NULL, buf, NULL);
> +       return __perf_pmu__new_alias(list, dirfd, name, NULL, buf, NULL);
>  }
>
>  static inline bool pmu_alias_info_file(char *name)
> @@ -457,17 +456,17 @@ static inline bool pmu_alias_info_file(char *name)
>   * Process all the sysfs attributes located under the directory
>   * specified in 'dir' parameter.
>   */
> -static int pmu_aliases_parse(char *dir, struct list_head *head)
> +static int pmu_aliases_parse(int dirfd, struct list_head *head)
>  {
>         struct dirent *evt_ent;
>         DIR *event_dir;
> +       int fd;
>
> -       event_dir = opendir(dir);
> +       event_dir = fdopendir(dirfd);
>         if (!event_dir)
>                 return -EINVAL;
>
>         while ((evt_ent = readdir(event_dir))) {
> -               char path[PATH_MAX];
>                 char *name = evt_ent->d_name;
>                 FILE *file;
>
> @@ -480,15 +479,14 @@ static int pmu_aliases_parse(char *dir, struct list_head *head)
>                 if (pmu_alias_info_file(name))
>                         continue;
>
> -               scnprintf(path, PATH_MAX, "%s/%s", dir, name);
> -
> -               file = fopen(path, "r");
> +               fd = openat(dirfd, name, O_RDONLY);
> +               file = fdopen(fd, "r");
>                 if (!file) {
> -                       pr_debug("Cannot open %s\n", path);
> +                       pr_debug("Cannot open %s\n", name);
>                         continue;
>                 }
>
> -               if (perf_pmu__new_alias(head, dir, name, file) < 0)
> +               if (perf_pmu__new_alias(head, dirfd, name, file) < 0)
>                         pr_debug("Cannot set up %s\n", name);
>                 fclose(file);
>         }
> @@ -501,17 +499,16 @@ static int pmu_aliases_parse(char *dir, struct list_head *head)
>   * Reading the pmu event aliases definition, which should be located at:
>   * /sys/bus/event_source/devices/<dev>/events as sysfs group attributes.
>   */
> -static int pmu_aliases(const char *name, struct list_head *head)
> +static int pmu_aliases(int dirfd, const char *name, struct list_head *head)
>  {
> -       char path[PATH_MAX];
> -
> -       if (!perf_pmu__pathname_scnprintf(path, sizeof(path), name, "events"))
> -               return -1;
> +       int fd;
>
> -       if (!file_available(path))
> +       fd = perf_pmu__pathname_fd(dirfd, name, "events", O_DIRECTORY);
> +       if (fd < 0)
>                 return 0;
>
> -       if (pmu_aliases_parse(path, head))
> +       /* it'll close the fd */
> +       if (pmu_aliases_parse(fd, head))
>                 return -1;
>
>         return 0;
> @@ -544,14 +541,15 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias,
>  /* Add all pmus in sysfs to pmu list: */
>  static void pmu_read_sysfs(void)
>  {
> -       char path[PATH_MAX];
> +       int fd;
>         DIR *dir;
>         struct dirent *dent;
>
> -       if (!perf_pmu__event_source_devices_scnprintf(path, sizeof(path)))
> +       fd = perf_pmu__event_source_devices_fd();
> +       if (fd < 0)
>                 return;
>
> -       dir = opendir(path);
> +       dir = fdopendir(fd);
>         if (!dir)
>                 return;
>
> @@ -559,7 +557,7 @@ static void pmu_read_sysfs(void)
>                 if (!strcmp(dent->d_name, ".") || !strcmp(dent->d_name, ".."))
>                         continue;
>                 /* add to static LIST_HEAD(pmus): */
> -               perf_pmu__find(dent->d_name);
> +               perf_pmu__find2(fd, dent->d_name);
>         }
>
>         closedir(dir);
> @@ -763,7 +761,7 @@ static int pmu_add_cpu_aliases_map_callback(const struct pmu_event *pe,
>
>  new_alias:
>         /* need type casts to override 'const' */
> -       __perf_pmu__new_alias(data->head, NULL, (char *)pe->name, (char *)pe->desc,
> +       __perf_pmu__new_alias(data->head, -1, (char *)pe->name, (char *)pe->desc,
>                               (char *)pe->event, pe);
>         return 0;
>  }
> @@ -814,7 +812,7 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
>
>         if (!strcmp(pmu->id, pe->compat) &&
>             pmu_uncore_alias_match(pe->pmu, pmu->name)) {
> -               __perf_pmu__new_alias(idata->head, NULL,
> +               __perf_pmu__new_alias(idata->head, -1,
>                                       (char *)pe->name,
>                                       (char *)pe->desc,
>                                       (char *)pe->event,
> @@ -863,7 +861,7 @@ static int pmu_max_precise(struct perf_pmu *pmu)
>         return max_precise;
>  }
>
> -static struct perf_pmu *pmu_lookup(const char *lookup_name)
> +static struct perf_pmu *pmu_lookup(int dirfd, const char *lookup_name)
>  {
>         struct perf_pmu *pmu;
>         LIST_HEAD(format);
> @@ -884,13 +882,13 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
>          * type value and format definitions. Load both right
>          * now.
>          */
> -       if (pmu_format(name, &format))
> +       if (pmu_format(dirfd, name, &format))
>                 return NULL;
>
>         /*
>          * Check the aliases first to avoid unnecessary work.
>          */
> -       if (pmu_aliases(name, &aliases))
> +       if (pmu_aliases(dirfd, name, &aliases))
>                 return NULL;
>
>         pmu = zalloc(sizeof(*pmu));
> @@ -1024,6 +1022,27 @@ bool evsel__is_aux_event(const struct evsel *evsel)
>  }
>
>  struct perf_pmu *perf_pmu__find(const char *name)
> +{
> +       struct perf_pmu *pmu;
> +       int dirfd;
> +
> +       /*
> +        * Once PMU is loaded it stays in the list,
> +        * so we keep us from multiple reading/parsing
> +        * the pmu format definitions.
> +        */
> +       pmu = pmu_find(name);
> +       if (pmu)
> +               return pmu;
> +
> +       dirfd = perf_pmu__event_source_devices_fd();
> +       pmu = pmu_lookup(dirfd, name);
> +       close(dirfd);
> +
> +       return pmu;
> +}
> +
> +static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
>  {
>         struct perf_pmu *pmu;
>
> @@ -1036,7 +1055,7 @@ struct perf_pmu *perf_pmu__find(const char *name)
>         if (pmu)
>                 return pmu;
>
> -       return pmu_lookup(name);
> +       return pmu_lookup(dirfd, name);
>  }
>
>  static struct perf_pmu_format *
> @@ -1938,6 +1957,18 @@ int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size)
>         return scnprintf(pathname, size, "%s/bus/event_source/devices/", sysfs);
>  }
>
> +int perf_pmu__event_source_devices_fd(void)
> +{
> +       char path[PATH_MAX];
> +       const char *sysfs = sysfs__mountpoint();
> +
> +       if (!sysfs)
> +               return -1;
> +
> +       scnprintf(path, sizeof(path), "%s/bus/event_source/devices/", sysfs);
> +       return open(path, O_DIRECTORY);
> +}
> +
>  /*
>   * 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"
> @@ -1957,6 +1988,14 @@ int perf_pmu__pathname_scnprintf(char *buf, size_t size,
>         return scnprintf(buf, size, "%s%s/%s", base_path, pmu_name, filename);
>  }
>
> +int perf_pmu__pathname_fd(int dirfd, const char *pmu_name, const char *filename, int flags)
> +{
> +       char path[PATH_MAX];
> +
> +       scnprintf(path, sizeof(path), "%s/%s", pmu_name, filename);
> +       return openat(dirfd, path, flags);
> +}
> +
>  static void perf_pmu__delete(struct perf_pmu *pmu)
>  {
>         perf_pmu__del_formats(&pmu->format);
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 72fd5de334c0..751c7016e7b6 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -211,7 +211,7 @@ void perf_pmu_error(struct list_head *list, char *name, char const *msg);
>  int perf_pmu__new_format(struct list_head *list, char *name,
>                          int config, unsigned long *bits);
>  void perf_pmu__set_format(unsigned long *bits, long from, long to);
> -int perf_pmu__format_parse(char *dir, struct list_head *head);
> +int perf_pmu__format_parse(int dirfd, struct list_head *head);
>  void perf_pmu__del_formats(struct list_head *formats);
>
>  struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu);
> @@ -257,6 +257,8 @@ double perf_pmu__cpu_slots_per_cycle(void);
>  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);
> +int perf_pmu__event_source_devices_fd(void);
> +int perf_pmu__pathname_fd(int dirfd, const char *pmu_name, const char *filename, int flags);
>  FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name);
>
>  void perf_pmu__destroy(void);
> --
> 2.40.0.348.gf938b09366-goog
>