[PATCHSET 0/9] perf tools: Update pmu scan using openat() (v1)

[PATCHSET 0/9] perf tools: Update pmu scan using openat() (v1)

[Namhyung Kim]

Hello,

This patchset changes PMU info scanning on sysfs using openat()
basically.  I got reports of occasional contention on the
opening files in sysfs.  While the root cause was a separate
issue, I discovered some inefficiencies in the perf code.

To scan PMUs, it roughly does something 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)
in the kernel unnecessarily.  We can use openat(2) to open the file in
the given directory.

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);
    ...
  }

I added a benchmark for pmu-scan and it showed a slight speedup
in the normal case too.

  $ ./perf.old bench internals pmu-scan
  # Running 'internals/pmu-scan' benchmark:
  Computing performance of sysfs PMU event scan for 100 times
    Average PMU scanning took: 6670.970 usec (+- 13.022 usec)

  $ ./perf.new bench internals pmu-scan
  # Running 'internals/pmu-scan' benchmark:
  Computing performance of sysfs PMU event scan for 100 times
    Average PMU scanning took: 6296.980 usec (+- 14.891 usec)

The 5~6% of improvement might be small but it may have bigger impact
when the system is contended.

You can get the code from 'perf/pmu-scan-v1' branch in

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung

Namhyung Kim (9):
  perf list: Use relative path for tracepoint scan
  perf tools: Fix a asan issue in parse_events_multi_pmu_add()
  perf pmu: Add perf_pmu__destroy() function
  perf bench: Add pmu-scan benchmark
  perf pmu: Use relative path for sysfs scan
  perf pmu: Use relative path in perf_pmu__caps_parse()
  perf pmu: Use relative path in setup_pmu_alias_list()
  perf pmu: Add perf_pmu__{open,scan}_file_at()
  perf intel-pt: Use perf_pmu__scan_file_at() if possible

 tools/perf/arch/x86/util/intel-pt.c |  52 ++++--
 tools/perf/arch/x86/util/pmu.c      |  13 +-
 tools/perf/bench/Build              |   1 +
 tools/perf/bench/bench.h            |   1 +
 tools/perf/bench/pmu-scan.c         | 184 ++++++++++++++++++
 tools/perf/builtin-bench.c          |   1 +
 tools/perf/tests/pmu.c              |   9 +-
 tools/perf/util/parse-events.c      |   2 +-
 tools/perf/util/pmu.c               | 278 ++++++++++++++++++++--------
 tools/perf/util/pmu.h               |  12 +-
 tools/perf/util/print-events.c      |  26 ++-
 11 files changed, 466 insertions(+), 113 deletions(-)
 create mode 100644 tools/perf/bench/pmu-scan.c


base-commit: 417c6adfb155f906f0441cc1034827f6e2b3c372
-- 
2.40.0.348.gf938b09366-goog

[PATCH 1/9] perf list: Use relative path for tracepoint scan

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/print-events.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
index 62e9ea7dcf40..26a7e017c928 100644
--- a/tools/perf/util/print-events.c
+++ b/tools/perf/util/print-events.c
@@ -4,6 +4,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <fcntl.h>
 #include <sys/param.h>
 
 #include <api/fs/tracing_path.h>
@@ -59,12 +60,20 @@ static const struct event_symbol event_symbols_tool[PERF_TOOL_MAX] = {
 void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state)
 {
 	struct dirent **sys_namelist = NULL;
+	char *events_path = get_tracing_file("events");
 	int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
+	int events_fd = open(events_path, O_PATH);
+
+	put_tracing_file(events_path);
+	if (events_fd < 0) {
+		printf("Error: failed to open tracing events directory\n");
+		return;
+	}
 
 	for (int i = 0; i < sys_items; i++) {
 		struct dirent *sys_dirent = sys_namelist[i];
 		struct dirent **evt_namelist = NULL;
-		char *dir_path;
+		int dir_fd;
 		int evt_items;
 
 		if (sys_dirent->d_type != DT_DIR ||
@@ -72,22 +81,26 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
 		    !strcmp(sys_dirent->d_name, ".."))
 			continue;
 
-		dir_path = get_events_file(sys_dirent->d_name);
-		if (!dir_path)
+		dir_fd = openat(events_fd, sys_dirent->d_name, O_PATH);
+		if (dir_fd < 0)
 			continue;
 
-		evt_items = scandir(dir_path, &evt_namelist, NULL, alphasort);
+		evt_items = scandirat(events_fd, sys_dirent->d_name, &evt_namelist, NULL, alphasort);
 		for (int j = 0; j < evt_items; j++) {
 			struct dirent *evt_dirent = evt_namelist[j];
 			char evt_path[MAXPATHLEN];
+			int evt_fd;
 
 			if (evt_dirent->d_type != DT_DIR ||
 			    !strcmp(evt_dirent->d_name, ".") ||
 			    !strcmp(evt_dirent->d_name, ".."))
 				continue;
 
-			if (tp_event_has_id(dir_path, evt_dirent) != 0)
+			snprintf(evt_path, sizeof(evt_path), "%s/id", evt_dirent->d_name);
+			evt_fd = openat(dir_fd, evt_path, O_RDONLY);
+			if (evt_fd < 0)
 				continue;
+			close(evt_fd);
 
 			snprintf(evt_path, MAXPATHLEN, "%s:%s",
 				 sys_dirent->d_name, evt_dirent->d_name);
@@ -103,10 +116,11 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
 					/*long_desc=*/NULL,
 					/*encoding_desc=*/NULL);
 		}
-		free(dir_path);
+		close(dir_fd);
 		free(evt_namelist);
 	}
 	free(sys_namelist);
+	close(events_fd);
 }
 
 void print_sdt_events(const struct print_callbacks *print_cb, void *print_state)
-- 
2.40.0.348.gf938b09366-goog

[PATCH 2/9] perf tools: Fix a asan issue in parse_events_multi_pmu_add()

[Namhyung Kim]

In the parse_events_multi_pmu_add() it passes the 'config' variable
twice to parse_events_term__num() - one for config and another for
loc_term.  I'm not sure about the second one as it's converted to
YYLTYPE variable.  Asan reports it like below:

  In function ‘parse_events_term__num’,
      inlined from ‘parse_events_multi_pmu_add’ at util/parse-events.c:1602:6:
  util/parse-events.c:2653:64: error: array subscript ‘YYLTYPE[0]’ is partly outside
                                      array bounds of ‘char[8]’ [-Werror=array-bounds]
   2653 |                 .err_term  = loc_term ? loc_term->first_column : 0,
        |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
  util/parse-events.c: In function ‘parse_events_multi_pmu_add’:
  util/parse-events.c:1587:15: note: object ‘config’ of size 8
   1587 |         char *config;
        |               ^~~~~~
  cc1: all warnings being treated as errors

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/parse-events.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index cc8e8766ca30..0010e5e0ee68 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1601,7 +1601,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 
 	if (parse_events_term__num(&term,
 				   PARSE_EVENTS__TERM_TYPE_USER,
-				   config, 1, false, &config,
+				   config, 1, false, NULL,
 					NULL) < 0) {
 		free(config);
 		goto out_err;
-- 
2.40.0.348.gf938b09366-goog

[PATCH 3/9] perf pmu: Add perf_pmu__destroy() function

[Namhyung Kim]

It seems there's no function to delete the perf pmu struct.  Add the
perf_pmu__destroy() to do the job.  While at it, add some more helper
functions to delete pmu aliases and caps.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/pmu.c | 50 +++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/pmu.h |  2 ++
 2 files changed, 52 insertions(+)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index e3aae731bd6f..b112606f36ec 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -300,6 +300,16 @@ void perf_pmu_free_alias(struct perf_pmu_alias *newalias)
 	free(newalias);
 }
 
+static void perf_pmu__del_aliases(struct perf_pmu *pmu)
+{
+	struct perf_pmu_alias *alias, *tmp;
+
+	list_for_each_entry_safe(alias, tmp, &pmu->aliases, list) {
+		list_del(&alias->list);
+		perf_pmu_free_alias(alias);
+	}
+}
+
 /* Merge an alias, search in alias list. If this name is already
  * present merge both of them to combine all information.
  */
