[PATCH 0/2] perf/ibs: Fix interface via core pmu events

[PATCH 0/2] perf/ibs: Fix interface via core pmu events

[Ravi Bangoria]

IBS used to allow event creation via core pmu events with precise_ip
attribute set, but it's broken since late 2019. 1st patch fixes the
issue and 2nd patch adds simple perf test for the same.

Ravi Bangoria (2):
  perf/ibs: Fix interface via core pmu events
  perf test: Add selftest to test IBS invocation via core pmu events

 arch/x86/events/amd/core.c          |  5 +++
 arch/x86/events/core.c              |  2 +
 arch/x86/events/perf_event.h        |  3 ++
 include/linux/perf_event.h          |  1 +
 kernel/events/core.c                | 11 +++--
 tools/perf/tests/Build              |  1 +
 tools/perf/tests/builtin-test.c     |  1 +
 tools/perf/tests/ibs-via-core-pmu.c | 70 +++++++++++++++++++++++++++++
 tools/perf/tests/tests.h            |  1 +
 9 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100644 tools/perf/tests/ibs-via-core-pmu.c

-- 
2.39.2

[PATCH 1/2] perf/ibs: Fix interface via core pmu events

[Ravi Bangoria]

Although, IBS pmu can be invoked via it's own interface, indirect
IBS invocation via core pmu event is also supported with fixed set
of events: cpu-cycles:p, r076:p (same as cpu-cycles:p) and r0C1:p
(micro-ops) for user convenience.

This indirect IBS invocation is broken since commit 66d258c5b048
("perf/core: Optimize perf_init_event()"), which added RAW pmu
under pmu_idr list and thus if event_init() fails with RAW pmu,
it started returning error instead of trying other pmus.

Fix it by introducing new pmu capability PERF_PMU_CAP_FORWARD_EVENT.
Kernel will try to open event on other pmus if user requested pmu,
having this capability, fails to open event.

Without patch:
  $ sudo ./perf record -C 0 -e r076:p -- sleep 1
  Error:
  The r076:p event is not supported.

With patch:
  $ sudo ./perf record -C 0 -e r076:p -- sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.341 MB perf.data (37 samples) ]

This new capability does not have a notion of forward pmu mapping.
i.e. it doesn't know which pmu(or set of pmus) the event should be
forwarded to. As of now, only AMD core pmu forwards a set of events
to IBS pmu when precise_ip attribute is set and thus trying with all
pmus works. But if more pmus start using this capability, some sort
of forward pmu mapping needs to be introduced through which the event
can directly get forwarded to only mapped pmus. Otherwise, trying all
pmus can inadvertently open event on wrong pmu.

