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

[PATCH v2 0/3] 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 does pre-
req changes, 2nd patch fixes the issue and 3rd patch adds simple perf
test for the same.

v1: https://lore.kernel.org/r/20230302092109.367-1-ravi.bangoria@amd.com
v1->v2:
 - Instead of using pmu capability, use error code -ESRCH to forward
   the event to different pmu.
 - Rebase on v6.3-rc1

Ravi Bangoria (3):
  perf/ibs: Introduce ibs_core_pmu_event()
  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          | 11 +++--
 arch/x86/events/amd/ibs.c           | 52 +++++++++++----------
 arch/x86/include/asm/perf_event.h   |  2 +
 kernel/events/core.c                | 10 ++++-
 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 +
 8 files changed, 121 insertions(+), 27 deletions(-)
 create mode 100644 tools/perf/tests/ibs-via-core-pmu.c

-- 
2.39.2

[PATCH v2 1/3] perf/ibs: Introduce ibs_core_pmu_event()

[Ravi Bangoria]

Split core pmu event comparing code from perf_ibs_precise_event() into
a separate function ibs_core_pmu_event().

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c         | 52 +++++++++++++++++--------------
 arch/x86/include/asm/perf_event.h |  2 ++
 2 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 64582954b5f6..67eb5b7564e8 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -218,29 +218,7 @@ static int perf_ibs_precise_event(struct perf_event *event, u64 *config)
 		return -EOPNOTSUPP;
 	}
 
-	switch (event->attr.type) {
-	case PERF_TYPE_HARDWARE:
-		switch (event->attr.config) {
-		case PERF_COUNT_HW_CPU_CYCLES:
-			*config = 0;
-			return 0;
-		}
-		break;
-	case PERF_TYPE_RAW:
-		switch (event->attr.config) {
-		case 0x0076:
-			*config = 0;
-			return 0;
-		case 0x00C1:
-			*config = IBS_OP_CNT_CTL;
-			return 0;
-		}
-		break;
-	default:
-		return -ENOENT;
-	}
-
-	return -EOPNOTSUPP;
+	return ibs_core_pmu_event(event, config);
 }
 
 static int perf_ibs_init(struct perf_event *event)
@@ -1296,6 +1274,34 @@ u32 get_ibs_caps(void)
 
 EXPORT_SYMBOL(get_ibs_caps);
 