@@ -921,6 +931,8 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
 
 	if (is_hybrid)
 		list_add_tail(&pmu->hybrid_list, &perf_pmu__hybrid_pmus);
+	else
+		INIT_LIST_HEAD(&pmu->hybrid_list);
 
 	pmu->default_config = perf_pmu__get_default_config(pmu);
 
@@ -1750,6 +1762,18 @@ static int perf_pmu__new_caps(struct list_head *list, char *name, char *value)
 	return -ENOMEM;
 }
 
+static void perf_pmu__del_caps(struct perf_pmu *pmu)
+{
+	struct perf_pmu_caps *caps, *tmp;
+
+	list_for_each_entry_safe(caps, tmp, &pmu->caps, list) {
+		list_del(&caps->list);
+		free(caps->name);
+		free(caps->value);
+		free(caps);
+	}
+}
+
 /*
  * Reading/parsing the given pmu capabilities, which should be located at:
  * /sys/bus/event_source/devices/<dev>/caps as sysfs group attributes.
@@ -1932,3 +1956,29 @@ int perf_pmu__pathname_scnprintf(char *buf, size_t size,
 		return 0;
 	return scnprintf(buf, size, "%s%s/%s", base_path, pmu_name, filename);
 }
+
+static void perf_pmu__delete(struct perf_pmu *pmu)
+{
+	perf_pmu__del_formats(&pmu->format);
+	perf_pmu__del_aliases(pmu);
+	perf_pmu__del_caps(pmu);
+
+	perf_cpu_map__put(pmu->cpus);
+
+	free(pmu->default_config);
+	free(pmu->name);
+	free(pmu->alias_name);
+	free(pmu);
+}
+
+void perf_pmu__destroy(void)
+{
+	struct perf_pmu *pmu, *tmp;
+
+	list_for_each_entry_safe(pmu, tmp, &pmus, list) {
+		list_del(&pmu->list);
+		list_del(&pmu->hybrid_list);
+
+		perf_pmu__delete(pmu);
+	}
+}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 24cf69ab32cd..72fd5de334c0 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -259,4 +259,6 @@ int perf_pmu__pathname_scnprintf(char *buf, size_t size,
 				 const char *pmu_name, const char *filename);
 FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name);
 
+void perf_pmu__destroy(void);
+
 #endif /* __PMU_H */
-- 
2.40.0.348.gf938b09366-goog

[PATCH 4/9] perf bench: Add pmu-scan benchmark

[Namhyung Kim]

The pmu-scan benchmark will repeatedly scan the sysfs to get the
available PMU information.

  $ ./perf bench internals pmu-scan
  # Running 'internals/pmu-scan' benchmark:
  Computing performance of sysfs PMU event scan for 100 times
    Average PMU scanning took: 6850.990 usec (+- 48.445 usec)

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/bench/Build      |   1 +
 tools/perf/bench/bench.h    |   1 +
 tools/perf/bench/pmu-scan.c | 184 ++++++++++++++++++++++++++++++++++++
 tools/perf/builtin-bench.c  |   1 +
 4 files changed, 187 insertions(+)
 create mode 100644 tools/perf/bench/pmu-scan.c

diff --git a/tools/perf/bench/Build b/tools/perf/bench/Build
index 6b6155a8ad09..0f158dc8139b 100644
--- a/tools/perf/bench/Build
+++ b/tools/perf/bench/Build
@@ -15,6 +15,7 @@ perf-y += find-bit-bench.o
 perf-y += inject-buildid.o
 perf-y += evlist-open-close.o
 perf-y += breakpoint.o
+perf-y += pmu-scan.o
 
 perf-$(CONFIG_X86_64) += mem-memcpy-x86-64-asm.o
 perf-$(CONFIG_X86_64) += mem-memset-x86-64-asm.o
diff --git a/tools/perf/bench/bench.h b/tools/perf/bench/bench.h
index edfc25793cca..0d2b65976212 100644
--- a/tools/perf/bench/bench.h
+++ b/tools/perf/bench/bench.h
@@ -42,6 +42,7 @@ int bench_inject_build_id(int argc, const char **argv);
 int bench_evlist_open_close(int argc, const char **argv);
 int bench_breakpoint_thread(int argc, const char **argv);
 int bench_breakpoint_enable(int argc, const char **argv);
+int bench_pmu_scan(int argc, const char **argv);
 
 #define BENCH_FORMAT_DEFAULT_STR	"default"
 #define BENCH_FORMAT_DEFAULT		0
diff --git a/tools/perf/bench/pmu-scan.c b/tools/perf/bench/pmu-scan.c
new file mode 100644
index 000000000000..f0f007843bb8
--- /dev/null
+++ b/tools/perf/bench/pmu-scan.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Benchmark scanning sysfs files for PMU information.
+ *
+ * Copyright 2023 Google LLC.
+ */
+#include <stdio.h>
+#include "bench.h"
+#include "util/debug.h"
+#include "util/pmu.h"
+#include "util/pmus.h"
+#include "util/stat.h"
+#include <linux/atomic.h>
+#include <linux/err.h>
+#include <linux/time64.h>
+#include <subcmd/parse-options.h>
+
+static unsigned int iterations = 100;
+
+struct pmu_scan_result {
+	char *name;
+	int nr_aliases;
+	int nr_formats;
+	int nr_caps;
+};
+
+static const struct option options[] = {
+	OPT_UINTEGER('i', "iterations", &iterations,
+		"Number of iterations used to compute average"),
+	OPT_END()
+};
+
+static const char *const bench_usage[] = {
+	"perf bench internals pmu-scan <options>",
+	NULL
+};
+
+static int nr_pmus;
+static struct pmu_scan_result *results;
+
+static int save_result(void)
+{
+	struct perf_pmu *pmu;
+	struct list_head *list;
+	struct pmu_scan_result *r;
+
+	perf_pmu__scan(NULL);
+
+	perf_pmus__for_each_pmu(pmu) {
+		r = realloc(results, (nr_pmus + 1) * sizeof(*r));
+		if (r == NULL)
+			return -ENOMEM;
+
+		results = r;
+		r = results + nr_pmus;
+
+		r->name = strdup(pmu->name);
+		r->nr_caps = pmu->nr_caps;
+
+		r->nr_aliases = 0;
+		list_for_each(list, &pmu->aliases)
+			r->nr_aliases++;
+
+		r->nr_formats = 0;
+		list_for_each(list, &pmu->format)
+			r->nr_formats++;
+
+		pr_debug("pmu[%d] name=%s, nr_caps=%d, nr_aliases=%d, nr_formats=%d\n",
+			nr_pmus, r->name, r->nr_caps, r->nr_aliases, r->nr_formats);
+		nr_pmus++;
+	}
+
+	perf_pmu__destroy();
+	return 0;
+}
+
+static int check_result(void)
+{
+	struct pmu_scan_result *r;
+	struct perf_pmu *pmu;
+	struct list_head *list;
+	int nr;
+
+	for (int i = 0; i < nr_pmus; i++) {
+		r = &results[i];
+		pmu = perf_pmu__find(r->name);
+		if (pmu == NULL) {
+			pr_err("Cannot find PMU %s\n", r->name);
+			return -1;
+		}
+
+		if (pmu->nr_caps != (u32)r->nr_caps) {
+			pr_err("Unmatched number of event caps in %s: expect %d vs got %d\n",
+				pmu->name, r->nr_caps, pmu->nr_caps);
+			return -1;
+		}
+
+		nr = 0;
+		list_for_each(list, &pmu->aliases)
+			nr++;
+		if (nr != r->nr_aliases) {
+			pr_err("Unmatched number of event aliases in %s: expect %d vs got %d\n",
+				pmu->name, r->nr_aliases, nr);
+			return -1;
+		}
+
+		nr = 0;
+		list_for_each(list, &pmu->format)
+			nr++;
+		if (nr != r->nr_formats) {
+			pr_err("Unmatched number of event formats in %s: expect %d vs got %d\n",
+				pmu->name, r->nr_formats, nr);
+			return -1;
+		}
+	}
+	return 0;
+}
+
+static void delete_result(void)
+{
+	for (int i = 0; i < nr_pmus; i++)
+		free(results[i].name);
+	free(results);
+
+	results = NULL;
+	nr_pmus = 0;
+}
+
+static int run_pmu_scan(void)
+{
+	struct stats stats;
+	struct timeval start, end, diff;
+	double time_average, time_stddev;
+	u64 runtime_us;
+	unsigned int i;
+	int ret;
+
+	init_stats(&stats);
+	pr_info("Computing performance of sysfs PMU event scan for %u times\n",
+		iterations);
+
+	if (save_result() < 0) {
+		pr_err("Failed to initialize PMU scan result\n");
+		return -1;
+	}
+
+	for (i = 0; i < iterations; i++) {
+		gettimeofday(&start, NULL);
+		perf_pmu__scan(NULL);
+		gettimeofday(&end, NULL);
+
+		timersub(&end, &start, &diff);
+		runtime_us = diff.tv_sec * USEC_PER_SEC + diff.tv_usec;
+		update_stats(&stats, runtime_us);
+
+		ret = check_result();
+		perf_pmu__destroy();
+		if (ret < 0)
+			break;
+	}
+
+	time_average = avg_stats(&stats);
+	time_stddev = stddev_stats(&stats);
+	pr_info("  Average PMU scanning took: %.3f usec (+- %.3f usec)\n",
+		time_average, time_stddev);
+
+	delete_result();
+	return 0;
+}
+
+int bench_pmu_scan(int argc, const char **argv)
+{
+	int err = 0;
+
+	argc = parse_options(argc, argv, options, bench_usage, 0);
+	if (argc) {
+		usage_with_options(bench_usage, options);
+		exit(EXIT_FAILURE);
+	}
+
+	err = run_pmu_scan();
+
+	return err;
+}
diff --git a/tools/perf/builtin-bench.c b/tools/perf/builtin-bench.c
index d0fcc3cdc185..58f1cfe1eb34 100644
--- a/tools/perf/builtin-bench.c
+++ b/tools/perf/builtin-bench.c
@@ -92,6 +92,7 @@ static struct bench internals_benchmarks[] = {
 	{ "kallsyms-parse", "Benchmark kallsyms parsing",	bench_kallsyms_parse	},
 	{ "inject-build-id", "Benchmark build-id injection",	bench_inject_build_id	},
 	{ "evlist-open-close", "Benchmark evlist open and close",	bench_evlist_open_close	},
+	{ "pmu-scan", "Benchmark sysfs PMU info scanning",	bench_pmu_scan		},
 	{ NULL,		NULL,					NULL			}
 };
 
