[PATCH v2 1/3] perf evsel: Improve the evsel__open_strerror for EBUSY

[PATCH v2 1/3] perf evsel: Improve the evsel__open_strerror for EBUSY

[Chun-Tse Shao]

From: Ian Rogers <irogers@google.com>

The existing EBUSY strerror message is:
```
The sys_perf_event_open() syscall returned with 16 (Device or resource busy) for event (intel_bts//).
"dmesg | grep -i perf" may provide additional information.
```
The dmesg won't be useful. What is more useful is knowing what
processes are potentially using the PMU, which some procfs scanning can
reveal. When parallel testing tests/shell/stat_all_pmu.sh this yields:
```
Testing intel_bts//
Error:
The PMU intel_bts counters are busy and in use by another process.
Possible processes:
2585882 perf list
2585902 perf list -j -o /tmp/__perf_test.list_output.json.KF9MY
2585904 perf list
2585911 perf record -e task-clock --filter period > 1 -o /dev/null --quiet true
2585912 perf list
2585915 perf list
2586042 /tmp/perf/perf record -asdg -e cpu-clock -o /tmp/perftool-testsuite_report.dIF/perf_report/perf.data -- sleep 2
2589078 perf record -g -e task-clock:u -o - perf test -w noploop
2589148 /tmp/perf/perf record --control=fifo:control,ack -e cpu-clock -m 1 sleep 10
2589379 perf --buildid-dir /tmp/perf.debug.Umx record --buildid-all -o /tmp/perf.data.YBm /tmp/perf.ex.MD5.ZQW
2589568 perf record -o /tmp/__perf_test.program.mtcZH/perf.data --branch-filter any,save_type,u -- perf test -w brstack
2589649 perf record --per-thread -o /tmp/__perf_test.perf.data.5d3dc perf test -w thloop
2589898 perf record -o /tmp/perf-test-script.BX2b27Dcnj/pp-perf.data --sample-cpu uname
```
Which gets a little closer to finding the issue.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 79 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index dbf9c8cee3c56..9a5b6a6f8d2e5 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3286,6 +3286,78 @@ static bool find_process(const char *name)
 	return ret ? false : true;
 }

+static int dump_perf_event_processes(char *msg, size_t size)
+{
+	DIR *proc_dir;
+	struct dirent *proc_entry;
+	int printed = 0;
+
+	proc_dir = opendir(procfs__mountpoint());
+	if (!proc_dir)
+		return 0;
+
+	/* Walk through the /proc directory. */
+	while ((proc_entry = readdir(proc_dir)) != NULL) {
+		char path[PATH_MAX];
+		DIR *fd_dir;
+		struct dirent *fd_entry;
+		int fd_dir_fd;
+
+		if ((proc_entry->d_type != DT_DIR) ||
+		     !strcmp(".", proc_entry->d_name) ||
+		     !strcmp("..", proc_entry->d_name))
+			continue;
+
+		scnprintf(path, sizeof(path), "%s/fd", proc_entry->d_name);
+		fd_dir_fd = openat(dirfd(proc_dir), path, O_DIRECTORY);
+		if (fd_dir_fd == -1)
+			continue;
+		fd_dir = fdopendir(fd_dir_fd);
+		if (!fd_dir) {
+			close(fd_dir_fd);
+			continue;
+		}
+		while ((fd_entry = readdir(fd_dir)) != NULL) {
+			ssize_t link_size;
+
+			if (fd_entry->d_type != DT_LNK)
+				continue;
+			link_size = readlinkat(fd_dir_fd, fd_entry->d_name, path, sizeof(path));
+			if (link_size < 0)
+				continue;
+			/* Take care as readlink doesn't null terminate the string. */
+			if (!strncmp(path, "anon_inode:[perf_event]", link_size)) {
+				int cmdline_fd;
+				ssize_t cmdline_size;
+
+				scnprintf(path, sizeof(path), "%s/cmdline", proc_entry->d_name);
+				cmdline_fd = openat(dirfd(proc_dir), path, O_RDONLY);
+				if (cmdline_fd == -1)
+					continue;
+				cmdline_size = read(cmdline_fd, path, sizeof(path) - 1);
+				close(cmdline_fd);
+				if (cmdline_size < 0)
+					continue;
+				path[cmdline_size] = '\0';
+				for (ssize_t i = 0; i < cmdline_size; i++) {
+					if (path[i] == '\0')
+						path[i] = ' ';
+				}
+
+				if (printed == 0)
+					printed += scnprintf(msg, size, "Possible processes:\n");
+
+				printed += scnprintf(msg + printed, size - printed,
+						"%s %s\n", proc_entry->d_name, path);
+				break;
+			}
+		}
+		closedir(fd_dir);
+	}
+	closedir(proc_dir);
+	return printed;
+}
+
 int __weak arch_evsel__open_strerror(struct evsel *evsel __maybe_unused,
 				     char *msg __maybe_unused,
 				     size_t size __maybe_unused)
@@ -3319,7 +3391,7 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
 			printed += scnprintf(msg, size,
 				"No permission to enable %s event.\n\n", evsel__name(evsel));

