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

[PATCH v3 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>
Change-Id: Ie1ed8688286c44e8f44a35e98fed8be3e2a344df
---
 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..d001ecfa26bf7 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 buf[256];
+		DIR *fd_dir;
+		struct dirent *fd_entry;
+		int fd_dir_fd;
+
+		if (proc_entry->d_type != DT_DIR ||
+		    !isdigit(proc_entry->d_name[0]) ||
+		    strlen(proc_entry->d_name) > sizeof(buf) - 4)
+			continue;
+
+		scnprintf(buf, sizeof(buf), "%s/fd", proc_entry->d_name);
+		fd_dir_fd = openat(dirfd(proc_dir), buf, 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, buf, sizeof(buf));
+			if (link_size < 0)
+				continue;
+			/* Take care as readlink doesn't null terminate the string. */
+			if (!strncmp(buf, "anon_inode:[perf_event]", link_size)) {
+				int cmdline_fd;
+				ssize_t cmdline_size;
+
+				scnprintf(buf, sizeof(buf), "%s/cmdline", proc_entry->d_name);
+				cmdline_fd = openat(dirfd(proc_dir), buf, O_RDONLY);
+				if (cmdline_fd == -1)
+					continue;
+				cmdline_size = read(cmdline_fd, buf, sizeof(buf) - 1);
+				close(cmdline_fd);
+				if (cmdline_size < 0)
+					continue;
+				buf[cmdline_size] = '\0';
+				for (ssize_t i = 0; i < cmdline_size; i++) {
+					if (buf[i] == '\0')
+						buf[i] = ' ';
+				}
+
+				if (printed == 0)
+					printed += scnprintf(msg, size, "Possible processes:\n");
+
+				printed += scnprintf(msg + printed, size - printed,
+						"%s %s\n", proc_entry->d_name, buf);
+				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.199.ga7371fff76-goog

[PATCH v3 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 config 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

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

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

diff --git a/kernel/events/core.c b/kernel/events/core.c
index cdd09769e6c56..398cac8b208b9 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,14 @@ 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", (unsigned long long)event->attr.config);
+}
+
 static const struct file_operations perf_fops = {
 	.release		= perf_release,
 	.read			= perf_read,
@@ -6828,6 +6837,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.199.ga7371fff76-goog

[PATCH v3 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>
Change-Id: Ic51a36ea3b2fd245663d7db78f35496bb4199d73
---
 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 d001ecfa26bf7..5400b795d9233 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(buf, "anon_inode:[perf_event]", link_size)) {
-				int cmdline_fd;
-				ssize_t cmdline_size;
-
-				scnprintf(buf, sizeof(buf), "%s/cmdline", proc_entry->d_name);
-				cmdline_fd = openat(dirfd(proc_dir), buf, O_RDONLY);
-				if (cmdline_fd == -1)
-					continue;
-				cmdline_size = read(cmdline_fd, buf, sizeof(buf) - 1);
-				close(cmdline_fd);
-				if (cmdline_size < 0)
+				int fdinfo_fd;
+				ssize_t fdinfo_size;
+				char *line;
+				u32 perf_event_type = UINT32_MAX;
+
+				/* Let's check the PMU type reserved by this process */
+				scnprintf(buf, sizeof(buf), "%s/fdinfo/%s",
+					  proc_entry->d_name, fd_entry->d_name);
+				fdinfo_fd = openat(dirfd(proc_dir), buf, O_RDONLY);
+				fdinfo_size = read(fdinfo_fd, buf, sizeof(buf) - 1);
+				if (fdinfo_size < 0)
 					continue;
-				buf[cmdline_size] = '\0';
-				for (ssize_t i = 0; i < cmdline_size; i++) {
-					if (buf[i] == '\0')
-						buf[i] = ' ';
+				buf[fdinfo_size] = '\0';
+
+				line = strtok(buf, "\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, buf);
-				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 == UINT32_MAX) {
+					int cmdline_fd;
+					ssize_t cmdline_size;
+
+					scnprintf(buf, sizeof(buf),
+						  "%s/cmdline",
+						  proc_entry->d_name);
+					cmdline_fd = openat(dirfd(proc_dir), buf, O_RDONLY);
+					if (cmdline_fd == -1)
+						continue;
+					cmdline_size = read(cmdline_fd, buf, sizeof(buf) - 1);
+					close(cmdline_fd);
+					if (cmdline_size < 0)
+						continue;
+					buf[cmdline_size] = '\0';
+					for (ssize_t i = 0; i < cmdline_size; i++) {
+						if (buf[i] == '\0')
+							buf[i] = ' ';
+					}
+
+					if (printed == 0)
+						printed += scnprintf(
+							msg, size,
+							"Possible processes:\n");
+
+					printed += scnprintf(msg + printed, size - printed,
+							"%s %s\n", proc_entry->d_name, buf);
+					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.199.ga7371fff76-goog

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

[Ian Rogers]

On Tue, Nov 5, 2024 at 4:30 PM Chun-Tse Shao <ctshao@google.com> wrote:
>
> It gives useful info on knowing which PMUs are reserved by this process.
> Also add config 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
>
> 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
>
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>

Reviewed-by: Ian Rogers <irogers@google.com>

> Change-Id: Ibea5618aaf00bae6f48a9b2a6e7798ab2b7f23ce
> ---
>  kernel/events/core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index cdd09769e6c56..398cac8b208b9 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,14 @@ 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", (unsigned long long)event->attr.config);

nit: is the cast necessary? I don't see __u64 listed on:
https://www.kernel.org/doc/Documentation/printk-formats.txt
so I'm unsure.

Thanks,
Ian

> +}
> +
>  static const struct file_operations perf_fops = {
>         .release                = perf_release,
>         .read                   = perf_read,
> @@ -6828,6 +6837,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.199.ga7371fff76-goog
>

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

[Ian Rogers]

On Tue, Nov 5, 2024 at 4:30 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.

I think this looks really nice!

> Signed-off-by: Chun-Tse Shao <ctshao@google.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Change-Id: Ic51a36ea3b2fd245663d7db78f35496bb4199d73
> ---
>  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 d001ecfa26bf7..5400b795d9233 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(buf, "anon_inode:[perf_event]", link_size)) {
> -                               int cmdline_fd;
> -                               ssize_t cmdline_size;
> -
> -                               scnprintf(buf, sizeof(buf), "%s/cmdline", proc_entry->d_name);
> -                               cmdline_fd = openat(dirfd(proc_dir), buf, O_RDONLY);
> -                               if (cmdline_fd == -1)
> -                                       continue;
> -                               cmdline_size = read(cmdline_fd, buf, sizeof(buf) - 1);
> -                               close(cmdline_fd);
> -                               if (cmdline_size < 0)
> +                               int fdinfo_fd;
> +                               ssize_t fdinfo_size;
> +                               char *line;
> +                               u32 perf_event_type = UINT32_MAX;
> +
> +                               /* Let's check the PMU type reserved by this process */
> +                               scnprintf(buf, sizeof(buf), "%s/fdinfo/%s",
> +                                         proc_entry->d_name, fd_entry->d_name);
> +                               fdinfo_fd = openat(dirfd(proc_dir), buf, O_RDONLY);
> +                               fdinfo_size = read(fdinfo_fd, buf, sizeof(buf) - 1);
> +                               if (fdinfo_size < 0)
>                                         continue;
> -                               buf[cmdline_size] = '\0';
> -                               for (ssize_t i = 0; i < cmdline_size; i++) {
> -                                       if (buf[i] == '\0')
> -                                               buf[i] = ' ';
> +                               buf[fdinfo_size] = '\0';
> +
> +                               line = strtok(buf, "\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, buf);
> -                               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 == UINT32_MAX) {
> +                                       int cmdline_fd;
> +                                       ssize_t cmdline_size;
> +
> +                                       scnprintf(buf, sizeof(buf),
> +                                                 "%s/cmdline",
> +                                                 proc_entry->d_name);
> +                                       cmdline_fd = openat(dirfd(proc_dir), buf, O_RDONLY);
> +                                       if (cmdline_fd == -1)
> +                                               continue;
> +                                       cmdline_size = read(cmdline_fd, buf, sizeof(buf) - 1);
> +                                       close(cmdline_fd);
> +                                       if (cmdline_size < 0)
> +                                               continue;
> +                                       buf[cmdline_size] = '\0';
> +                                       for (ssize_t i = 0; i < cmdline_size; i++) {
> +                                               if (buf[i] == '\0')
> +                                                       buf[i] = ' ';
> +                                       }
> +
> +                                       if (printed == 0)
> +                                               printed += scnprintf(
> +                                                       msg, size,
> +                                                       "Possible processes:\n");
> +
> +                                       printed += scnprintf(msg + printed, size - printed,
> +                                                       "%s %s\n", proc_entry->d_name, buf);
> +                                       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.199.ga7371fff76-goog
>

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

[Chun-Tse Shao]

Hi Ian,

On Thu, Nov 14, 2024 at 7:49 AM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Nov 5, 2024 at 4:30 PM Chun-Tse Shao <ctshao@google.com> wrote:
> >
> > It gives useful info on knowing which PMUs are reserved by this proceszs.
> > Also add config 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
> >
> > 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
> >
> > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
>
> Reviewed-by: Ian Rogers <irogers@google.com>
>
> > Change-Id: Ibea5618aaf00bae6f48a9b2a6e7798ab2b7f23ce
> > ---
> >  kernel/events/core.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index cdd09769e6c56..398cac8b208b9 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,14 @@ 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", (unsigned long long)event->attr.config);
>
> nit: is the cast necessary? I don't see __u64 listed on:
> https://www.kernel.org/doc/Documentation/printk-formats.txt
> so I'm unsure.
>

In this case I think it is safer to cast it to `unsigned long long`,
since I don't know if any architecture would have an exception on
__u64.

Thanks,
CT

> Thanks,
> Ian
>
> > +}
> > +
> >  static const struct file_operations perf_fops = {
> >         .release                = perf_release,
> >         .read                   = perf_read,
> > @@ -6828,6 +6837,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.199.ga7371fff76-goog
> >

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

[Chun-Tse Shao]

Hi, just a friendly ping for review, in case anyone missed this.

Thanks,
CT

On Thu, Nov 14, 2024 at 10:30 AM Chun-Tse Shao <ctshao@google.com> wrote:
>
> Hi Ian,
>
> On Thu, Nov 14, 2024 at 7:49 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Tue, Nov 5, 2024 at 4:30 PM Chun-Tse Shao <ctshao@google.com> wrote:
> > >
> > > It gives useful info on knowing which PMUs are reserved by this proceszs.
> > > Also add config 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
> > >
> > > 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
> > >
> > > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> >
> > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > > Change-Id: Ibea5618aaf00bae6f48a9b2a6e7798ab2b7f23ce
> > > ---
> > >  kernel/events/core.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index cdd09769e6c56..398cac8b208b9 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,14 @@ 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", (unsigned long long)event->attr.config);
> >
> > nit: is the cast necessary? I don't see __u64 listed on:
> > https://www.kernel.org/doc/Documentation/printk-formats.txt
> > so I'm unsure.
> >
>
> In this case I think it is safer to cast it to `unsigned long long`,
> since I don't know if any architecture would have an exception on
> __u64.
>
> Thanks,
> CT
>
> > Thanks,
> > Ian
> >
> > > +}
> > > +
> > >  static const struct file_operations perf_fops = {
> > >         .release                = perf_release,
> > >         .read                   = perf_read,
> > > @@ -6828,6 +6837,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.199.ga7371fff76-goog
> > >

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

[Chun-Tse Shao]

Hi Peter, just want to bring your attention to this patch since you
left comments on the previous version of patch.

Thanks,
CT

On Thu, Nov 21, 2024 at 1:18 PM Chun-Tse Shao <ctshao@google.com> wrote:
>
> Hi, just a friendly ping for review, in case anyone missed this.
>
> Thanks,
> CT
>
> On Thu, Nov 14, 2024 at 10:30 AM Chun-Tse Shao <ctshao@google.com> wrote:
> >
> > Hi Ian,
> >
> > On Thu, Nov 14, 2024 at 7:49 AM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Tue, Nov 5, 2024 at 4:30 PM Chun-Tse Shao <ctshao@google.com> wrote:
> > > >
> > > > It gives useful info on knowing which PMUs are reserved by this proceszs.
> > > > Also add config 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
> > > >
> > > > 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
> > > >
> > > > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> > >
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> > >
> > > > Change-Id: Ibea5618aaf00bae6f48a9b2a6e7798ab2b7f23ce
> > > > ---
> > > >  kernel/events/core.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > > index cdd09769e6c56..398cac8b208b9 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,14 @@ 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", (unsigned long long)event->attr.config);
> > >
> > > nit: is the cast necessary? I don't see __u64 listed on:
> > > https://www.kernel.org/doc/Documentation/printk-formats.txt
> > > so I'm unsure.
> > >
> >
> > In this case I think it is safer to cast it to `unsigned long long`,
> > since I don't know if any architecture would have an exception on
> > __u64.
> >
> > Thanks,
> > CT
> >
> > > Thanks,
> > > Ian
> > >
> > > > +}
> > > > +
> > > >  static const struct file_operations perf_fops = {
> > > >         .release                = perf_release,
> > > >         .read                   = perf_read,
> > > > @@ -6828,6 +6837,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.199.ga7371fff76-goog
> > > >

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

[Ian Rogers]

On Tue, Nov 5, 2024 at 4:30 PM Chun-Tse Shao <ctshao@google.com> 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.
>
> 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>

Ping.

Thanks,
Ian

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

[Arnaldo Carvalho de Melo]

On Mon, Jan 06, 2025 at 01:59:25PM -0800, Ian Rogers wrote:
> On Tue, Nov 5, 2024 at 4:30 PM Chun-Tse Shao <ctshao@google.com> 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.
> >
> > 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>
> 
> Ping.

Thanks for the ping, great stuff, testing the first patch:

Committer testing:

  root@number:~#
  root@number:~# grep -m1 "model name" /proc/cpuinfo
  model name    : Intel(R) Core(TM) i7-14700K
  root@number:~#

Before:

  root@number:~# perf stat -e intel_bts// &
  [1] 197954
  root@number:~# perf test "perf all PMU test"
  124: perf all PMU test                                               : FAILED!
  root@number:~# perf test -v "perf all PMU test" |& tail
  Testing i915/vecs0-busy/
  Testing i915/vecs0-sema/
  Testing i915/vecs0-wait/
  Testing intel_bts//
  Unexpected signal in main
  Error:
  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.
  ---- end(-1) ----
  124: perf all PMU test                                               : FAILED!
  root@number:~#

After:

  root@number:~# perf stat -e intel_bts// &
  [1] 200195
  root@number:~# perf test "perf all PMU test"
  123: perf all PMU test                                               : FAILED!
  root@number:~# perf test -v "perf all PMU test" |& tail
  Testing i915/vecs0-wait/
  Testing intel_bts//
  Unexpected signal in main
  Error:
  The PMU intel_bts counters are busy and in use by another process.
  Possible processes:
  200195 perf stat -e intel_bts//
  2319766 /root/bin/perf top --stdio
  ---- end(-1) ----
  123: perf all PMU test                                               : FAILED!
  root@number:~#

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

[Arnaldo Carvalho de Melo]

On Wed, Nov 06, 2024 at 12:30:06AM +0000, Chun-Tse Shao wrote:
> It gives useful info on knowing which PMUs are reserved by this process.
> Also add config which would be useful.
> Testing cycles:

I applied the first patch in the series, that already helps, Peter, what
about this one for the kernel?

- Arnaldo
 
>   $ ./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
> 
> 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
> 
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> Change-Id: Ibea5618aaf00bae6f48a9b2a6e7798ab2b7f23ce
> ---
>  kernel/events/core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index cdd09769e6c56..398cac8b208b9 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,14 @@ 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", (unsigned long long)event->attr.config);
> +}
> +
>  static const struct file_operations perf_fops = {
>  	.release		= perf_release,
>  	.read			= perf_read,
> @@ -6828,6 +6837,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.199.ga7371fff76-goog

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

[Ian Rogers]

On Tue, Jan 7, 2025 at 11:44 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Wed, Nov 06, 2024 at 12:30:06AM +0000, Chun-Tse Shao wrote:
> > It gives useful info on knowing which PMUs are reserved by this process.
> > Also add config which would be useful.
> > Testing cycles:
>
> I applied the first patch in the series, that already helps, Peter, what
> about this one for the kernel?

Peter, did you have any input on this? I think this is simple value
add with no real downside. I kind of wish the patch did more than just
dumping type and config in fdinfo, but getting those two is a big
start.

Thanks,
Ian

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

[Ian Rogers]

On Mon, Mar 17, 2025 at 8:42 AM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Jan 7, 2025 at 11:44 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Wed, Nov 06, 2024 at 12:30:06AM +0000, Chun-Tse Shao wrote:
> > > It gives useful info on knowing which PMUs are reserved by this process.
> > > Also add config which would be useful.
> > > Testing cycles:
> >
> > I applied the first patch in the series, that already helps, Peter, what
> > about this one for the kernel?
>
> Peter, did you have any input on this? I think this is simple value
> add with no real downside. I kind of wish the patch did more than just
> dumping type and config in fdinfo, but getting those two is a big
> start.

Ping.

Thanks,
Ian