-- 
2.40.0.348.gf938b09366-goog

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

[Namhyung Kim]

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);
 	}
 
 	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

[PATCH 6/9] perf pmu: Use relative path in perf_pmu__caps_parse()

[Namhyung Kim]

Likewise, it needs to traverse the pmu/caps directory, let's use
openat() with the dirfd instead of open() using the absolute path.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/pmu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 9fc6b8b5732b..0c1d87f10b23 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1804,6 +1804,7 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
 	char caps_path[PATH_MAX];
 	DIR *caps_dir;
 	struct dirent *evt_ent;
+	int caps_fd;
 
 	if (pmu->caps_initialized)
 		return pmu->nr_caps;
@@ -1822,18 +1823,19 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
 	if (!caps_dir)
 		return -EINVAL;
 
+	caps_fd = dirfd(caps_dir);
+
 	while ((evt_ent = readdir(caps_dir)) != NULL) {
-		char path[PATH_MAX + NAME_MAX + 1];
 		char *name = evt_ent->d_name;
 		char value[128];
 		FILE *file;
+		int fd;
 
 		if (!strcmp(name, ".") || !strcmp(name, ".."))
 			continue;
 
-		snprintf(path, sizeof(path), "%s/%s", caps_path, name);
-
-		file = fopen(path, "r");
+		fd = openat(caps_fd, name, O_RDONLY);
+		file = fdopen(fd, "r");
 		if (!file)
 			continue;
 
-- 
2.40.0.348.gf938b09366-goog

[PATCH 7/9] perf pmu: Use relative path in setup_pmu_alias_list()

[Namhyung Kim]

Likewise, x86 needs to traverse the PMU list to build alias.
Let's use the new helpers to use relative paths.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/arch/x86/util/pmu.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c
index f73b80dcd8bd..3c0de3370d7e 100644
--- a/tools/perf/arch/x86/util/pmu.c
+++ b/tools/perf/arch/x86/util/pmu.c
@@ -71,7 +71,7 @@ static struct pmu_alias *pmu_alias__new(char *name, char *alias)
 
 static int setup_pmu_alias_list(void)
 {
-	char path[PATH_MAX];
+	int fd, dirfd;
 	DIR *dir;
 	struct dirent *dent;
 	struct pmu_alias *pmu_alias;
@@ -79,10 +79,11 @@ static int setup_pmu_alias_list(void)
 	FILE *file;
 	int ret = -ENOMEM;
 
-	if (!perf_pmu__event_source_devices_scnprintf(path, sizeof(path)))
+	dirfd = perf_pmu__event_source_devices_fd();
+	if (dirfd < 0)
 		return -1;
 
-	dir = opendir(path);
+	dir = fdopendir(dirfd);
 	if (!dir)
 		return -errno;
 
@@ -91,11 +92,11 @@ static int setup_pmu_alias_list(void)
 		    !strcmp(dent->d_name, ".."))
 			continue;
 
-		perf_pmu__pathname_scnprintf(path, sizeof(path), dent->d_name, "alias");
-		if (!file_available(path))
+		fd = perf_pmu__pathname_fd(dirfd, dent->d_name, "alias", O_RDONLY);
+		if (fd < 0)
 			continue;
 
-		file = fopen(path, "r");
+		file = fdopen(fd, "r");
 		if (!file)
 			continue;
 
-- 
2.40.0.348.gf938b09366-goog

[PATCH 8/9] perf pmu: Add perf_pmu__{open,scan}_file_at()

[Namhyung Kim]

These two helpers will also use openat() to reduce the overhead with
relative pathnames.  Convert other functions in pmu_lookup() to use
the new helpers.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/pmu.c | 57 ++++++++++++++++++++++++++++++++++---------
 tools/perf/util/pmu.h |  6 ++++-
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 0c1d87f10b23..78a407b42ad1 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -567,7 +567,7 @@ static void pmu_read_sysfs(void)
  * Uncore PMUs have a "cpumask" file under sysfs. CPU PMUs (e.g. on arm/arm64)
  * may have a "cpus" file.
  */
-static struct perf_cpu_map *pmu_cpumask(const char *name)
+static struct perf_cpu_map *pmu_cpumask(int dirfd, const char *name)
 {
 	struct perf_cpu_map *cpus;
 	const char *templates[] = {
@@ -582,10 +582,11 @@ static struct perf_cpu_map *pmu_cpumask(const char *name)
 
 	strlcpy(pmu_name, name, sizeof(pmu_name));
 	for (template = templates; *template; template++) {
-		file = perf_pmu__open_file(&pmu, *template);
+		file = perf_pmu__open_file_at(&pmu, dirfd, *template);
 		if (!file)
 			continue;
 		cpus = perf_cpu_map__read(file);
+		fclose(file);
 		if (cpus)
 			return cpus;
 	}
@@ -593,15 +594,19 @@ static struct perf_cpu_map *pmu_cpumask(const char *name)
 	return NULL;
 }
 
-static bool pmu_is_uncore(const char *name)
+static bool pmu_is_uncore(int dirfd, const char *name)
 {
-	char path[PATH_MAX];
+	int fd;
 
 	if (perf_pmu__hybrid_mounted(name))
 		return false;
 
-	perf_pmu__pathname_scnprintf(path, sizeof(path), name, "cpumask");
-	return file_available(path);
+	fd = perf_pmu__pathname_fd(dirfd, name, "cpumask", O_PATH);
+	if (fd < 0)
+		return false;
+
+	close(fd);
+	return true;
 }
 
 static char *pmu_id(const char *name)
@@ -853,11 +858,11 @@ pmu_find_alias_name(const char *name __maybe_unused)
 	return NULL;
 }
 
-static int pmu_max_precise(struct perf_pmu *pmu)
+static int pmu_max_precise(int dirfd, struct perf_pmu *pmu)
 {
 	int max_precise = -1;
 
-	perf_pmu__scan_file(pmu, "caps/max_precise", "%d", &max_precise);
+	perf_pmu__scan_file_at(pmu, dirfd, "caps/max_precise", "%d", &max_precise);
 	return max_precise;
 }
 
@@ -895,14 +900,14 @@ static struct perf_pmu *pmu_lookup(int dirfd, const char *lookup_name)
 	if (!pmu)
 		return NULL;
 
-	pmu->cpus = pmu_cpumask(name);
+	pmu->cpus = pmu_cpumask(dirfd, name);
 	pmu->name = strdup(name);
 
 	if (!pmu->name)
 		goto err;
 
 	/* Read type, and ensure that type value is successfully assigned (return 1) */
-	if (perf_pmu__scan_file(pmu, "type", "%u", &type) != 1)
+	if (perf_pmu__scan_file_at(pmu, dirfd, "type", "%u", &type) != 1)
 		goto err;
 
 	alias_name = pmu_find_alias_name(name);
@@ -913,10 +918,10 @@ static struct perf_pmu *pmu_lookup(int dirfd, const char *lookup_name)
 	}
 
 	pmu->type = type;