-		return scnprintf(msg + printed, size - printed,
+		return printed + scnprintf(msg + printed, size - printed,
 		 "Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open\n"
 		 "access to performance monitoring and observability operations for processes\n"
 		 "without CAP_PERFMON, CAP_SYS_PTRACE or CAP_SYS_ADMIN Linux capability.\n"
@@ -3382,6 +3454,11 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
 			return scnprintf(msg, size,
 	"The PMU counters are busy/taken by another profiler.\n"
 	"We found oprofile daemon running, please stop it and try again.");
+		printed += scnprintf(
+			msg, size,
+			"The PMU %s counters are busy and in use by another process.\n",
+			evsel->pmu ? evsel->pmu->name : "");
+		return printed + dump_perf_event_processes(msg + printed, size - printed);
 		break;
 	case EINVAL:
 		if (evsel->core.attr.sample_type & PERF_SAMPLE_CODE_PAGE_SIZE && perf_missing_features.code_page_size)
--
2.47.0.163.g1226f6d8fa-goog

[PATCH v2 2/3] perf: Reveal PMU type in fdinfo

[Chun-Tse Shao]

It gives useful info on knowing which PMUs are reserved by this process.
Also add extra attributes which would be useful.

```
Testing cycles
$ ./perf stat -e cycles &
$ cat /proc/`pidof perf`/fdinfo/3
pos:    0
flags:  02000002
mnt_id: 16
ino:    3081
perf_event-attr.type:   0
perf_event-attr.config: 0
perf_event-attr.config1:        0
perf_event-attr.config2:        0
perf_event-attr.config3:        0

Testing L1-dcache-load-misses//
$ ./perf stat -e L1-dcache-load-misses &
$ cat /proc/`pidof perf`/fdinfo/3
pos:    0
flags:  02000002
mnt_id: 16
ino:    1072
perf_event-attr.type:   3
perf_event-attr.config: 65536
perf_event-attr.config1:        0
perf_event-attr.config2:        0
perf_event-attr.config3:        0
```

Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 kernel/events/core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index cdd09769e6c56..c950b6fc92cda 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -55,6 +55,7 @@
 #include <linux/pgtable.h>
 #include <linux/buildid.h>
 #include <linux/task_work.h>
+#include <linux/seq_file.h>

 #include "internal.h"

@@ -6820,6 +6821,17 @@ static int perf_fasync(int fd, struct file *filp, int on)
 	return 0;
 }

+static void perf_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	struct perf_event *event = f->private_data;
+
+	seq_printf(m, "perf_event-attr.type:\t%u\n", event->orig_type);
+	seq_printf(m, "perf_event-attr.config:\t%llu\n", event->attr.config);
+	seq_printf(m, "perf_event-attr.config1:\t%llu\n", event->attr.config1);
+	seq_printf(m, "perf_event-attr.config2:\t%llu\n", event->attr.config2);
+	seq_printf(m, "perf_event-attr.config3:\t%llu\n", event->attr.config3);
+}
+
 static const struct file_operations perf_fops = {
 	.release		= perf_release,
 	.read			= perf_read,
@@ -6828,6 +6840,7 @@ static const struct file_operations perf_fops = {
 	.compat_ioctl		= perf_compat_ioctl,
 	.mmap			= perf_mmap,
 	.fasync			= perf_fasync,
+	.show_fdinfo		= perf_show_fdinfo,
 };

 /*
--
2.47.0.163.g1226f6d8fa-goog

[PATCH v2 3/3] perf evsel: Find process with busy PMUs for EBUSY

[Chun-Tse Shao]

It parses fdinfo with PMU type, comparing with the event which failed to
open, and report the processes causing EBUSY error.

```
Testing cycles and intel_pt//
$ ./perf stat -e cycles &
[1] 55569
$ ./perf stat -e intel_pt// &
[2] 55683
$ ./perf stat -e intel_pt//
Error:
The PMU intel_pt counters are busy and in use by another process.
Possible processes:
55683 ./perf stat -e intel_pt//
```
Only perf with intel_pt was reported.

Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 tools/perf/util/evsel.c | 79 +++++++++++++++++++++++++++++------------
 1 file changed, 57 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 9a5b6a6f8d2e5..dfcb801d8921a 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3286,7 +3286,8 @@ static bool find_process(const char *name)
 	return ret ? false : true;
 }

-static int dump_perf_event_processes(char *msg, size_t size)
+static int dump_perf_event_processes(const struct perf_event_attr *failed_attr,
+				     char *msg, size_t size)
 {
 	DIR *proc_dir;
 	struct dirent *proc_entry;
@@ -3327,29 +3328,61 @@ static int dump_perf_event_processes(char *msg, size_t size)
 				continue;
 			/* Take care as readlink doesn't null terminate the string. */
 			if (!strncmp(path, "anon_inode:[perf_event]", link_size)) {
-				int cmdline_fd;
-				ssize_t cmdline_size;
-
-				scnprintf(path, sizeof(path), "%s/cmdline", proc_entry->d_name);
-				cmdline_fd = openat(dirfd(proc_dir), path, O_RDONLY);
-				if (cmdline_fd == -1)
-					continue;
-				cmdline_size = read(cmdline_fd, path, sizeof(path) - 1);
-				close(cmdline_fd);
-				if (cmdline_size < 0)
+				int fdinfo_fd;
+				ssize_t fdinfo_size;
+				char *line;
+				u32 perf_event_type = INT_MAX;
+
+				/* Let's check the PMU type reserved by this process */
+				scnprintf(path, sizeof(path), "%s/fdinfo/%s",
+					  proc_entry->d_name, fd_entry->d_name);
+				fdinfo_fd = openat(dirfd(proc_dir), path, O_RDONLY);
+				fdinfo_size = read(fdinfo_fd, path, sizeof(path) - 1);
+				if (fdinfo_size < 0)
 					continue;
-				path[cmdline_size] = '\0';
-				for (ssize_t i = 0; i < cmdline_size; i++) {
-					if (path[i] == '\0')
-						path[i] = ' ';
+				path[fdinfo_size] = '\0';
+
+				line = strtok(path, "\n");
+				while (line != NULL) {
+					if (sscanf(line,
+						   "perf_event-attr.type:\t%u",
+						   &perf_event_type) == 1)
+						break;
+					line = strtok(NULL, "\n");
 				}

-				if (printed == 0)
-					printed += scnprintf(msg, size, "Possible processes:\n");
-
-				printed += scnprintf(msg + printed, size - printed,
-						"%s %s\n", proc_entry->d_name, path);
-				break;
+				/* Report the process which reserves the conflicted PMU. */
+				/* If fdinfo does not contain PMU type, report it too. */
+				if (perf_event_type == failed_attr->type ||
+				    perf_event_type == INT_MAX) {
+					int cmdline_fd;
+					ssize_t cmdline_size;
+
+					scnprintf(path, sizeof(path),
+						  "%s/cmdline",
+						  proc_entry->d_name);
+					cmdline_fd = openat(dirfd(proc_dir), path, O_RDONLY);
+					if (cmdline_fd == -1)
+						continue;
+					cmdline_size = read(cmdline_fd, path, sizeof(path) - 1);
+					close(cmdline_fd);
+					if (cmdline_size < 0)
+						continue;
+					path[cmdline_size] = '\0';
+					for (ssize_t i = 0; i < cmdline_size; i++) {
+						if (path[i] == '\0')
+							path[i] = ' ';
+					}
+
+					if (printed == 0)
+						printed += scnprintf(
+							msg, size,
+							"Possible processes:\n");
+
+					printed += scnprintf(msg + printed, size - printed,
+							"%s %s\n", proc_entry->d_name, path);
+					break;
+				}
 			}
 		}
 		closedir(fd_dir);
@@ -3458,7 +3491,9 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
 			msg, size,
 			"The PMU %s counters are busy and in use by another process.\n",
 			evsel->pmu ? evsel->pmu->name : "");