Fixes: 66d258c5b048 ("perf/core: Optimize perf_init_event()")
Reported-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/core.c   |  5 +++++
 arch/x86/events/core.c       |  2 ++
 arch/x86/events/perf_event.h |  3 +++
 include/linux/perf_event.h   |  1 +
 kernel/events/core.c         | 11 ++++++++---
 5 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index 8c45b198b62f..f4c67362cfde 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -1264,6 +1264,11 @@ static __initconst const struct x86_pmu amd_pmu = {
 	.cpu_dead		= amd_pmu_cpu_dead,
 
 	.amd_nb_constraints	= 1,
+	/*
+	 * Raw events with precise attribute set needs to be
+	 * forwarded to IBS pmu.
+	 */
+	.capabilities		= PERF_PMU_CAP_FORWARD_EVENT,
 };
 
 static ssize_t branches_show(struct device *cdev,
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index d096b04bf80e..3f27b44f337a 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2156,6 +2156,8 @@ static int __init init_hw_perf_events(void)
 	if (err)
 		goto out1;
 
+	pmu.capabilities |= x86_pmu.capabilities;
+
 	if (!is_hybrid()) {
 		err = perf_pmu_register(&pmu, "cpu", PERF_TYPE_RAW);
 		if (err)
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index d6de4487348c..41e792bb442d 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -941,6 +941,9 @@ struct x86_pmu {
 	int				num_hybrid_pmus;
 	struct x86_hybrid_pmu		*hybrid_pmu;
 	u8 (*get_hybrid_cpu_type)	(void);
+
+	/* Capabilities that needs to be forwarded to pmu->capabilities */
+	int				capabilities;
 };
 
 struct x86_perf_task_context_opt {
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index d5628a7b5eaa..4459e0918e28 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -292,6 +292,7 @@ struct perf_event_pmu_context;
 #define PERF_PMU_CAP_NO_EXCLUDE			0x0080
 #define PERF_PMU_CAP_AUX_OUTPUT			0x0100
 #define PERF_PMU_CAP_EXTENDED_HW_TYPE		0x0200
+#define PERF_PMU_CAP_FORWARD_EVENT		0x0400
 
 struct perf_output_handle;
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index a5a51dfdd622..c3f59d937280 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11633,9 +11633,13 @@ static struct pmu *perf_init_event(struct perf_event *event)
 			goto fail;
 
 		ret = perf_try_init_event(pmu, event);
-		if (ret == -ENOENT && event->attr.type != type && !extended_type) {
-			type = event->attr.type;
-			goto again;
+		if (ret == -ENOENT) {
+			if (event->attr.type != type && !extended_type) {
+				type = event->attr.type;
+				goto again;
+			}
+			if (pmu->capabilities & PERF_PMU_CAP_FORWARD_EVENT)
+				goto try_all;
 		}
 
 		if (ret)
@@ -11644,6 +11648,7 @@ static struct pmu *perf_init_event(struct perf_event *event)
 		goto unlock;
 	}
 
+try_all:
 	list_for_each_entry_rcu(pmu, &pmus, entry, lockdep_is_held(&pmus_srcu)) {
 		ret = perf_try_init_event(pmu, event);
 		if (!ret)
-- 
2.39.2

[PATCH 2/2] perf test: Add selftest to test IBS invocation via core pmu events

[Ravi Bangoria]

IBS pmu can be invoked via fixed set of core pmu events. Add simple
event open test for all these events.

Without kernel fix:
  $ sudo ./perf test -vv 76
   76: IBS via core pmu                                                :
  --- start ---
  test child forked, pid 6553
  Using CPUID AuthenticAMD-25-1-1
  type: 0x0, config: 0x0, fd: 3  -  Pass
  type: 0x0, config: 0x1, fd: -1  -  Pass
  type: 0x4, config: 0x76, fd: -1  -  Fail
  type: 0x4, config: 0xc1, fd: -1  -  Fail
  type: 0x4, config: 0x12, fd: -1  -  Pass
  test child finished with -1
  ---- end ----
  IBS via core pmu: FAILED!

With kernel fix:
  $ sudo ./perf test -vv 76
   76: IBS via core pmu                                                :
  --- start ---
  test child forked, pid 7526
  Using CPUID AuthenticAMD-25-1-1
  type: 0x0, config: 0x0, fd: 3  -  Pass
  type: 0x0, config: 0x1, fd: -1  -  Pass
  type: 0x4, config: 0x76, fd: 3  -  Pass
  type: 0x4, config: 0xc1, fd: 3  -  Pass
  type: 0x4, config: 0x12, fd: -1  -  Pass
  test child finished with 0
  ---- end ----
  IBS via core pmu: Ok

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 tools/perf/tests/Build              |  1 +
 tools/perf/tests/builtin-test.c     |  1 +
 tools/perf/tests/ibs-via-core-pmu.c | 70 +++++++++++++++++++++++++++++
 tools/perf/tests/tests.h            |  1 +
 4 files changed, 73 insertions(+)
 create mode 100644 tools/perf/tests/ibs-via-core-pmu.c

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index fb9ac5dc4079..1a232cf13c33 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -69,6 +69,7 @@ perf-y += dlfilter-test.o
 perf-y += sigtrap.o
 perf-y += event_groups.o
 perf-y += symbols.o
+perf-y += ibs-via-core-pmu.o
 
 $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
 	$(call rule_mkdir)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 35cc3807cc9e..aed887234500 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -119,6 +119,7 @@ static struct test_suite *generic_tests[] = {
 	&suite__sigtrap,
 	&suite__event_groups,
 	&suite__symbols,
+	&suite__ibs_via_core_pmu,
 	NULL,
 };
 
diff --git a/tools/perf/tests/ibs-via-core-pmu.c b/tools/perf/tests/ibs-via-core-pmu.c
new file mode 100644
index 000000000000..6ac539509791
--- /dev/null
+++ b/tools/perf/tests/ibs-via-core-pmu.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "linux/perf_event.h"
+#include "tests.h"
+#include "pmu.h"
+#include "pmus.h"
+#include "../perf-sys.h"
+#include "debug.h"
+
+#define NR_SUB_TESTS 5
+
+static struct sub_tests {
+	int type;
+	unsigned long config;
+	bool valid;
+} sub_tests[NR_SUB_TESTS] = {
+	{ PERF_TYPE_HARDWARE, PERF_COUNT_HW_CPU_CYCLES, true },
+	{ PERF_TYPE_HARDWARE, PERF_COUNT_HW_INSTRUCTIONS, false },
+	{ PERF_TYPE_RAW, 0x076, true },
+	{ PERF_TYPE_RAW, 0x0C1, true },
+	{ PERF_TYPE_RAW, 0x012, false },
+};
+
+static int event_open(int type, unsigned long config)
+{
+	struct perf_event_attr attr;
+
+	memset(&attr, 0, sizeof(struct perf_event_attr));
+	attr.type = type;
+	attr.size = sizeof(struct perf_event_attr);
+	attr.config = config;
+	attr.disabled = 1;
+	attr.precise_ip = 1;
+	attr.sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_TID;
+	attr.sample_period = 100000;
+
+	return sys_perf_event_open(&attr, -1, 0, -1, 0);
+}
+
+static int test__ibs_via_core_pmu(struct test_suite *text __maybe_unused,
+				  int subtest __maybe_unused)
+{
+	struct perf_pmu *ibs_pmu;
+	int ret = TEST_OK;
+	int fd, i;
+
+	if (list_empty(&pmus))
+		perf_pmu__scan(NULL);
+
+	ibs_pmu = perf_pmu__find("ibs_op");
+	if (!ibs_pmu)
+		return TEST_SKIP;
+
+	for (i = 0; i < NR_SUB_TESTS; i++) {
+		fd = event_open(sub_tests[i].type, sub_tests[i].config);
+		pr_debug("type: 0x%x, config: 0x%lx, fd: %d  -  ", sub_tests[i].type,
+			 sub_tests[i].config, fd);
+		if ((sub_tests[i].valid && fd == -1) ||
+		    (!sub_tests[i].valid && fd > 0)) {
+			pr_debug("Fail\n");
+			ret = TEST_FAIL;
+		} else {
+			pr_debug("Pass\n");
+			close(fd);
+		}
+	}
+
+	return ret;
+}
+
+DEFINE_SUITE("IBS via core pmu", ibs_via_core_pmu);
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 9a0f3904e53d..36339fdf9c36 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -149,6 +149,7 @@ DECLARE_SUITE(dlfilter);
 DECLARE_SUITE(sigtrap);
 DECLARE_SUITE(event_groups);
 DECLARE_SUITE(symbols);
+DECLARE_SUITE(ibs_via_core_pmu);
 
 /*
  * PowerPC and S390 do not support creation of instruction breakpoints using the
-- 
2.39.2

Re: [PATCH 1/2] perf/ibs: Fix interface via core pmu events

[Namhyung Kim]

Hi Ravi,

On Thu, Mar 2, 2023 at 1:22 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Although, IBS pmu can be invoked via it's own interface, indirect
> IBS invocation via core pmu event is also supported with fixed set
> of events: cpu-cycles:p, r076:p (same as cpu-cycles:p) and r0C1:p
> (micro-ops) for user convenience.
>
> This indirect IBS invocation is broken since commit 66d258c5b048
> ("perf/core: Optimize perf_init_event()"), which added RAW pmu
> under pmu_idr list and thus if event_init() fails with RAW pmu,
> it started returning error instead of trying other pmus.
>
> Fix it by introducing new pmu capability PERF_PMU_CAP_FORWARD_EVENT.
> Kernel will try to open event on other pmus if user requested pmu,
> having this capability, fails to open event.
>
> Without patch:
>   $ sudo ./perf record -C 0 -e r076:p -- sleep 1
>   Error:
>   The r076:p event is not supported.
>
> With patch:
>   $ sudo ./perf record -C 0 -e r076:p -- sleep 1
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.341 MB perf.data (37 samples) ]
>
> This new capability does not have a notion of forward pmu mapping.
> i.e. it doesn't know which pmu(or set of pmus) the event should be
> forwarded to. As of now, only AMD core pmu forwards a set of events
> to IBS pmu when precise_ip attribute is set and thus trying with all
> pmus works. But if more pmus start using this capability, some sort
> of forward pmu mapping needs to be introduced through which the event
> can directly get forwarded to only mapped pmus. Otherwise, trying all
> pmus can inadvertently open event on wrong pmu.
>
> Fixes: 66d258c5b048 ("perf/core: Optimize perf_init_event()")
> Reported-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  arch/x86/events/amd/core.c   |  5 +++++
>  arch/x86/events/core.c       |  2 ++
>  arch/x86/events/perf_event.h |  3 +++
>  include/linux/perf_event.h   |  1 +
>  kernel/events/core.c         | 11 ++++++++---
>  5 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 8c45b198b62f..f4c67362cfde 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -1264,6 +1264,11 @@ static __initconst const struct x86_pmu amd_pmu = {
>         .cpu_dead               = amd_pmu_cpu_dead,
>
>         .amd_nb_constraints     = 1,
> +       /*
> +        * Raw events with precise attribute set needs to be
> +        * forwarded to IBS pmu.
> +        */
> +       .capabilities           = PERF_PMU_CAP_FORWARD_EVENT,
>  };
>
>  static ssize_t branches_show(struct device *cdev,
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index d096b04bf80e..3f27b44f337a 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2156,6 +2156,8 @@ static int __init init_hw_perf_events(void)
>         if (err)
>                 goto out1;
>
> +       pmu.capabilities |= x86_pmu.capabilities;
> +
>         if (!is_hybrid()) {
>                 err = perf_pmu_register(&pmu, "cpu", PERF_TYPE_RAW);
>                 if (err)
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index d6de4487348c..41e792bb442d 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -941,6 +941,9 @@ struct x86_pmu {
>         int                             num_hybrid_pmus;
>         struct x86_hybrid_pmu           *hybrid_pmu;
>         u8 (*get_hybrid_cpu_type)       (void);
> +
> +       /* Capabilities that needs to be forwarded to pmu->capabilities */
> +       int                             capabilities;
>  };
>
>  struct x86_perf_task_context_opt {
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index d5628a7b5eaa..4459e0918e28 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -292,6 +292,7 @@ struct perf_event_pmu_context;
>  #define PERF_PMU_CAP_NO_EXCLUDE                        0x0080
>  #define PERF_PMU_CAP_AUX_OUTPUT                        0x0100
>  #define PERF_PMU_CAP_EXTENDED_HW_TYPE          0x0200
> +#define PERF_PMU_CAP_FORWARD_EVENT             0x0400
>
>  struct perf_output_handle;
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a5a51dfdd622..c3f59d937280 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11633,9 +11633,13 @@ static struct pmu *perf_init_event(struct perf_event *event)
>                         goto fail;
>
>                 ret = perf_try_init_event(pmu, event);
> -               if (ret == -ENOENT && event->attr.type != type && !extended_type) {
> -                       type = event->attr.type;
> -                       goto again;
> +               if (ret == -ENOENT) {
> +                       if (event->attr.type != type && !extended_type) {
> +                               type = event->attr.type;
> +                               goto again;
> +                       }
> +                       if (pmu->capabilities & PERF_PMU_CAP_FORWARD_EVENT)
> +                               goto try_all;

Wouldn't it be better to use a different error code to indicate
it's about precise_ip (or forwarding in general)?  Otherwise
other invalid config might cause the forwarding unnecessarily..

Thanks,
Namhyung


>                 }
>
>                 if (ret)
> @@ -11644,6 +11648,7 @@ static struct pmu *perf_init_event(struct perf_event *event)
>                 goto unlock;
>         }
>
> +try_all:
>         list_for_each_entry_rcu(pmu, &pmus, entry, lockdep_is_held(&pmus_srcu)) {
>                 ret = perf_try_init_event(pmu, event);
>                 if (!ret)
> --
> 2.39.2
>

Re: [PATCH 2/2] perf test: Add selftest to test IBS invocation via core pmu events

[Namhyung Kim]

On Thu, Mar 2, 2023 at 1:22 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> IBS pmu can be invoked via fixed set of core pmu events. Add simple
> event open test for all these events.
>
> Without kernel fix:
>   $ sudo ./perf test -vv 76
>    76: IBS via core pmu                                                :
>   --- start ---
>   test child forked, pid 6553
>   Using CPUID AuthenticAMD-25-1-1
>   type: 0x0, config: 0x0, fd: 3  -  Pass
>   type: 0x0, config: 0x1, fd: -1  -  Pass
>   type: 0x4, config: 0x76, fd: -1  -  Fail
>   type: 0x4, config: 0xc1, fd: -1  -  Fail
>   type: 0x4, config: 0x12, fd: -1  -  Pass
>   test child finished with -1
>   ---- end ----
>   IBS via core pmu: FAILED!
>
> With kernel fix:
>   $ sudo ./perf test -vv 76
>    76: IBS via core pmu                                                :
>   --- start ---
>   test child forked, pid 7526
>   Using CPUID AuthenticAMD-25-1-1
>   type: 0x0, config: 0x0, fd: 3  -  Pass
>   type: 0x0, config: 0x1, fd: -1  -  Pass
>   type: 0x4, config: 0x76, fd: 3  -  Pass
>   type: 0x4, config: 0xc1, fd: 3  -  Pass
>   type: 0x4, config: 0x12, fd: -1  -  Pass
>   test child finished with 0
>   ---- end ----
>   IBS via core pmu: Ok
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  tools/perf/tests/Build              |  1 +
>  tools/perf/tests/builtin-test.c     |  1 +
>  tools/perf/tests/ibs-via-core-pmu.c | 70 +++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h            |  1 +
>  4 files changed, 73 insertions(+)
>  create mode 100644 tools/perf/tests/ibs-via-core-pmu.c
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index fb9ac5dc4079..1a232cf13c33 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -69,6 +69,7 @@ perf-y += dlfilter-test.o
>  perf-y += sigtrap.o
>  perf-y += event_groups.o
>  perf-y += symbols.o
> +perf-y += ibs-via-core-pmu.o
>
>  $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
>         $(call rule_mkdir)
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 35cc3807cc9e..aed887234500 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -119,6 +119,7 @@ static struct test_suite *generic_tests[] = {
>         &suite__sigtrap,
>         &suite__event_groups,
>         &suite__symbols,
> +       &suite__ibs_via_core_pmu,
>         NULL,
>  };
>
> diff --git a/tools/perf/tests/ibs-via-core-pmu.c b/tools/perf/tests/ibs-via-core-pmu.c
> new file mode 100644
> index 000000000000..6ac539509791
> --- /dev/null
> +++ b/tools/perf/tests/ibs-via-core-pmu.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "linux/perf_event.h"
> +#include "tests.h"
> +#include "pmu.h"
> +#include "pmus.h"
> +#include "../perf-sys.h"
> +#include "debug.h"
> +
> +#define NR_SUB_TESTS 5
> +
> +static struct sub_tests {
> +       int type;
> +       unsigned long config;
> +       bool valid;
> +} sub_tests[NR_SUB_TESTS] = {
> +       { PERF_TYPE_HARDWARE, PERF_COUNT_HW_CPU_CYCLES, true },
> +       { PERF_TYPE_HARDWARE, PERF_COUNT_HW_INSTRUCTIONS, false },
> +       { PERF_TYPE_RAW, 0x076, true },
> +       { PERF_TYPE_RAW, 0x0C1, true },
> +       { PERF_TYPE_RAW, 0x012, false },
> +};
> +
> +static int event_open(int type, unsigned long config)
> +{
> +       struct perf_event_attr attr;
> +
> +       memset(&attr, 0, sizeof(struct perf_event_attr));
> +       attr.type = type;
> +       attr.size = sizeof(struct perf_event_attr);
> +       attr.config = config;
> +       attr.disabled = 1;
> +       attr.precise_ip = 1;
> +       attr.sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_TID;
> +       attr.sample_period = 100000;
> +
> +       return sys_perf_event_open(&attr, -1, 0, -1, 0);
> +}
> +
> +static int test__ibs_via_core_pmu(struct test_suite *text __maybe_unused,
> +                                 int subtest __maybe_unused)
> +{
> +       struct perf_pmu *ibs_pmu;
> +       int ret = TEST_OK;
> +       int fd, i;
> +
> +       if (list_empty(&pmus))
> +               perf_pmu__scan(NULL);
> +
> +       ibs_pmu = perf_pmu__find("ibs_op");
> +       if (!ibs_pmu)
> +               return TEST_SKIP;
> +
> +       for (i = 0; i < NR_SUB_TESTS; i++) {
> +               fd = event_open(sub_tests[i].type, sub_tests[i].config);
> +               pr_debug("type: 0x%x, config: 0x%lx, fd: %d  -  ", sub_tests[i].type,
> +                        sub_tests[i].config, fd);
> +               if ((sub_tests[i].valid && fd == -1) ||
> +                   (!sub_tests[i].valid && fd > 0)) {
> +                       pr_debug("Fail\n");
> +                       ret = TEST_FAIL;

You need to close(fd) when it's succeeded for invalid one.

Thanks,
Namhyung


> +               } else {
> +                       pr_debug("Pass\n");
> +                       close(fd);
> +               }
> +       }
> +
> +       return ret;
> +}
> +
> +DEFINE_SUITE("IBS via core pmu", ibs_via_core_pmu);
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 9a0f3904e53d..36339fdf9c36 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -149,6 +149,7 @@ DECLARE_SUITE(dlfilter);
>  DECLARE_SUITE(sigtrap);
>  DECLARE_SUITE(event_groups);
>  DECLARE_SUITE(symbols);
> +DECLARE_SUITE(ibs_via_core_pmu);
>
>  /*
>   * PowerPC and S390 do not support creation of instruction breakpoints using the
> --
> 2.39.2
>

Re: [PATCH 1/2] perf/ibs: Fix interface via core pmu events

[Ravi Bangoria]

Hi Namhyung,

>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index a5a51dfdd622..c3f59d937280 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -11633,9 +11633,13 @@ static struct pmu *perf_init_event(struct perf_event *event)
>>                         goto fail;
>>
>>                 ret = perf_try_init_event(pmu, event);
>> -               if (ret == -ENOENT && event->attr.type != type && !extended_type) {
>> -                       type = event->attr.type;
>> -                       goto again;
>> +               if (ret == -ENOENT) {
>> +                       if (event->attr.type != type && !extended_type) {
>> +                               type = event->attr.type;
>> +                               goto again;
>> +                       }
>> +                       if (pmu->capabilities & PERF_PMU_CAP_FORWARD_EVENT)
>> +                               goto try_all;
> 
> Wouldn't it be better to use a different error code to indicate
> it's about precise_ip (or forwarding in general)?  Otherwise
> other invalid config might cause the forwarding unnecessarily..

That would make things easier and we might not need this new capability.
Most appropriate error codes seems ENOENT, EOPNOTSUPP and EINVAL but all
are already used for other purposes. Any other suggestions?

Thanks,
Ravi

Re: [PATCH 2/2] perf test: Add selftest to test IBS invocation via core pmu events

[Ravi Bangoria]

Hi Namhyung,

>> +static int test__ibs_via_core_pmu(struct test_suite *text __maybe_unused,
>> +                                 int subtest __maybe_unused)
>> +{
>> +       struct perf_pmu *ibs_pmu;
>> +       int ret = TEST_OK;
>> +       int fd, i;
>> +
>> +       if (list_empty(&pmus))
>> +               perf_pmu__scan(NULL);
>> +
>> +       ibs_pmu = perf_pmu__find("ibs_op");
>> +       if (!ibs_pmu)
>> +               return TEST_SKIP;
>> +
>> +       for (i = 0; i < NR_SUB_TESTS; i++) {
>> +               fd = event_open(sub_tests[i].type, sub_tests[i].config);
>> +               pr_debug("type: 0x%x, config: 0x%lx, fd: %d  -  ", sub_tests[i].type,
>> +                        sub_tests[i].config, fd);
>> +               if ((sub_tests[i].valid && fd == -1) ||
>> +                   (!sub_tests[i].valid && fd > 0)) {
>> +                       pr_debug("Fail\n");
>> +                       ret = TEST_FAIL;
> 
> You need to close(fd) when it's succeeded for invalid one.

Yup. Will fix it.

Thanks,
Ravi

Re: [PATCH 1/2] perf/ibs: Fix interface via core pmu events

[Namhyung Kim]

On Thu, Mar 2, 2023 at 9:54 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Namhyung,
>
> >> diff --git a/kernel/events/core.c b/kernel/events/core.c
> >> index a5a51dfdd622..c3f59d937280 100644
> >> --- a/kernel/events/core.c
> >> +++ b/kernel/events/core.c
> >> @@ -11633,9 +11633,13 @@ static struct pmu *perf_init_event(struct perf_event *event)
> >>                         goto fail;
> >>
> >>                 ret = perf_try_init_event(pmu, event);
> >> -               if (ret == -ENOENT && event->attr.type != type && !extended_type) {
> >> -                       type = event->attr.type;
> >> -                       goto again;
> >> +               if (ret == -ENOENT) {
> >> +                       if (event->attr.type != type && !extended_type) {
> >> +                               type = event->attr.type;
> >> +                               goto again;
> >> +                       }
> >> +                       if (pmu->capabilities & PERF_PMU_CAP_FORWARD_EVENT)
> >> +                               goto try_all;
> >
> > Wouldn't it be better to use a different error code to indicate
> > it's about precise_ip (or forwarding in general)?  Otherwise
> > other invalid config might cause the forwarding unnecessarily..
>
> That would make things easier and we might not need this new capability.
> Most appropriate error codes seems ENOENT, EOPNOTSUPP and EINVAL but all
> are already used for other purposes. Any other suggestions?

Maybe we can have more liberty for the error code since
it's not returned to the user.  How about ESRCH, EIO or ENXIO?

Thanks,
Namhyung

Re: [PATCH 1/2] perf/ibs: Fix interface via core pmu events

[Ravi Bangoria]

On 07-Mar-23 3:59 AM, Namhyung Kim wrote:
> On Thu, Mar 2, 2023 at 9:54 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>
>> Hi Namhyung,
>>
>>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>>> index a5a51dfdd622..c3f59d937280 100644
>>>> --- a/kernel/events/core.c
>>>> +++ b/kernel/events/core.c
>>>> @@ -11633,9 +11633,13 @@ static struct pmu *perf_init_event(struct perf_event *event)
>>>>                         goto fail;
>>>>
>>>>                 ret = perf_try_init_event(pmu, event);
>>>> -               if (ret == -ENOENT && event->attr.type != type && !extended_type) {
>>>> -                       type = event->attr.type;
>>>> -                       goto again;
>>>> +               if (ret == -ENOENT) {
>>>> +                       if (event->attr.type != type && !extended_type) {
>>>> +                               type = event->attr.type;
>>>> +                               goto again;
>>>> +                       }
>>>> +                       if (pmu->capabilities & PERF_PMU_CAP_FORWARD_EVENT)
>>>> +                               goto try_all;
>>>
>>> Wouldn't it be better to use a different error code to indicate
>>> it's about precise_ip (or forwarding in general)?  Otherwise
>>> other invalid config might cause the forwarding unnecessarily..
>>
>> That would make things easier and we might not need this new capability.
>> Most appropriate error codes seems ENOENT, EOPNOTSUPP and EINVAL but all
>> are already used for other purposes. Any other suggestions?
> 
> Maybe we can have more liberty for the error code since
> it's not returned to the user.  How about ESRCH, EIO or ENXIO?

Ok. We can probably use ESRCH, although it's meaning is little different:

  $ errno -l | grep ESRCH
  ESRCH 3 No such process

Thanks,
Ravi