-	pmu->is_uncore = pmu_is_uncore(name);
+	pmu->is_uncore = pmu_is_uncore(dirfd, name);
 	if (pmu->is_uncore)
 		pmu->id = pmu_id(name);
-	pmu->max_precise = pmu_max_precise(pmu);
+	pmu->max_precise = pmu_max_precise(dirfd, pmu);
 	pmu_add_cpu_aliases(&aliases, pmu);
 	pmu_add_sys_aliases(&aliases, pmu);
 
@@ -1730,6 +1735,17 @@ FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name)
 	return fopen(path, "r");
 }
 
+FILE *perf_pmu__open_file_at(struct perf_pmu *pmu, int dirfd, const char *name)
+{
+	int fd;
+
+	fd = perf_pmu__pathname_fd(dirfd, pmu->name, name, O_RDONLY);
+	if (fd < 0)
+		return NULL;
+
+	return fdopen(fd, "r");
+}
+
 int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt,
 			...)
 {
@@ -1747,6 +1763,23 @@ int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt,
 	return ret;
 }
 
+int perf_pmu__scan_file_at(struct perf_pmu *pmu, int dirfd, const char *name,
+			   const char *fmt, ...)
+{
+	va_list args;
+	FILE *file;
+	int ret = EOF;
+
+	va_start(args, fmt);
+	file = perf_pmu__open_file_at(pmu, dirfd, name);
+	if (file) {
+		ret = vfscanf(file, fmt, args);
+		fclose(file);
+	}
+	va_end(args);
+	return ret;
+}
+
 bool perf_pmu__file_exists(struct perf_pmu *pmu, const char *name)
 {
 	char path[PATH_MAX];
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 751c7016e7b6..32c3a75bca0e 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -220,7 +220,12 @@ bool is_pmu_core(const char *name);
 void print_pmu_events(const struct print_callbacks *print_cb, void *print_state);
 bool pmu_have_event(const char *pname, const char *name);
 
+FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name);
+FILE *perf_pmu__open_file_at(struct perf_pmu *pmu, int dirfd, const char *name);
+
 int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt, ...) __scanf(3, 4);
+int perf_pmu__scan_file_at(struct perf_pmu *pmu, int dirfd, const char *name,
+			   const char *fmt, ...) __scanf(4, 5);
 
 bool perf_pmu__file_exists(struct perf_pmu *pmu, const char *name);
 
@@ -259,7 +264,6 @@ 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

[PATCH 9/9] perf intel-pt: Use perf_pmu__scan_file_at() if possible

[Namhyung Kim]

Intel-PT calls perf_pmu__scan_file() a lot, let's use relative address
when it accesses multiple files at one place.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/arch/x86/util/intel-pt.c | 52 ++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
index 1e39a034cee9..2cff11de9d8a 100644
--- a/tools/perf/arch/x86/util/intel-pt.c
+++ b/tools/perf/arch/x86/util/intel-pt.c
@@ -194,16 +194,19 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
 	int pos = 0;
 	u64 config;
 	char c;
+	int dirfd;
+
+	dirfd = perf_pmu__event_source_devices_fd();
 
 	pos += scnprintf(buf + pos, sizeof(buf) - pos, "tsc");
 
-	if (perf_pmu__scan_file(intel_pt_pmu, "caps/mtc", "%d",
-				&mtc) != 1)
+	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "caps/mtc", "%d",
+				   &mtc) != 1)
 		mtc = 1;
 
 	if (mtc) {
-		if (perf_pmu__scan_file(intel_pt_pmu, "caps/mtc_periods", "%x",
-					&mtc_periods) != 1)
+		if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "caps/mtc_periods", "%x",
+					   &mtc_periods) != 1)
 			mtc_periods = 0;
 		if (mtc_periods) {
 			mtc_period = intel_pt_pick_bit(mtc_periods, 3);
@@ -212,13 +215,13 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
 		}
 	}
 
-	if (perf_pmu__scan_file(intel_pt_pmu, "caps/psb_cyc", "%d",
-				&psb_cyc) != 1)
+	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "caps/psb_cyc", "%d",
+				   &psb_cyc) != 1)
 		psb_cyc = 1;
 
 	if (psb_cyc && mtc_periods) {
-		if (perf_pmu__scan_file(intel_pt_pmu, "caps/psb_periods", "%x",
-					&psb_periods) != 1)
+		if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "caps/psb_periods", "%x",
+					   &psb_periods) != 1)
 			psb_periods = 0;
 		if (psb_periods) {
 			psb_period = intel_pt_pick_bit(psb_periods, 3);
@@ -227,8 +230,8 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
 		}
 	}
 
-	if (perf_pmu__scan_file(intel_pt_pmu, "format/pt", "%c", &c) == 1 &&
-	    perf_pmu__scan_file(intel_pt_pmu, "format/branch", "%c", &c) == 1)
+	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "format/pt", "%c", &c) == 1 &&
+	    perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "format/branch", "%c", &c) == 1)
 		pos += scnprintf(buf + pos, sizeof(buf) - pos, ",pt,branch");
 
 	pr_debug2("%s default config: %s\n", intel_pt_pmu->name, buf);
@@ -236,6 +239,7 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
 	intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format, buf,
 			     &config);
 
+	close(dirfd);
 	return config;
 }
 
@@ -488,7 +492,7 @@ static void intel_pt_valid_str(char *str, size_t len, u64 valid)
 	}
 }
 
-static int intel_pt_val_config_term(struct perf_pmu *intel_pt_pmu,
+static int intel_pt_val_config_term(struct perf_pmu *intel_pt_pmu, int dirfd,
 				    const char *caps, const char *name,
 				    const char *supported, u64 config)
 {
@@ -498,11 +502,11 @@ static int intel_pt_val_config_term(struct perf_pmu *intel_pt_pmu,
 	u64 bits;
 	int ok;
 
-	if (perf_pmu__scan_file(intel_pt_pmu, caps, "%llx", &valid) != 1)
+	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, caps, "%llx", &valid) != 1)
 		valid = 0;
 
 	if (supported &&
-	    perf_pmu__scan_file(intel_pt_pmu, supported, "%d", &ok) == 1 && !ok)
+	    perf_pmu__scan_file_at(intel_pt_pmu, dirfd, supported, "%d", &ok) == 1 && !ok)
 		valid = 0;
 
 	valid |= 1;
