Messages in this thread | | | From | Ian Rogers <> | Date | Mon, 3 Apr 2023 10:23:27 -0700 | Subject | Re: [PATCH 5/9] perf pmu: Use relative path for sysfs scan |
| |
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 >
| |