-		return printed + dump_perf_event_processes(msg + printed, size - printed);
+		return printed + dump_perf_event_processes(&evsel->core.attr,
+							   msg + printed,
+							   size - printed);
 		break;
 	case EINVAL:
 		if (evsel->core.attr.sample_type & PERF_SAMPLE_CODE_PAGE_SIZE && perf_missing_features.code_page_size)
--
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH v2 3/3] perf evsel: Find process with busy PMUs for EBUSY

[Chun-Tse Shao]

On Fri, Nov 1, 2024 at 2:18 PM Chun-Tse Shao <ctshao@google.com> wrote:
>
> It parses fdinfo with PMU type, comparing with the event which failed to
> open, and report the processes causing EBUSY error.
>
> ```
> Testing cycles and intel_pt//
> $ ./perf stat -e cycles &
> [1] 55569
> $ ./perf stat -e intel_pt// &
> [2] 55683
> $ ./perf stat -e intel_pt//
> Error:
> The PMU intel_pt counters are busy and in use by another process.
> Possible processes:
> 55683 ./perf stat -e intel_pt//
> ```
> Only perf with intel_pt was reported.
>
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> ---
>  tools/perf/util/evsel.c | 79 +++++++++++++++++++++++++++++------------
>  1 file changed, 57 insertions(+), 22 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 9a5b6a6f8d2e5..dfcb801d8921a 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3286,7 +3286,8 @@ static bool find_process(const char *name)
>         return ret ? false : true;
>  }
>
> -static int dump_perf_event_processes(char *msg, size_t size)
> +static int dump_perf_event_processes(const struct perf_event_attr *failed_attr,
> +                                    char *msg, size_t size)
>  {
>         DIR *proc_dir;
>         struct dirent *proc_entry;
> @@ -3327,29 +3328,61 @@ static int dump_perf_event_processes(char *msg, size_t size)
>                                 continue;
>                         /* Take care as readlink doesn't null terminate the string. */
>                         if (!strncmp(path, "anon_inode:[perf_event]", link_size)) {
> -                               int cmdline_fd;
> -                               ssize_t cmdline_size;
> -
> -                               scnprintf(path, sizeof(path), "%s/cmdline", proc_entry->d_name);
> -                               cmdline_fd = openat(dirfd(proc_dir), path, O_RDONLY);
> -                               if (cmdline_fd == -1)
> -                                       continue;
> -                               cmdline_size = read(cmdline_fd, path, sizeof(path) - 1);
> -                               close(cmdline_fd);
> -                               if (cmdline_size < 0)
> +                               int fdinfo_fd;
> +                               ssize_t fdinfo_size;
> +                               char *line;
> +                               u32 perf_event_type = INT_MAX;

This is a typo, it should be `UINT32_MAX`. I will fix this in v3,
along with other potential fixes after review.

> +
> +                               /* Let's check the PMU type reserved by this process */
> +                               scnprintf(path, sizeof(path), "%s/fdinfo/%s",
> +                                         proc_entry->d_name, fd_entry->d_name);
> +                               fdinfo_fd = openat(dirfd(proc_dir), path, O_RDONLY);
> +                               fdinfo_size = read(fdinfo_fd, path, sizeof(path) - 1);
> +                               if (fdinfo_size < 0)
>                                         continue;
> -                               path[cmdline_size] = '\0';
> -                               for (ssize_t i = 0; i < cmdline_size; i++) {
> -                                       if (path[i] == '\0')
> -                                               path[i] = ' ';
> +                               path[fdinfo_size] = '\0';
> +
> +                               line = strtok(path, "\n");
> +                               while (line != NULL) {
> +                                       if (sscanf(line,
> +                                                  "perf_event-attr.type:\t%u",
> +                                                  &perf_event_type) == 1)
> +                                               break;
> +                                       line = strtok(NULL, "\n");
>                                 }
>
> -                               if (printed == 0)
> -                                       printed += scnprintf(msg, size, "Possible processes:\n");
> -
> -                               printed += scnprintf(msg + printed, size - printed,
> -                                               "%s %s\n", proc_entry->d_name, path);
> -                               break;
> +                               /* Report the process which reserves the conflicted PMU. */
> +                               /* If fdinfo does not contain PMU type, report it too. */
> +                               if (perf_event_type == failed_attr->type ||
> +                                   perf_event_type == INT_MAX) {
> +                                       int cmdline_fd;
> +                                       ssize_t cmdline_size;
> +
> +                                       scnprintf(path, sizeof(path),
> +                                                 "%s/cmdline",
> +                                                 proc_entry->d_name);
> +                                       cmdline_fd = openat(dirfd(proc_dir), path, O_RDONLY);
> +                                       if (cmdline_fd == -1)
> +                                               continue;
> +                                       cmdline_size = read(cmdline_fd, path, sizeof(path) - 1);
> +                                       close(cmdline_fd);
> +                                       if (cmdline_size < 0)
> +                                               continue;
> +                                       path[cmdline_size] = '\0';
> +                                       for (ssize_t i = 0; i < cmdline_size; i++) {
> +                                               if (path[i] == '\0')
> +                                                       path[i] = ' ';
> +                                       }
> +
> +                                       if (printed == 0)
> +                                               printed += scnprintf(
> +                                                       msg, size,
> +                                                       "Possible processes:\n");
> +
> +                                       printed += scnprintf(msg + printed, size - printed,
> +                                                       "%s %s\n", proc_entry->d_name, path);
> +                                       break;
> +                               }
>                         }
>                 }
>                 closedir(fd_dir);
> @@ -3458,7 +3491,9 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
>                         msg, size,
>                         "The PMU %s counters are busy and in use by another process.\n",
>                         evsel->pmu ? evsel->pmu->name : "");
> -               return printed + dump_perf_event_processes(msg + printed, size - printed);
> +               return printed + dump_perf_event_processes(&evsel->core.attr,
> +                                                          msg + printed,
> +                                                          size - printed);
>                 break;
>         case EINVAL:
>                 if (evsel->core.attr.sample_type & PERF_SAMPLE_CODE_PAGE_SIZE && perf_missing_features.code_page_size)
> --
> 2.47.0.163.g1226f6d8fa-goog
>