@@ -531,37 +535,45 @@ static int intel_pt_val_config_term(struct perf_pmu *intel_pt_pmu,
 static int intel_pt_validate_config(struct perf_pmu *intel_pt_pmu,
 				    struct evsel *evsel)
 {
-	int err;
+	int err, dirfd;
 	char c;
 
 	if (!evsel)
 		return 0;
 
+	dirfd = perf_pmu__event_source_devices_fd();
+	if (dirfd < 0)
+		return dirfd;
+
 	/*
 	 * If supported, force pass-through config term (pt=1) even if user
 	 * sets pt=0, which avoids senseless kernel errors.
 	 */
-	if (perf_pmu__scan_file(intel_pt_pmu, "format/pt", "%c", &c) == 1 &&
+	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "format/pt", "%c", &c) == 1 &&
 	    !(evsel->core.attr.config & 1)) {
 		pr_warning("pt=0 doesn't make sense, forcing pt=1\n");
 		evsel->core.attr.config |= 1;
 	}
 
-	err = intel_pt_val_config_term(intel_pt_pmu, "caps/cycle_thresholds",
+	err = intel_pt_val_config_term(intel_pt_pmu, dirfd, "caps/cycle_thresholds",
 				       "cyc_thresh", "caps/psb_cyc",
 				       evsel->core.attr.config);
 	if (err)
-		return err;
+		goto out;
 
-	err = intel_pt_val_config_term(intel_pt_pmu, "caps/mtc_periods",
+	err = intel_pt_val_config_term(intel_pt_pmu, dirfd, "caps/mtc_periods",
 				       "mtc_period", "caps/mtc",
 				       evsel->core.attr.config);
 	if (err)
-		return err;
+		goto out;
 
-	return intel_pt_val_config_term(intel_pt_pmu, "caps/psb_periods",
+	err = intel_pt_val_config_term(intel_pt_pmu, dirfd, "caps/psb_periods",
 					"psb_period", "caps/psb_cyc",
 					evsel->core.attr.config);
+
+out:
+	close(dirfd);
+	return err;
 }
 
 static void intel_pt_config_sample_mode(struct perf_pmu *intel_pt_pmu,
-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH 9/9] perf intel-pt: Use perf_pmu__scan_file_at() if possible

[Adrian Hunter]

On 31/03/23 23:29, Namhyung Kim wrote:
> Intel-PT calls perf_pmu__scan_file() a lot, let's use relative address
> when it accesses multiple files at one place.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/arch/x86/util/intel-pt.c | 52 ++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
> index 1e39a034cee9..2cff11de9d8a 100644
> --- a/tools/perf/arch/x86/util/intel-pt.c
> +++ b/tools/perf/arch/x86/util/intel-pt.c
> @@ -194,16 +194,19 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
>  	int pos = 0;
>  	u64 config;
>  	char c;
> +	int dirfd;
> +
> +	dirfd = perf_pmu__event_source_devices_fd();
>  
>  	pos += scnprintf(buf + pos, sizeof(buf) - pos, "tsc");
>  
> -	if (perf_pmu__scan_file(intel_pt_pmu, "caps/mtc", "%d",
> -				&mtc) != 1)
> +	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "caps/mtc", "%d",
> +				   &mtc) != 1)
>  		mtc = 1;
>  
>  	if (mtc) {
> -		if (perf_pmu__scan_file(intel_pt_pmu, "caps/mtc_periods", "%x",
> -					&mtc_periods) != 1)
> +		if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "caps/mtc_periods", "%x",
> +					   &mtc_periods) != 1)
>  			mtc_periods = 0;
>  		if (mtc_periods) {
>  			mtc_period = intel_pt_pick_bit(mtc_periods, 3);
> @@ -212,13 +215,13 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
>  		}
>  	}
>  
> -	if (perf_pmu__scan_file(intel_pt_pmu, "caps/psb_cyc", "%d",
> -				&psb_cyc) != 1)
> +	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "caps/psb_cyc", "%d",
> +				   &psb_cyc) != 1)
>  		psb_cyc = 1;
>  
>  	if (psb_cyc && mtc_periods) {
> -		if (perf_pmu__scan_file(intel_pt_pmu, "caps/psb_periods", "%x",
> -					&psb_periods) != 1)
> +		if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "caps/psb_periods", "%x",
> +					   &psb_periods) != 1)
>  			psb_periods = 0;
>  		if (psb_periods) {
>  			psb_period = intel_pt_pick_bit(psb_periods, 3);
> @@ -227,8 +230,8 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
>  		}
>  	}
>  
> -	if (perf_pmu__scan_file(intel_pt_pmu, "format/pt", "%c", &c) == 1 &&
> -	    perf_pmu__scan_file(intel_pt_pmu, "format/branch", "%c", &c) == 1)
> +	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "format/pt", "%c", &c) == 1 &&
> +	    perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "format/branch", "%c", &c) == 1)
>  		pos += scnprintf(buf + pos, sizeof(buf) - pos, ",pt,branch");
>  
>  	pr_debug2("%s default config: %s\n", intel_pt_pmu->name, buf);
> @@ -236,6 +239,7 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
>  	intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format, buf,
>  			     &config);
>  
> +	close(dirfd);
>  	return config;
>  }
>  
> @@ -488,7 +492,7 @@ static void intel_pt_valid_str(char *str, size_t len, u64 valid)
>  	}
>  }
>  
> -static int intel_pt_val_config_term(struct perf_pmu *intel_pt_pmu,
> +static int intel_pt_val_config_term(struct perf_pmu *intel_pt_pmu, int dirfd,
>  				    const char *caps, const char *name,
>  				    const char *supported, u64 config)
>  {
> @@ -498,11 +502,11 @@ static int intel_pt_val_config_term(struct perf_pmu *intel_pt_pmu,
>  	u64 bits;
>  	int ok;
>  
> -	if (perf_pmu__scan_file(intel_pt_pmu, caps, "%llx", &valid) != 1)
> +	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, caps, "%llx", &valid) != 1)
>  		valid = 0;
>  
>  	if (supported &&
> -	    perf_pmu__scan_file(intel_pt_pmu, supported, "%d", &ok) == 1 && !ok)
> +	    perf_pmu__scan_file_at(intel_pt_pmu, dirfd, supported, "%d", &ok) == 1 && !ok)
>  		valid = 0;
>  
>  	valid |= 1;
> @@ -531,37 +535,45 @@ static int intel_pt_val_config_term(struct perf_pmu *intel_pt_pmu,
>  static int intel_pt_validate_config(struct perf_pmu *intel_pt_pmu,
>  				    struct evsel *evsel)
>  {
> -	int err;
> +	int err, dirfd;
>  	char c;
>  
>  	if (!evsel)
>  		return 0;
>  
> +	dirfd = perf_pmu__event_source_devices_fd();
> +	if (dirfd < 0)
> +		return dirfd;
> +
>  	/*
>  	 * If supported, force pass-through config term (pt=1) even if user
>  	 * sets pt=0, which avoids senseless kernel errors.
>  	 */
> -	if (perf_pmu__scan_file(intel_pt_pmu, "format/pt", "%c", &c) == 1 &&
> +	if (perf_pmu__scan_file_at(intel_pt_pmu, dirfd, "format/pt", "%c", &c) == 1 &&
>  	    !(evsel->core.attr.config & 1)) {
>  		pr_warning("pt=0 doesn't make sense, forcing pt=1\n");
>  		evsel->core.attr.config |= 1;
>  	}
>  
> -	err = intel_pt_val_config_term(intel_pt_pmu, "caps/cycle_thresholds",
> +	err = intel_pt_val_config_term(intel_pt_pmu, dirfd, "caps/cycle_thresholds",
>  				       "cyc_thresh", "caps/psb_cyc",
>  				       evsel->core.attr.config);
>  	if (err)
> -		return err;
> +		goto out;
>  
> -	err = intel_pt_val_config_term(intel_pt_pmu, "caps/mtc_periods",
> +	err = intel_pt_val_config_term(intel_pt_pmu, dirfd, "caps/mtc_periods",
>  				       "mtc_period", "caps/mtc",
>  				       evsel->core.attr.config);
>  	if (err)
> -		return err;
> +		goto out;
>  
> -	return intel_pt_val_config_term(intel_pt_pmu, "caps/psb_periods",
> +	err = intel_pt_val_config_term(intel_pt_pmu, dirfd, "caps/psb_periods",
>  					"psb_period", "caps/psb_cyc",
>  					evsel->core.attr.config);
> +
> +out:
> +	close(dirfd);
> +	return err;
>  }
>  
>  static void intel_pt_config_sample_mode(struct perf_pmu *intel_pt_pmu,

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

[Ian Rogers]

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
>

Re: [PATCHSET 0/9] perf tools: Update pmu scan using openat() (v1)

[Ian Rogers]

On Fri, Mar 31, 2023 at 1:29 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> This patchset changes PMU info scanning on sysfs using openat()
> basically.  I got reports of occasional contention on the
> opening files in sysfs.  While the root cause was a separate
> issue, I discovered some inefficiencies in the perf code.
>
> To scan PMUs, it roughly does something 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)
> in the kernel unnecessarily.  We can use openat(2) to open the file in
> the given directory.
>
> 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);
>     ...
>   }
>
> I added a benchmark for pmu-scan and it showed a slight speedup
> in the normal case too.
>
>   $ ./perf.old bench internals pmu-scan
>   # Running 'internals/pmu-scan' benchmark:
>   Computing performance of sysfs PMU event scan for 100 times
>     Average PMU scanning took: 6670.970 usec (+- 13.022 usec)
>
>   $ ./perf.new bench internals pmu-scan
>   # Running 'internals/pmu-scan' benchmark:
>   Computing performance of sysfs PMU event scan for 100 times
>     Average PMU scanning took: 6296.980 usec (+- 14.891 usec)
>
> The 5~6% of improvement might be small but it may have bigger impact
> when the system is contended.
>
> You can get the code from 'perf/pmu-scan-v1' branch in
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>
> Thanks,
> Namhyung

Some nice stack size savings too. The reentrant fix was pre-existing. Series:
Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Namhyung Kim (9):
>   perf list: Use relative path for tracepoint scan
>   perf tools: Fix a asan issue in parse_events_multi_pmu_add()
>   perf pmu: Add perf_pmu__destroy() function
>   perf bench: Add pmu-scan benchmark
>   perf pmu: Use relative path for sysfs scan
>   perf pmu: Use relative path in perf_pmu__caps_parse()
>   perf pmu: Use relative path in setup_pmu_alias_list()
>   perf pmu: Add perf_pmu__{open,scan}_file_at()
>   perf intel-pt: Use perf_pmu__scan_file_at() if possible
>
>  tools/perf/arch/x86/util/intel-pt.c |  52 ++++--
>  tools/perf/arch/x86/util/pmu.c      |  13 +-
>  tools/perf/bench/Build              |   1 +
>  tools/perf/bench/bench.h            |   1 +
>  tools/perf/bench/pmu-scan.c         | 184 ++++++++++++++++++
>  tools/perf/builtin-bench.c          |   1 +
>  tools/perf/tests/pmu.c              |   9 +-
>  tools/perf/util/parse-events.c      |   2 +-
>  tools/perf/util/pmu.c               | 278 ++++++++++++++++++++--------
>  tools/perf/util/pmu.h               |  12 +-
>  tools/perf/util/print-events.c      |  26 ++-
>  11 files changed, 466 insertions(+), 113 deletions(-)
>  create mode 100644 tools/perf/bench/pmu-scan.c
>
>
> base-commit: 417c6adfb155f906f0441cc1034827f6e2b3c372
> --
> 2.40.0.348.gf938b09366-goog
>

Re: [PATCHSET 0/9] perf tools: Update pmu scan using openat() (v1)

[Arnaldo Carvalho de Melo]

Em Mon, Apr 03, 2023 at 10:28:28AM -0700, Ian Rogers escreveu:
> On Fri, Mar 31, 2023 at 1:29 PM Namhyung Kim <namhyung@kernel.org> wrote:
> 
> Some nice stack size savings too. The reentrant fix was pre-existing. Series:
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied.

- Arnaldo

Re: [PATCH 1/9] perf list: Use relative path for tracepoint scan

[Arnaldo Carvalho de Melo]

Em Fri, Mar 31, 2023 at 01:29:41PM -0700, Namhyung Kim escreveu:
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/print-events.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)

Add to add this to fix on Alma Linux 8, and scandirat isn't being found
on musl libc (Alpine Linux), probably we'll need some scaffolding...

- Arnaldo


diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
index 26a7e017c9284c01..28aa0b9300253d0a 100644
--- a/tools/perf/util/print-events.c
+++ b/tools/perf/util/print-events.c
@@ -6,6 +6,7 @@
 #include <string.h>
 #include <fcntl.h>
 #include <sys/param.h>
+#include <unistd.h>
 
 #include <api/fs/tracing_path.h>
 #include <linux/stddef.h>
 
> diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> index 62e9ea7dcf40..26a7e017c928 100644
> --- a/tools/perf/util/print-events.c
> +++ b/tools/perf/util/print-events.c
> @@ -4,6 +4,7 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <fcntl.h>
>  #include <sys/param.h>
>  
>  #include <api/fs/tracing_path.h>
> @@ -59,12 +60,20 @@ static const struct event_symbol event_symbols_tool[PERF_TOOL_MAX] = {
>  void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state)
>  {
>  	struct dirent **sys_namelist = NULL;
> +	char *events_path = get_tracing_file("events");
>  	int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
> +	int events_fd = open(events_path, O_PATH);
> +
> +	put_tracing_file(events_path);
> +	if (events_fd < 0) {
> +		printf("Error: failed to open tracing events directory\n");
> +		return;
> +	}
>  
>  	for (int i = 0; i < sys_items; i++) {
>  		struct dirent *sys_dirent = sys_namelist[i];
>  		struct dirent **evt_namelist = NULL;
> -		char *dir_path;
> +		int dir_fd;
>  		int evt_items;
>  
>  		if (sys_dirent->d_type != DT_DIR ||
> @@ -72,22 +81,26 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
>  		    !strcmp(sys_dirent->d_name, ".."))
>  			continue;
>  
> -		dir_path = get_events_file(sys_dirent->d_name);
> -		if (!dir_path)
> +		dir_fd = openat(events_fd, sys_dirent->d_name, O_PATH);
> +		if (dir_fd < 0)
>  			continue;
>  
> -		evt_items = scandir(dir_path, &evt_namelist, NULL, alphasort);
> +		evt_items = scandirat(events_fd, sys_dirent->d_name, &evt_namelist, NULL, alphasort);
>  		for (int j = 0; j < evt_items; j++) {
>  			struct dirent *evt_dirent = evt_namelist[j];
>  			char evt_path[MAXPATHLEN];
> +			int evt_fd;
>  
>  			if (evt_dirent->d_type != DT_DIR ||
>  			    !strcmp(evt_dirent->d_name, ".") ||
>  			    !strcmp(evt_dirent->d_name, ".."))
>  				continue;
>  
> -			if (tp_event_has_id(dir_path, evt_dirent) != 0)
> +			snprintf(evt_path, sizeof(evt_path), "%s/id", evt_dirent->d_name);
> +			evt_fd = openat(dir_fd, evt_path, O_RDONLY);
> +			if (evt_fd < 0)
>  				continue;
> +			close(evt_fd);
>  
>  			snprintf(evt_path, MAXPATHLEN, "%s:%s",
>  				 sys_dirent->d_name, evt_dirent->d_name);
> @@ -103,10 +116,11 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
>  					/*long_desc=*/NULL,
>  					/*encoding_desc=*/NULL);
>  		}
> -		free(dir_path);
> +		close(dir_fd);
>  		free(evt_namelist);
>  	}
>  	free(sys_namelist);
> +	close(events_fd);
>  }
>  
>  void print_sdt_events(const struct print_callbacks *print_cb, void *print_state)
> -- 
> 2.40.0.348.gf938b09366-goog
> 

-- 

- Arnaldo

Re: [PATCH 1/9] perf list: Use relative path for tracepoint scan

[Arnaldo Carvalho de Melo]

Em Mon, Apr 03, 2023 at 06:59:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Mar 31, 2023 at 01:29:41PM -0700, Namhyung Kim escreveu:
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/print-events.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> Add to add this to fix on Alma Linux 8, and scandirat isn't being found
> on musl libc (Alpine Linux), probably we'll need some scaffolding...

Some discussion about this for some other project:

https://gitlab.com/apparmor/apparmor/-/merge_requests/107
 
> - Arnaldo
> 
> 
> diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> index 26a7e017c9284c01..28aa0b9300253d0a 100644
> --- a/tools/perf/util/print-events.c
> +++ b/tools/perf/util/print-events.c
> @@ -6,6 +6,7 @@
>  #include <string.h>
>  #include <fcntl.h>
>  #include <sys/param.h>
> +#include <unistd.h>
>  
>  #include <api/fs/tracing_path.h>
>  #include <linux/stddef.h>
>  
> > diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> > index 62e9ea7dcf40..26a7e017c928 100644
> > --- a/tools/perf/util/print-events.c
> > +++ b/tools/perf/util/print-events.c
> > @@ -4,6 +4,7 @@
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > +#include <fcntl.h>
> >  #include <sys/param.h>
> >  
> >  #include <api/fs/tracing_path.h>
> > @@ -59,12 +60,20 @@ static const struct event_symbol event_symbols_tool[PERF_TOOL_MAX] = {
> >  void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state)
> >  {
> >  	struct dirent **sys_namelist = NULL;
> > +	char *events_path = get_tracing_file("events");
> >  	int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
> > +	int events_fd = open(events_path, O_PATH);
> > +
> > +	put_tracing_file(events_path);
> > +	if (events_fd < 0) {
> > +		printf("Error: failed to open tracing events directory\n");
> > +		return;
> > +	}
> >  
> >  	for (int i = 0; i < sys_items; i++) {
> >  		struct dirent *sys_dirent = sys_namelist[i];
> >  		struct dirent **evt_namelist = NULL;
> > -		char *dir_path;
> > +		int dir_fd;
> >  		int evt_items;
> >  
> >  		if (sys_dirent->d_type != DT_DIR ||
> > @@ -72,22 +81,26 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
> >  		    !strcmp(sys_dirent->d_name, ".."))
> >  			continue;
> >  
> > -		dir_path = get_events_file(sys_dirent->d_name);
> > -		if (!dir_path)
> > +		dir_fd = openat(events_fd, sys_dirent->d_name, O_PATH);
> > +		if (dir_fd < 0)
> >  			continue;
> >  
> > -		evt_items = scandir(dir_path, &evt_namelist, NULL, alphasort);
> > +		evt_items = scandirat(events_fd, sys_dirent->d_name, &evt_namelist, NULL, alphasort);
> >  		for (int j = 0; j < evt_items; j++) {
> >  			struct dirent *evt_dirent = evt_namelist[j];
> >  			char evt_path[MAXPATHLEN];
> > +			int evt_fd;
> >  
> >  			if (evt_dirent->d_type != DT_DIR ||
> >  			    !strcmp(evt_dirent->d_name, ".") ||
> >  			    !strcmp(evt_dirent->d_name, ".."))
> >  				continue;
> >  
> > -			if (tp_event_has_id(dir_path, evt_dirent) != 0)
> > +			snprintf(evt_path, sizeof(evt_path), "%s/id", evt_dirent->d_name);
> > +			evt_fd = openat(dir_fd, evt_path, O_RDONLY);
> > +			if (evt_fd < 0)
> >  				continue;
> > +			close(evt_fd);
> >  
> >  			snprintf(evt_path, MAXPATHLEN, "%s:%s",
> >  				 sys_dirent->d_name, evt_dirent->d_name);
> > @@ -103,10 +116,11 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
> >  					/*long_desc=*/NULL,
> >  					/*encoding_desc=*/NULL);
> >  		}
> > -		free(dir_path);
> > +		close(dir_fd);
> >  		free(evt_namelist);
> >  	}
> >  	free(sys_namelist);
> > +	close(events_fd);
> >  }
> >  
> >  void print_sdt_events(const struct print_callbacks *print_cb, void *print_state)
> > -- 
> > 2.40.0.348.gf938b09366-goog
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