+/* Convert core pmu event into IBS event */
+int ibs_core_pmu_event(struct perf_event *event, u64 *config)
+{
+	switch (event->attr.type) {
+	case PERF_TYPE_HARDWARE:
+		switch (event->attr.config) {
+		case PERF_COUNT_HW_CPU_CYCLES:
+			*config = 0;
+			return 0;
+		}
+		break;
+	case PERF_TYPE_RAW:
+		switch (event->attr.config) {
+		case 0x0076:
+			*config = 0;
+			return 0;
+		case 0x00C1:
+			*config = IBS_OP_CNT_CTL;
+			return 0;
+		}
+		break;
+	default:
+		return -ENOENT;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static inline int get_eilvt(int offset)
 {
 	return !setup_APIC_eilvt(offset, 0, APIC_EILVT_MSG_NMI, 1);
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 8fc15ed5e60b..1ad9d504ae71 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -475,8 +475,10 @@ struct pebs_xmm {
 
 #ifdef CONFIG_X86_LOCAL_APIC
 extern u32 get_ibs_caps(void);
+extern int ibs_core_pmu_event(struct perf_event *event, u64 *config);
 #else
 static inline u32 get_ibs_caps(void) { return 0; }
+static inline int ibs_core_pmu_event(struct perf_event *event, u64 *config) { return -ENOENT; }
 #endif
 
 #ifdef CONFIG_PERF_EVENTS
-- 
2.39.2

[PATCH v2 2/3] 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 trying to open event on all pmus if event_init() on user
requested pmu returns -ESRCH.

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) ]

Note that there is no notion of forward pmu mapping. i.e. kernel doesn't
know which specific pmu(or a 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 starts returning -ESRCH, 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 | 11 ++++++++---
 kernel/events/core.c       | 10 +++++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index 8c45b198b62f..81d67b899371 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -371,10 +371,15 @@ static inline int amd_has_nb(struct cpu_hw_events *cpuc)
 static int amd_pmu_hw_config(struct perf_event *event)
 {
 	int ret;
+	u64 dummy;
 
-	/* pass precise event sampling to ibs: */
-	if (event->attr.precise_ip && get_ibs_caps())
-		return -ENOENT;
+	if (event->attr.precise_ip) {
+		/* pass precise event sampling to ibs by returning -ESRCH */
+		if (get_ibs_caps() && !ibs_core_pmu_event(event, &dummy))
+			return -ESRCH;
+		else
+			return -ENOENT;
+	}
 
 	if (has_branch_stack(event) && !x86_pmu.lbr_nr)
 		return -EOPNOTSUPP;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index f79fd8b87f75..e990c71ba34a 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11639,18 +11639,26 @@ static struct pmu *perf_init_event(struct perf_event *event)
 			goto again;
 		}
 
+		/*
+		 * pmu->event_init() should return -ESRCH only when it
+		 * wants to forward the event to other pmu.
+		 */
+		if (ret == -ESRCH)
+			goto try_all;
+
 		if (ret)
 			pmu = ERR_PTR(ret);
 
 		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)
 			goto unlock;
 
-		if (ret != -ENOENT) {
+		if (ret != -ENOENT && ret != -ESRCH) {
 			pmu = ERR_PTR(ret);
 			goto unlock;
 		}
-- 
2.39.2

[PATCH v2 3/3] 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 a 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 v2 2/3] perf/ibs: Fix interface via core pmu events

[Namhyung Kim]

On Thu, Mar 9, 2023 at 2:12 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 trying to open event on all pmus if event_init() on user
> requested pmu returns -ESRCH.
>
> 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) ]
>
> Note that there is no notion of forward pmu mapping. i.e. kernel doesn't
> know which specific pmu(or a 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 starts returning -ESRCH, 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 | 11 ++++++++---
>  kernel/events/core.c       | 10 +++++++++-
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 8c45b198b62f..81d67b899371 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -371,10 +371,15 @@ static inline int amd_has_nb(struct cpu_hw_events *cpuc)
>  static int amd_pmu_hw_config(struct perf_event *event)
>  {
>         int ret;
> +       u64 dummy;
>
> -       /* pass precise event sampling to ibs: */
> -       if (event->attr.precise_ip && get_ibs_caps())
> -               return -ENOENT;
> +       if (event->attr.precise_ip) {
> +               /* pass precise event sampling to ibs by returning -ESRCH */
> +               if (get_ibs_caps() && !ibs_core_pmu_event(event, &dummy))
> +                       return -ESRCH;
> +               else
> +                       return -ENOENT;
> +       }
>
>         if (has_branch_stack(event) && !x86_pmu.lbr_nr)
>                 return -EOPNOTSUPP;
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f79fd8b87f75..e990c71ba34a 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11639,18 +11639,26 @@ static struct pmu *perf_init_event(struct perf_event *event)
>                         goto again;
>                 }
>
> +               /*
> +                * pmu->event_init() should return -ESRCH only when it
> +                * wants to forward the event to other pmu.
> +                */

Can we add this to the comment in the struct pmu?  There is a
description already for other error codes.

Otherwise looks good.

Thanks,
Namhyung


> +               if (ret == -ESRCH)
> +                       goto try_all;
> +
>                 if (ret)
>                         pmu = ERR_PTR(ret);
>
>                 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)
>                         goto unlock;
>
> -               if (ret != -ENOENT) {
> +               if (ret != -ENOENT && ret != -ESRCH) {
>                         pmu = ERR_PTR(ret);
>                         goto unlock;
>                 }
> --
> 2.39.2
>

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

[Peter Zijlstra]

On Thu, Mar 09, 2023 at 03:41:10PM +0530, Ravi Bangoria wrote:
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 8c45b198b62f..81d67b899371 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -371,10 +371,15 @@ static inline int amd_has_nb(struct cpu_hw_events *cpuc)
>  static int amd_pmu_hw_config(struct perf_event *event)
>  {
>  	int ret;
> +	u64 dummy;
>  
> -	/* pass precise event sampling to ibs: */
> -	if (event->attr.precise_ip && get_ibs_caps())
> -		return -ENOENT;
> +	if (event->attr.precise_ip) {
> +		/* pass precise event sampling to ibs by returning -ESRCH */
> +		if (get_ibs_caps() && !ibs_core_pmu_event(event, &dummy))
> +			return -ESRCH;
> +		else
> +			return -ENOENT;
> +	}
>  
>  	if (has_branch_stack(event) && !x86_pmu.lbr_nr)
>  		return -EOPNOTSUPP;
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f79fd8b87f75..e990c71ba34a 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11639,18 +11639,26 @@ static struct pmu *perf_init_event(struct perf_event *event)
>  			goto again;
>  		}
>  
> +		/*
> +		 * pmu->event_init() should return -ESRCH only when it
> +		 * wants to forward the event to other pmu.
> +		 */
> +		if (ret == -ESRCH)
> +			goto try_all;
> +
>  		if (ret)
>  			pmu = ERR_PTR(ret);
>  
>  		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)
>  			goto unlock;
>  
> -		if (ret != -ENOENT) {
> +		if (ret != -ENOENT && ret != -ESRCH) {
>  			pmu = ERR_PTR(ret);
>  			goto unlock;
>  		}

Urgh.. So amd_pmu_hw_config() knows what PMU it should be but has no
real way to communicate this, so you make it try all of them again.

Now, we already have a gruesome hack in there, and I'm thikning you
should use that instead of adding yet another one. Note:

		if (ret == -ENOENT && event->attr.type != type && !extended_type) {
			type = event->attr.type;
			goto again;

So if you have amd_pmu_hw_config() do:

	event->attr.type = ibs_pmu.type;
	return -ENOENT;

it should all just work no?

And now thinking about this, I'm thinking we can clean up the whole
swevent mess too, a little something like the below perhaps... Then it
might just be possible to remove that list_for_each_entry_rcu()
entirely.

Hmm?


---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index f79fd8b87f75..26130d1ca40b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9951,6 +9951,9 @@ static void sw_perf_event_destroy(struct perf_event *event)
 	swevent_hlist_put();
 }
 
+static struct pmu perf_cpu_clock; /* fwd declaration */
+static struct pmu perf_task_clock;
+
 static int perf_swevent_init(struct perf_event *event)
 {
 	u64 event_id = event->attr.config;
@@ -9966,7 +9969,11 @@ static int perf_swevent_init(struct perf_event *event)
 
 	switch (event_id) {
 	case PERF_COUNT_SW_CPU_CLOCK:
+		event->attr.type = perf_cpu_clock.type;
+		return -ENOENT;
+
 	case PERF_COUNT_SW_TASK_CLOCK:
+		event->attr.type = perf_task_clock.type;
 		return -ENOENT;
 
 	default:

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

[Ravi Bangoria]

Hi Peter,

On 12-Mar-23 8:24 PM, Peter Zijlstra wrote:
> On Thu, Mar 09, 2023 at 03:41:10PM +0530, Ravi Bangoria wrote:
>> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
>> index 8c45b198b62f..81d67b899371 100644
>> --- a/arch/x86/events/amd/core.c
>> +++ b/arch/x86/events/amd/core.c
>> @@ -371,10 +371,15 @@ static inline int amd_has_nb(struct cpu_hw_events *cpuc)
>>  static int amd_pmu_hw_config(struct perf_event *event)
>>  {
>>  	int ret;
>> +	u64 dummy;
>>  
>> -	/* pass precise event sampling to ibs: */
>> -	if (event->attr.precise_ip && get_ibs_caps())
>> -		return -ENOENT;
>> +	if (event->attr.precise_ip) {
>> +		/* pass precise event sampling to ibs by returning -ESRCH */
>> +		if (get_ibs_caps() && !ibs_core_pmu_event(event, &dummy))
>> +			return -ESRCH;
>> +		else
>> +			return -ENOENT;
>> +	}
>>  
>>  	if (has_branch_stack(event) && !x86_pmu.lbr_nr)
>>  		return -EOPNOTSUPP;
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index f79fd8b87f75..e990c71ba34a 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -11639,18 +11639,26 @@ static struct pmu *perf_init_event(struct perf_event *event)
>>  			goto again;
>>  		}
>>  
>> +		/*
>> +		 * pmu->event_init() should return -ESRCH only when it
>> +		 * wants to forward the event to other pmu.
>> +		 */
>> +		if (ret == -ESRCH)
>> +			goto try_all;
>> +
>>  		if (ret)
>>  			pmu = ERR_PTR(ret);
>>  
>>  		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)
>>  			goto unlock;
>>  
>> -		if (ret != -ENOENT) {
>> +		if (ret != -ENOENT && ret != -ESRCH) {
>>  			pmu = ERR_PTR(ret);
>>  			goto unlock;
>>  		}
> 
> Urgh.. So amd_pmu_hw_config() knows what PMU it should be but has no
> real way to communicate this, so you make it try all of them again.
> 
> Now, we already have a gruesome hack in there, and I'm thikning you
> should use that instead of adding yet another one. Note:
> 
> 		if (ret == -ENOENT && event->attr.type != type && !extended_type) {
> 			type = event->attr.type;
> 			goto again;
> 
> So if you have amd_pmu_hw_config() do:
> 
> 	event->attr.type = ibs_pmu.type;
> 	return -ENOENT;
> 
> it should all just work no?

IBS driver needs to convert RAW pmu config to IBS config, which it does
based on original event->attr.type. See perf_ibs_precise_event(). This
logic will fail with event->attr.type overwrite.

> 
> And now thinking about this, I'm thinking we can clean up the whole
> swevent mess too, a little something like the below perhaps... Then it
> might just be possible to remove that list_for_each_entry_rcu()
> entirely.
> 
> Hmm?

I'll check this and revert back.

> 
> 
> ---
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f79fd8b87f75..26130d1ca40b 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9951,6 +9951,9 @@ static void sw_perf_event_destroy(struct perf_event *event)
>  	swevent_hlist_put();
>  }
>  
> +static struct pmu perf_cpu_clock; /* fwd declaration */
> +static struct pmu perf_task_clock;
> +
>  static int perf_swevent_init(struct perf_event *event)
>  {
>  	u64 event_id = event->attr.config;
> @@ -9966,7 +9969,11 @@ static int perf_swevent_init(struct perf_event *event)
>  
>  	switch (event_id) {
>  	case PERF_COUNT_SW_CPU_CLOCK:
> +		event->attr.type = perf_cpu_clock.type;
> +		return -ENOENT;
> +
>  	case PERF_COUNT_SW_TASK_CLOCK:
> +		event->attr.type = perf_task_clock.type;
>  		return -ENOENT;
>  
>  	default:

Thanks,
Ravi

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

[Ravi Bangoria]

>> +               /*
>> +                * pmu->event_init() should return -ESRCH only when it
>> +                * wants to forward the event to other pmu.
>> +                */
> 
> Can we add this to the comment in the struct pmu?  There is a
> description already for other error codes.

Sure. I'll update there if we continue with this approach.

> 
> Otherwise looks good.

Thanks,
Ravi

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

[Peter Zijlstra]

On Mon, Mar 13, 2023 at 05:59:46PM +0530, Ravi Bangoria wrote:

> > Now, we already have a gruesome hack in there, and I'm thikning you
> > should use that instead of adding yet another one. Note:
> > 
> > 		if (ret == -ENOENT && event->attr.type != type && !extended_type) {
> > 			type = event->attr.type;
> > 			goto again;
> > 
> > So if you have amd_pmu_hw_config() do:
> > 
> > 	event->attr.type = ibs_pmu.type;
> > 	return -ENOENT;
> > 
> > it should all just work no?
> 
> IBS driver needs to convert RAW pmu config to IBS config, which it does
> based on original event->attr.type. See perf_ibs_precise_event(). This
> logic will fail with event->attr.type overwrite.

amd_pmu_hw_config() could also rewrite event->attr.config I suppose.

I don't think we actually use/expose these event->attr fields again
after all this, do wel?

The closest to that is perf_event_modify_attr(), but that is limited to
TYPE_BREAKPOINT for the time being (also, could this be used to cure
your in-kernel IBS usage woes?).

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

[Ravi Bangoria]

On 13-Mar-23 7:51 PM, Peter Zijlstra wrote:
> On Mon, Mar 13, 2023 at 05:59:46PM +0530, Ravi Bangoria wrote:
> 
>>> Now, we already have a gruesome hack in there, and I'm thikning you
>>> should use that instead of adding yet another one. Note:
>>>
>>> 		if (ret == -ENOENT && event->attr.type != type && !extended_type) {
>>> 			type = event->attr.type;
>>> 			goto again;
>>>
>>> So if you have amd_pmu_hw_config() do:
>>>
>>> 	event->attr.type = ibs_pmu.type;
>>> 	return -ENOENT;
>>>
>>> it should all just work no?
>>
>> IBS driver needs to convert RAW pmu config to IBS config, which it does
>> based on original event->attr.type. See perf_ibs_precise_event(). This
>> logic will fail with event->attr.type overwrite.
> 
> amd_pmu_hw_config() could also rewrite event->attr.config I suppose.

This might work. Let me try.

> 
> I don't think we actually use/expose these event->attr fields again
> after all this, do wel?

I'll confirm this as well.

> 
> The closest to that is perf_event_modify_attr(), but that is limited to
> TYPE_BREAKPOINT for the time being (also, could this be used to cure
> your in-kernel IBS usage woes?).

I think doing it transparently at the arch layer would be better.

Thanks,
Ravi