Re: [PATCH v2 1/3] perf evsel: Improve the evsel__open_strerror for EBUSY

[Namhyung Kim]

Hello CT,

On Fri, Nov 01, 2024 at 09:17:55PM +0000, Chun-Tse Shao wrote:
> From: Ian Rogers <irogers@google.com>
> 
> The existing EBUSY strerror message is:
> ```
> The sys_perf_event_open() syscall returned with 16 (Device or resource busy) for event (intel_bts//).
> "dmesg | grep -i perf" may provide additional information.
> ```

Just a nitpick.  I'd like to avoid this github markdown style notation
of triple backticks as it doesn't clearly separate code blocks (IMHO)
nor protect anything like '#' sign in the beginning of a line.

I prefer indenting with 2 spaces before and after a blank line.


> The dmesg won't be useful. What is more useful is knowing what
> processes are potentially using the PMU, which some procfs scanning can
> reveal. When parallel testing tests/shell/stat_all_pmu.sh this yields:
> ```
> Testing intel_bts//
> Error:
> The PMU intel_bts counters are busy and in use by another process.
> Possible processes:
> 2585882 perf list
> 2585902 perf list -j -o /tmp/__perf_test.list_output.json.KF9MY
> 2585904 perf list
> 2585911 perf record -e task-clock --filter period > 1 -o /dev/null --quiet true
> 2585912 perf list
> 2585915 perf list
> 2586042 /tmp/perf/perf record -asdg -e cpu-clock -o /tmp/perftool-testsuite_report.dIF/perf_report/perf.data -- sleep 2
> 2589078 perf record -g -e task-clock:u -o - perf test -w noploop
> 2589148 /tmp/perf/perf record --control=fifo:control,ack -e cpu-clock -m 1 sleep 10
> 2589379 perf --buildid-dir /tmp/perf.debug.Umx record --buildid-all -o /tmp/perf.data.YBm /tmp/perf.ex.MD5.ZQW
> 2589568 perf record -o /tmp/__perf_test.program.mtcZH/perf.data --branch-filter any,save_type,u -- perf test -w brstack
> 2589649 perf record --per-thread -o /tmp/__perf_test.perf.data.5d3dc perf test -w thloop
> 2589898 perf record -o /tmp/perf-test-script.BX2b27Dcnj/pp-perf.data --sample-cpu uname
> ```
> Which gets a little closer to finding the issue.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 79 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index dbf9c8cee3c56..9a5b6a6f8d2e5 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3286,6 +3286,78 @@ static bool find_process(const char *name)
>  	return ret ? false : true;
>  }
> 
> +static int dump_perf_event_processes(char *msg, size_t size)
> +{
> +	DIR *proc_dir;
> +	struct dirent *proc_entry;
> +	int printed = 0;
> +
> +	proc_dir = opendir(procfs__mountpoint());
> +	if (!proc_dir)
> +		return 0;
> +
> +	/* Walk through the /proc directory. */
> +	while ((proc_entry = readdir(proc_dir)) != NULL) {
> +		char path[PATH_MAX];

Can we use a much smaller buffer as it expects PIDs only?


> +		DIR *fd_dir;
> +		struct dirent *fd_entry;
> +		int fd_dir_fd;
> +
> +		if ((proc_entry->d_type != DT_DIR) ||
> +		     !strcmp(".", proc_entry->d_name) ||
> +		     !strcmp("..", proc_entry->d_name))

Maybe something like this?

		if (proc_entry->d_type != DT_DIR ||
		    !isdigit(proc_entry->d_name[0]) ||
		    strlen(proc_entry->d_name) > sizeof(path) - 4)

Thanks,
Namhyung


> +			continue;
> +
> +		scnprintf(path, sizeof(path), "%s/fd", proc_entry->d_name);
> +		fd_dir_fd = openat(dirfd(proc_dir), path, O_DIRECTORY);
> +		if (fd_dir_fd == -1)
> +			continue;
> +		fd_dir = fdopendir(fd_dir_fd);
> +		if (!fd_dir) {
> +			close(fd_dir_fd);
> +			continue;
> +		}
> +		while ((fd_entry = readdir(fd_dir)) != NULL) {
> +			ssize_t link_size;
> +
> +			if (fd_entry->d_type != DT_LNK)
> +				continue;
> +			link_size = readlinkat(fd_dir_fd, fd_entry->d_name, path, sizeof(path));
> +			if (link_size < 0)
> +				continue;
> +			/* Take care as readlink doesn't null terminate the string. */
> +			if (!strncmp(path, "anon_inode:[perf_event]", link_size)) {
> +				int cmdline_fd;
> +				ssize_t cmdline_size;
> +
> +				scnprintf(path, sizeof(path), "%s/cmdline", proc_entry->d_name);
> +				cmdline_fd = openat(dirfd(proc_dir), path, O_RDONLY);
> +				if (cmdline_fd == -1)
> +					continue;
> +				cmdline_size = read(cmdline_fd, path, sizeof(path) - 1);
> +				close(cmdline_fd);
> +				if (cmdline_size < 0)
> +					continue;
> +				path[cmdline_size] = '\0';
> +				for (ssize_t i = 0; i < cmdline_size; i++) {
> +					if (path[i] == '\0')
> +						path[i] = ' ';
> +				}
> +
> +				if (printed == 0)
> +					printed += scnprintf(msg, size, "Possible processes:\n");
> +
> +				printed += scnprintf(msg + printed, size - printed,
> +						"%s %s\n", proc_entry->d_name, path);
> +				break;
> +			}
> +		}
> +		closedir(fd_dir);
> +	}
> +	closedir(proc_dir);
> +	return printed;
> +}
> +
>  int __weak arch_evsel__open_strerror(struct evsel *evsel __maybe_unused,
>  				     char *msg __maybe_unused,
>  				     size_t size __maybe_unused)
> @@ -3319,7 +3391,7 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
>  			printed += scnprintf(msg, size,
>  				"No permission to enable %s event.\n\n", evsel__name(evsel));
> 
> -		return scnprintf(msg + printed, size - printed,
> +		return printed + scnprintf(msg + printed, size - printed,
>  		 "Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open\n"
>  		 "access to performance monitoring and observability operations for processes\n"
>  		 "without CAP_PERFMON, CAP_SYS_PTRACE or CAP_SYS_ADMIN Linux capability.\n"
> @@ -3382,6 +3454,11 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
>  			return scnprintf(msg, size,
>  	"The PMU counters are busy/taken by another profiler.\n"
>  	"We found oprofile daemon running, please stop it and try again.");
> +		printed += scnprintf(
> +			msg, size,
> +			"The PMU %s counters are busy and in use by another process.\n",
> +			evsel->pmu ? evsel->pmu->name : "");
> +		return printed + dump_perf_event_processes(msg + printed, size - printed);
>  		break;
>  	case EINVAL:
>  		if (evsel->core.attr.sample_type & PERF_SAMPLE_CODE_PAGE_SIZE && perf_missing_features.code_page_size)
> --
> 2.47.0.163.g1226f6d8fa-goog
> 

Re: [PATCH v2 2/3] perf: Reveal PMU type in fdinfo

[Namhyung Kim]

On Fri, Nov 01, 2024 at 09:17:56PM +0000, Chun-Tse Shao wrote:
> It gives useful info on knowing which PMUs are reserved by this process.
> Also add extra attributes which would be useful.
> 
> ```
> Testing cycles
> $ ./perf stat -e cycles &
> $ cat /proc/`pidof perf`/fdinfo/3
> pos:    0
> flags:  02000002
> mnt_id: 16
> ino:    3081
> perf_event-attr.type:   0

Maybe 'perf_event_attr' would be appropriate as it's the name of the
struct.

> perf_event-attr.config: 0
> perf_event-attr.config1:        0
> perf_event-attr.config2:        0
> perf_event-attr.config3:        0

It's hard to pick which fields to show here but I'd say that those
config[123] are not used frequently at least for regular events.
Maybe just showing type and config is fine.

> 
> Testing L1-dcache-load-misses//
> $ ./perf stat -e L1-dcache-load-misses &
> $ cat /proc/`pidof perf`/fdinfo/3
> pos:    0
> flags:  02000002
> mnt_id: 16
> ino:    1072
> perf_event-attr.type:   3
> perf_event-attr.config: 65536
> perf_event-attr.config1:        0
> perf_event-attr.config2:        0
> perf_event-attr.config3:        0
> ```
> 
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> ---
>  kernel/events/core.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

FYI usually the kernel changes are applied to a different tree than the
tools.

> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index cdd09769e6c56..c950b6fc92cda 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -55,6 +55,7 @@
>  #include <linux/pgtable.h>
>  #include <linux/buildid.h>
>  #include <linux/task_work.h>
> +#include <linux/seq_file.h>
> 
>  #include "internal.h"
> 
> @@ -6820,6 +6821,17 @@ static int perf_fasync(int fd, struct file *filp, int on)
>  	return 0;
>  }
> 
> +static void perf_show_fdinfo(struct seq_file *m, struct file *f)
> +{
> +	struct perf_event *event = f->private_data;
> +
> +	seq_printf(m, "perf_event-attr.type:\t%u\n", event->orig_type);
> +	seq_printf(m, "perf_event-attr.config:\t%llu\n", event->attr.config);

I'm not sure if all archs are happy with treating it as %llu.

Thanks,
Namhyung


> +	seq_printf(m, "perf_event-attr.config1:\t%llu\n", event->attr.config1);
> +	seq_printf(m, "perf_event-attr.config2:\t%llu\n", event->attr.config2);
> +	seq_printf(m, "perf_event-attr.config3:\t%llu\n", event->attr.config3);
> +}
> +
>  static const struct file_operations perf_fops = {
>  	.release		= perf_release,
>  	.read			= perf_read,
> @@ -6828,6 +6840,7 @@ static const struct file_operations perf_fops = {
>  	.compat_ioctl		= perf_compat_ioctl,
>  	.mmap			= perf_mmap,
>  	.fasync			= perf_fasync,
> +	.show_fdinfo		= perf_show_fdinfo,
>  };
> 
>  /*
> --
> 2.47.0.163.g1226f6d8fa-goog
> 

Re: [PATCH v2 1/3] perf evsel: Improve the evsel__open_strerror for EBUSY

[Chun-Tse Shao]

Thanks for your review Namhyung! Here is the link to the v3 patch:
https://lore.kernel.org/all/20241106003007.2112584-1-ctshao@google.com/

On Mon, Nov 4, 2024 at 4:01 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello CT,
>
> On Fri, Nov 01, 2024 at 09:17:55PM +0000, Chun-Tse Shao wrote:
> > From: Ian Rogers <irogers@google.com>
> >
> > The existing EBUSY strerror message is:
> > ```
> > The sys_perf_event_open() syscall returned with 16 (Device or resource busy) for event (intel_bts//).
> > "dmesg | grep -i perf" may provide additional information.
> > ```
>
> Just a nitpick.  I'd like to avoid this github markdown style notation
> of triple backticks as it doesn't clearly separate code blocks (IMHO)
> nor protect anything like '#' sign in the beginning of a line.
>
> I prefer indenting with 2 spaces before and after a blank line.
>

Fixed in v3.

>
> > The dmesg won't be useful. What is more useful is knowing what
> > processes are potentially using the PMU, which some procfs scanning can
> > reveal. When parallel testing tests/shell/stat_all_pmu.sh this yields:
> > ```
> > Testing intel_bts//
> > Error:
> > The PMU intel_bts counters are busy and in use by another process.
> > Possible processes:
> > 2585882 perf list
> > 2585902 perf list -j -o /tmp/__perf_test.list_output.json.KF9MY
> > 2585904 perf list
> > 2585911 perf record -e task-clock --filter period > 1 -o /dev/null --quiet true
> > 2585912 perf list
> > 2585915 perf list
> > 2586042 /tmp/perf/perf record -asdg -e cpu-clock -o /tmp/perftool-testsuite_report.dIF/perf_report/perf.data -- sleep 2
> > 2589078 perf record -g -e task-clock:u -o - perf test -w noploop
> > 2589148 /tmp/perf/perf record --control=fifo:control,ack -e cpu-clock -m 1 sleep 10
> > 2589379 perf --buildid-dir /tmp/perf.debug.Umx record --buildid-all -o /tmp/perf.data.YBm /tmp/perf.ex.MD5.ZQW
> > 2589568 perf record -o /tmp/__perf_test.program.mtcZH/perf.data --branch-filter any,save_type,u -- perf test -w brstack
> > 2589649 perf record --per-thread -o /tmp/__perf_test.perf.data.5d3dc perf test -w thloop
> > 2589898 perf record -o /tmp/perf-test-script.BX2b27Dcnj/pp-perf.data --sample-cpu uname
> > ```
> > Which gets a little closer to finding the issue.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/evsel.c | 79 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 78 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index dbf9c8cee3c56..9a5b6a6f8d2e5 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -3286,6 +3286,78 @@ static bool find_process(const char *name)
> >       return ret ? false : true;
> >  }
> >
> > +static int dump_perf_event_processes(char *msg, size_t size)
> > +{
> > +     DIR *proc_dir;
> > +     struct dirent *proc_entry;
> > +     int printed = 0;
> > +
> > +     proc_dir = opendir(procfs__mountpoint());
> > +     if (!proc_dir)
> > +             return 0;
> > +
> > +     /* Walk through the /proc directory. */
> > +     while ((proc_entry = readdir(proc_dir)) != NULL) {
> > +             char path[PATH_MAX];
>
> Can we use a much smaller buffer as it expects PIDs only?
>

It is actually used as a general purpose buffer, not only for `path`.
It is also served for parsing `fdinfo` in patch 3, which I think 256
would be safe for now.

>
> > +             DIR *fd_dir;
> > +             struct dirent *fd_entry;
> > +             int fd_dir_fd;
> > +
> > +             if ((proc_entry->d_type != DT_DIR) ||
> > +                  !strcmp(".", proc_entry->d_name) ||
> > +                  !strcmp("..", proc_entry->d_name))
>
> Maybe something like this?

Fixed in v3.


>
>                 if (proc_entry->d_type != DT_DIR ||
>                     !isdigit(proc_entry->d_name[0]) ||
>                     strlen(proc_entry->d_name) > sizeof(path) - 4)
>
> Thanks,
> Namhyung
>
>
> > +                     continue;
> > +
> > +             scnprintf(path, sizeof(path), "%s/fd", proc_entry->d_name);
> > +             fd_dir_fd = openat(dirfd(proc_dir), path, O_DIRECTORY);
> > +             if (fd_dir_fd == -1)
> > +                     continue;
> > +             fd_dir = fdopendir(fd_dir_fd);
> > +             if (!fd_dir) {
> > +                     close(fd_dir_fd);
> > +                     continue;
> > +             }
> > +             while ((fd_entry = readdir(fd_dir)) != NULL) {
> > +                     ssize_t link_size;
> > +
> > +                     if (fd_entry->d_type != DT_LNK)
> > +                             continue;
> > +                     link_size = readlinkat(fd_dir_fd, fd_entry->d_name, path, sizeof(path));
> > +                     if (link_size < 0)
> > +                             continue;
> > +                     /* Take care as readlink doesn't null terminate the string. */
> > +                     if (!strncmp(path, "anon_inode:[perf_event]", link_size)) {
> > +                             int cmdline_fd;
> > +                             ssize_t cmdline_size;
> > +
> > +                             scnprintf(path, sizeof(path), "%s/cmdline", proc_entry->d_name);
> > +                             cmdline_fd = openat(dirfd(proc_dir), path, O_RDONLY);
> > +                             if (cmdline_fd == -1)
> > +                                     continue;
> > +                             cmdline_size = read(cmdline_fd, path, sizeof(path) - 1);
> > +                             close(cmdline_fd);
> > +                             if (cmdline_size < 0)
> > +                                     continue;
> > +                             path[cmdline_size] = '\0';
> > +                             for (ssize_t i = 0; i < cmdline_size; i++) {
> > +                                     if (path[i] == '\0')
> > +                                             path[i] = ' ';
> > +                             }
> > +
> > +                             if (printed == 0)
> > +                                     printed += scnprintf(msg, size, "Possible processes:\n");
> > +
> > +                             printed += scnprintf(msg + printed, size - printed,
> > +                                             "%s %s\n", proc_entry->d_name, path);
> > +                             break;
> > +                     }
> > +             }
> > +             closedir(fd_dir);
> > +     }
> > +     closedir(proc_dir);
> > +     return printed;
> > +}
> > +
> >  int __weak arch_evsel__open_strerror(struct evsel *evsel __maybe_unused,
> >                                    char *msg __maybe_unused,
> >                                    size_t size __maybe_unused)
> > @@ -3319,7 +3391,7 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
> >                       printed += scnprintf(msg, size,
> >                               "No permission to enable %s event.\n\n", evsel__name(evsel));
> >
> > -             return scnprintf(msg + printed, size - printed,
> > +             return printed + scnprintf(msg + printed, size - printed,
> >                "Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open\n"
> >                "access to performance monitoring and observability operations for processes\n"
> >                "without CAP_PERFMON, CAP_SYS_PTRACE or CAP_SYS_ADMIN Linux capability.\n"
> > @@ -3382,6 +3454,11 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
> >                       return scnprintf(msg, size,
> >       "The PMU counters are busy/taken by another profiler.\n"
> >       "We found oprofile daemon running, please stop it and try again.");
> > +             printed += scnprintf(
> > +                     msg, size,
> > +                     "The PMU %s counters are busy and in use by another process.\n",
> > +                     evsel->pmu ? evsel->pmu->name : "");
> > +             return printed + dump_perf_event_processes(msg + printed, size - printed);
> >               break;
> >       case EINVAL:
> >               if (evsel->core.attr.sample_type & PERF_SAMPLE_CODE_PAGE_SIZE && perf_missing_features.code_page_size)
> > --
> > 2.47.0.163.g1226f6d8fa-goog
> >

Re: [PATCH v2 2/3] perf: Reveal PMU type in fdinfo

[Chun-Tse Shao]

Thanks for your review, Namhyung! Here is the link to patch v3:
https://lore.kernel.org/all/20241106003007.2112584-2-ctshao@google.com/

On Mon, Nov 4, 2024 at 4:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Nov 01, 2024 at 09:17:56PM +0000, Chun-Tse Shao wrote:
> > It gives useful info on knowing which PMUs are reserved by this process.
> > Also add extra attributes which would be useful.
> >
> > ```
> > Testing cycles
> > $ ./perf stat -e cycles &
> > $ cat /proc/`pidof perf`/fdinfo/3
> > pos:    0
> > flags:  02000002
> > mnt_id: 16
> > ino:    3081
> > perf_event-attr.type:   0
>
> Maybe 'perf_event_attr' would be appropriate as it's the name of the
> struct.
>

Agree.

> > perf_event-attr.config: 0
> > perf_event-attr.config1:        0
> > perf_event-attr.config2:        0
> > perf_event-attr.config3:        0
>
> It's hard to pick which fields to show here but I'd say that those
> config[123] are not used frequently at least for regular events.
> Maybe just showing type and config is fine.
>

Agree, it should be fine to add more if needed.

> >
> > Testing L1-dcache-load-misses//
> > $ ./perf stat -e L1-dcache-load-misses &
> > $ cat /proc/`pidof perf`/fdinfo/3
> > pos:    0
> > flags:  02000002
> > mnt_id: 16
> > ino:    1072
> > perf_event-attr.type:   3
> > perf_event-attr.config: 65536
> > perf_event-attr.config1:        0
> > perf_event-attr.config2:        0
> > perf_event-attr.config3:        0
> > ```
> >
> > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> > ---
> >  kernel/events/core.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
>
> FYI usually the kernel changes are applied to a different tree than the
> tools.
>

Thanks for the heads up!

> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index cdd09769e6c56..c950b6fc92cda 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -55,6 +55,7 @@
> >  #include <linux/pgtable.h>
> >  #include <linux/buildid.h>
> >  #include <linux/task_work.h>
> > +#include <linux/seq_file.h>
> >
> >  #include "internal.h"
> >
> > @@ -6820,6 +6821,17 @@ static int perf_fasync(int fd, struct file *filp, int on)
> >       return 0;
> >  }
> >
> > +static void perf_show_fdinfo(struct seq_file *m, struct file *f)
> > +{
> > +     struct perf_event *event = f->private_data;
> > +
> > +     seq_printf(m, "perf_event-attr.type:\t%u\n", event->orig_type);
> > +     seq_printf(m, "perf_event-attr.config:\t%llu\n", event->attr.config);
>
> I'm not sure if all archs are happy with treating it as %llu.
>

I think with type casting to `unsigned long long`, it will make it
print in `%llu` for all archs.


> Thanks,
> Namhyung
>
>
> > +     seq_printf(m, "perf_event-attr.config1:\t%llu\n", event->attr.config1);
> > +     seq_printf(m, "perf_event-attr.config2:\t%llu\n", event->attr.config2);
> > +     seq_printf(m, "perf_event-attr.config3:\t%llu\n", event->attr.config3);
> > +}
> > +
> >  static const struct file_operations perf_fops = {
> >       .release                = perf_release,
> >       .read                   = perf_read,
> > @@ -6828,6 +6840,7 @@ static const struct file_operations perf_fops = {
> >       .compat_ioctl           = perf_compat_ioctl,
> >       .mmap                   = perf_mmap,
> >       .fasync                 = perf_fasync,
> > +     .show_fdinfo            = perf_show_fdinfo,
> >  };
> >
> >  /*
> > --
> > 2.47.0.163.g1226f6d8fa-goog
> >