Re: [PATCH 1/9] perf list: Use relative path for tracepoint scan

[Arnaldo Carvalho de Melo]

Em Mon, Apr 03, 2023 at 06:59:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Mar 31, 2023 at 01:29:41PM -0700, Namhyung Kim escreveu:
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/print-events.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> Add to add this to fix on Alma Linux 8, and scandirat isn't being found
> on musl libc (Alpine Linux), probably we'll need some scaffolding...

About scandirat I'm adding the feature test below and this patch to the
patch that starts using scandirat():

diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
index 28aa0b9300253d0a..386b1ab0b60e1d93 100644
--- a/tools/perf/util/print-events.c
+++ b/tools/perf/util/print-events.c
@@ -58,11 +58,9 @@ static const struct event_symbol event_symbols_tool[PERF_TOOL_MAX] = {
 /*
  * Print the events from <debugfs_mount_point>/tracing/events
  */
-void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state)
+void print_tracepoint_events(const struct print_callbacks *print_cb __maybe_unused, void *print_state __maybe_unused)
 {
-	struct dirent **sys_namelist = NULL;
 	char *events_path = get_tracing_file("events");
-	int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
 	int events_fd = open(events_path, O_PATH);
 
 	put_tracing_file(events_path);
@@ -71,6 +69,11 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
 		return;
 	}
 
+#ifdef HAVE_SCANDIRAT_SUPPORT
+{
+	struct dirent **sys_namelist = NULL;
+	int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
+
 	for (int i = 0; i < sys_items; i++) {
 		struct dirent *sys_dirent = sys_namelist[i];
 		struct dirent **evt_namelist = NULL;
@@ -120,7 +123,13 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
 		close(dir_fd);
 		free(evt_namelist);
 	}
+
 	free(sys_namelist);
+}
+#else
+	printf("\nWARNING: Your libc doesn't have the scandir function, please ask its maintainers to implement it.\n"
+	       "         As a rough fallback, please do 'ls %s' to see the available tracepoint events.\n", events_path);
+#endif
 	close(events_fd);
 }
 

From b25f9de2f944a550b063322b7210f4392e622a5e Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Tue, 4 Apr 2023 11:05:57 -0300
Subject: [PATCH 1/1] tools build: Add a feature test for scandirat(), that is
 not implemented so far in musl and uclibc

We use it just when listing tracepoint events, and for root, so just
emit a warning about it to get users to ask the library maintainers to
implement it, as suggested in this systemd ticket:

 https://github.com/systemd/casync/issues/129

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/build/Makefile.feature   | 1 +
 tools/build/feature/Makefile   | 4 ++++
 tools/build/feature/test-all.c | 5 +++++
 tools/perf/Makefile.config     | 4 ++++
 4 files changed, 14 insertions(+)

diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 214622d7537cc56b..934e2777a2dbcd90 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -64,6 +64,7 @@ FEATURE_TESTS_BASIC :=                  \
         lzma                            \
         get_cpuid                       \
         bpf                             \
+        scandirat			\
         sched_getcpu			\
         sdt				\
         setns				\
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index 0a3b9281f8b08000..0f0aa9b7d7b5e43c 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -65,6 +65,7 @@ FILES=                                          \
          test-gettid.bin			\
          test-jvmti.bin				\
          test-jvmti-cmlr.bin			\
+         test-scandirat.bin			\
          test-sched_getcpu.bin			\
          test-setns.bin				\
          test-libopencsd.bin			\
@@ -129,6 +130,9 @@ $(OUTPUT)test-get_current_dir_name.bin:
 $(OUTPUT)test-glibc.bin:
 	$(BUILD)
 
+$(OUTPUT)test-scandirat.bin:
+	$(BUILD)
+
 $(OUTPUT)test-sched_getcpu.bin:
 	$(BUILD)
 
diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
index 957c02c7b163b579..6f4bf386a3b5c4b0 100644
--- a/tools/build/feature/test-all.c
+++ b/tools/build/feature/test-all.c
@@ -114,6 +114,10 @@
 # include "test-pthread-barrier.c"
 #undef main
 
+#define main main_test_scandirat
+# include "test-scandirat.c"
+#undef main
+
 #define main main_test_sched_getcpu
 # include "test-sched_getcpu.c"
 #undef main
@@ -206,6 +210,7 @@ int main(int argc, char *argv[])
 	main_test_get_cpuid();
 	main_test_bpf();
 	main_test_libcrypto();
+	main_test_scandirat();
 	main_test_sched_getcpu();
 	main_test_sdt();
 	main_test_setns();
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index dd203f0a2b7e64e1..16bea51f0bcd95ea 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -478,6 +478,10 @@ ifdef NO_DWARF
   NO_LIBDW_DWARF_UNWIND := 1
 endif
 
+ifeq ($(feature-scandirat), 1)
+  CFLAGS += -DHAVE_SCANDIRAT_SUPPORT
+endif
+
 ifeq ($(feature-sched_getcpu), 1)
   CFLAGS += -DHAVE_SCHED_GETCPU_SUPPORT
 endif
-- 
2.39.2

Re: [PATCH 1/9] perf list: Use relative path for tracepoint scan

[Namhyung Kim]

Hi Arnaldo,

On Tue, Apr 4, 2023 at 7:10 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Mon, Apr 03, 2023 at 06:59:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Mar 31, 2023 at 01:29:41PM -0700, Namhyung Kim escreveu:
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/util/print-events.c | 26 ++++++++++++++++++++------
> > >  1 file changed, 20 insertions(+), 6 deletions(-)
> >
> > Add to add this to fix on Alma Linux 8, and scandirat isn't being found
> > on musl libc (Alpine Linux), probably we'll need some scaffolding...
>
> About scandirat I'm adding the feature test below and this patch to the
> patch that starts using scandirat():

Thanks for doing this!

>
> diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> index 28aa0b9300253d0a..386b1ab0b60e1d93 100644
> --- a/tools/perf/util/print-events.c
> +++ b/tools/perf/util/print-events.c
> @@ -58,11 +58,9 @@ static const struct event_symbol event_symbols_tool[PERF_TOOL_MAX] = {
>  /*
>   * Print the events from <debugfs_mount_point>/tracing/events
>   */
> -void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state)
> +void print_tracepoint_events(const struct print_callbacks *print_cb __maybe_unused, void *print_state __maybe_unused)
>  {
> -       struct dirent **sys_namelist = NULL;
>         char *events_path = get_tracing_file("events");
> -       int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
>         int events_fd = open(events_path, O_PATH);
>
>         put_tracing_file(events_path);
> @@ -71,6 +69,11 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
>                 return;
>         }
>
> +#ifdef HAVE_SCANDIRAT_SUPPORT
> +{
> +       struct dirent **sys_namelist = NULL;
> +       int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
> +
>         for (int i = 0; i < sys_items; i++) {
>                 struct dirent *sys_dirent = sys_namelist[i];
>                 struct dirent **evt_namelist = NULL;
> @@ -120,7 +123,13 @@ void print_tracepoint_events(const struct print_callbacks *print_cb, void *print
>                 close(dir_fd);
>                 free(evt_namelist);
>         }
> +
>         free(sys_namelist);
> +}
> +#else
> +       printf("\nWARNING: Your libc doesn't have the scandir function, please ask its maintainers to implement it.\n"
> +              "         As a rough fallback, please do 'ls %s' to see the available tracepoint events.\n", events_path);
> +#endif
>         close(events_fd);
>  }
>
>
> From b25f9de2f944a550b063322b7210f4392e622a5e Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Tue, 4 Apr 2023 11:05:57 -0300
> Subject: [PATCH 1/1] tools build: Add a feature test for scandirat(), that is
>  not implemented so far in musl and uclibc
>
> We use it just when listing tracepoint events, and for root, so just
> emit a warning about it to get users to ask the library maintainers to
> implement it, as suggested in this systemd ticket:
>
>  https://github.com/systemd/casync/issues/129
>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/build/Makefile.feature   | 1 +
>  tools/build/feature/Makefile   | 4 ++++
>  tools/build/feature/test-all.c | 5 +++++
>  tools/perf/Makefile.config     | 4 ++++

Maybe you forgot to add the tools/build/feature/test-scandirat.c :)

Thanks,
Namhyung


>  4 files changed, 14 insertions(+)
>
> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> index 214622d7537cc56b..934e2777a2dbcd90 100644
> --- a/tools/build/Makefile.feature
> +++ b/tools/build/Makefile.feature
> @@ -64,6 +64,7 @@ FEATURE_TESTS_BASIC :=                  \
>          lzma                            \
>          get_cpuid                       \
>          bpf                             \
> +        scandirat                      \
>          sched_getcpu                   \
>          sdt                            \
>          setns                          \
> diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
> index 0a3b9281f8b08000..0f0aa9b7d7b5e43c 100644
> --- a/tools/build/feature/Makefile
> +++ b/tools/build/feature/Makefile
> @@ -65,6 +65,7 @@ FILES=                                          \
>           test-gettid.bin                       \
>           test-jvmti.bin                                \
>           test-jvmti-cmlr.bin                   \
> +         test-scandirat.bin                    \
>           test-sched_getcpu.bin                 \
>           test-setns.bin                                \
>           test-libopencsd.bin                   \
> @@ -129,6 +130,9 @@ $(OUTPUT)test-get_current_dir_name.bin:
>  $(OUTPUT)test-glibc.bin:
>         $(BUILD)
>
> +$(OUTPUT)test-scandirat.bin:
> +       $(BUILD)
> +
>  $(OUTPUT)test-sched_getcpu.bin:
>         $(BUILD)
>
> diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
> index 957c02c7b163b579..6f4bf386a3b5c4b0 100644
> --- a/tools/build/feature/test-all.c
> +++ b/tools/build/feature/test-all.c
> @@ -114,6 +114,10 @@
>  # include "test-pthread-barrier.c"
>  #undef main
>
> +#define main main_test_scandirat
> +# include "test-scandirat.c"
> +#undef main
> +
>  #define main main_test_sched_getcpu
>  # include "test-sched_getcpu.c"
>  #undef main
> @@ -206,6 +210,7 @@ int main(int argc, char *argv[])
>         main_test_get_cpuid();
>         main_test_bpf();
>         main_test_libcrypto();
> +       main_test_scandirat();
>         main_test_sched_getcpu();
>         main_test_sdt();
>         main_test_setns();
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index dd203f0a2b7e64e1..16bea51f0bcd95ea 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -478,6 +478,10 @@ ifdef NO_DWARF
>    NO_LIBDW_DWARF_UNWIND := 1
>  endif
>
> +ifeq ($(feature-scandirat), 1)
> +  CFLAGS += -DHAVE_SCANDIRAT_SUPPORT
> +endif
> +
>  ifeq ($(feature-sched_getcpu), 1)
>    CFLAGS += -DHAVE_SCHED_GETCPU_SUPPORT
>  endif
> --
> 2.39.2
>

Re: [PATCH 1/9] perf list: Use relative path for tracepoint scan

[Arnaldo Carvalho de Melo]

On April 4, 2023 7:08:31 PM GMT-03:00, Namhyung Kim <namhyung@kernel.org> wrote:

>> ---
>>  tools/build/Makefile.feature   | 1 +
>>  tools/build/feature/Makefile   | 4 ++++
>>  tools/build/feature/test-all.c | 5 +++++
>>  tools/perf/Makefile.config     | 4 ++++
>
>Maybe you forgot to add the tools/build/feature/test-scandirat.c :)


Yeah , I noticed that and fixed it:

https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/build/feature/test-scandirat.c?h=tmp.perf-tools-next

- Arnaldo