[PATCH v3 0/3] perf: Rework event forwarding logic

[PATCH v3 0/3] perf: Rework event forwarding logic

[Ravi Bangoria]

Usually events are opened on the same pmu as requested by user via
perf_event_attr->type argument. But certain special events are
internally redirected to different pmu. Currently such pmus needs to
be treated specially and thus requires some gruesome hacks.

An approach, suggested by Peter Zijlstra, to get rid of these hacks
was to overwrite event attributes with new pmu's attribute values
within the kernel and let perf_event_init() retry opening the event
with overwritten values. This patch series implements it.

v2: https://lore.kernel.org/r/20230309101111.444-1-ravi.bangoria@amd.com
v2->v3:
 - Both uses different approach to solve the problem. v2 was using new
   error value -ESRCH to let perf_event_init() try opening event with
   all pmus, hoping that the event will get forwarded to right pmu.
   OTOH, v3 uses different approach as described in the description.
 - v3 also fixes SW pmu to task-clock/cpu-clock pmu event forwarding.

Patches are prepared on v6.3.

Ravi Bangoria (3):
  perf/core: Rework forwarding of {task|cpu}-clock events
  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              |  2 +-
 arch/x86/events/amd/ibs.c               | 53 ++++++++---------
 arch/x86/include/asm/perf_event.h       |  2 +
 include/linux/perf_event.h              | 11 ++++
 kernel/events/core.c                    | 79 +++++++++++--------------
 tools/perf/tests/Build                  |  1 +
 tools/perf/tests/amd-ibs-via-core-pmu.c | 72 ++++++++++++++++++++++
 tools/perf/tests/builtin-test.c         |  1 +
 tools/perf/tests/tests.h                |  1 +
 9 files changed, 151 insertions(+), 71 deletions(-)
 create mode 100644 tools/perf/tests/amd-ibs-via-core-pmu.c

-- 
2.40.0

[PATCH v3 1/3] perf/core: Rework forwarding of {task|cpu}-clock events

[Ravi Bangoria]

Currently, PERF_TYPE_SOFTWARE is treated specially since task-clock and
cpu-clock events are interfaced through it but internally gets forwarded
to their own pmus.

Rework this by overwriting event->attr.type in perf_swevent_init() which
will cause perf_init_event() to retry with updated type and event will
automatically get forwarded to right pmu. With the change, SW pmu no
longer needs to be treated specially and can be included in 'pmu_idr'
list.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 include/linux/perf_event.h | 11 ++++++
 kernel/events/core.c       | 69 ++++++++++++++++++++------------------
 2 files changed, 47 insertions(+), 33 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index d5628a7b5eaa..40647d707fb3 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -322,6 +322,9 @@ struct pmu {
 	/* number of address filters this PMU can do */
 	unsigned int			nr_addr_filters;
 
+	/* Skip creating pmu device and sysfs interface. */
+	bool				skip_sysfs_dev;
+
 	/*
 	 * Fully disable/enable this PMU, can be used to protect from the PMI
 	 * as well as for lazy/batch writing of the MSRs.
@@ -827,6 +830,14 @@ struct perf_event {
 	void *security;
 #endif
 	struct list_head		sb_list;
+
+	/*
+	 * Certain events gets forwarded to another pmu internally by over-
+	 * writing kernel copy of event->attr.type without user being aware
+	 * of it. event->orig_type contains original 'type' requested by
+	 * user.
+	 */
+	__u32				orig_type;
 #endif /* CONFIG_PERF_EVENTS */
 };
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 435815d3be3f..151299940d9a 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6647,7 +6647,7 @@ static void perf_sigtrap(struct perf_event *event)
 		return;
 
 	send_sig_perf((void __user *)event->pending_addr,
-		      event->attr.type, event->attr.sig_data);
+		      event->orig_type, event->attr.sig_data);
 }
 
 /*
@@ -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,10 @@ 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:
@@ -11086,7 +11092,7 @@ static void cpu_clock_event_read(struct perf_event *event)
 
 static int cpu_clock_event_init(struct perf_event *event)
 {
-	if (event->attr.type != PERF_TYPE_SOFTWARE)
+	if (event->attr.type != perf_cpu_clock.type)
 		return -ENOENT;
 
 	if (event->attr.config != PERF_COUNT_SW_CPU_CLOCK)
@@ -11107,6 +11113,7 @@ static struct pmu perf_cpu_clock = {
 	.task_ctx_nr	= perf_sw_context,
 
 	.capabilities	= PERF_PMU_CAP_NO_NMI,
+	.skip_sysfs_dev	= true,
 
 	.event_init	= cpu_clock_event_init,
 	.add		= cpu_clock_event_add,
@@ -11167,7 +11174,7 @@ static void task_clock_event_read(struct perf_event *event)
 
 static int task_clock_event_init(struct perf_event *event)
 {
-	if (event->attr.type != PERF_TYPE_SOFTWARE)
+	if (event->attr.type != perf_task_clock.type)
 		return -ENOENT;
 
 	if (event->attr.config != PERF_COUNT_SW_TASK_CLOCK)
@@ -11188,6 +11195,7 @@ static struct pmu perf_task_clock = {
 	.task_ctx_nr	= perf_sw_context,
 
 	.capabilities	= PERF_PMU_CAP_NO_NMI,
+	.skip_sysfs_dev	= true,
 
 	.event_init	= task_clock_event_init,
 	.add		= task_clock_event_add,
@@ -11415,31 +11423,31 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type)
 		goto unlock;
 
 	pmu->type = -1;
-	if (!name)
-		goto skip_type;
+	if (WARN_ONCE(!name, "Can not register anonymous pmu.\n")) {
+		ret = -EINVAL;
+		goto free_pdc;
+	}
+
 	pmu->name = name;
 
-	if (type != PERF_TYPE_SOFTWARE) {
-		if (type >= 0)
-			max = type;
+	if (type >= 0)
+		max = type;
 
-		ret = idr_alloc(&pmu_idr, pmu, max, 0, GFP_KERNEL);
-		if (ret < 0)
-			goto free_pdc;
+	ret = idr_alloc(&pmu_idr, pmu, max, 0, GFP_KERNEL);
+	if (ret < 0)
+		goto free_pdc;
 
-		WARN_ON(type >= 0 && ret != type);
+	WARN_ON(type >= 0 && ret != type);
 
-		type = ret;
-	}
+	type = ret;
 	pmu->type = type;
 
-	if (pmu_bus_running) {
+	if (pmu_bus_running && !pmu->skip_sysfs_dev) {
 		ret = pmu_dev_alloc(pmu);
 		if (ret)
 			goto free_idr;
 	}
 
-skip_type:
 	ret = -ENOMEM;
 	pmu->cpu_pmu_context = alloc_percpu(struct perf_cpu_pmu_context);
 	if (!pmu->cpu_pmu_context)
@@ -11481,16 +11489,7 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type)
 	if (!pmu->event_idx)
 		pmu->event_idx = perf_event_idx_default;
 
-	/*
-	 * Ensure the TYPE_SOFTWARE PMUs are at the head of the list,
-	 * since these cannot be in the IDR. This way the linear search
-	 * is fast, provided a valid software event is provided.
-	 */
-	if (type == PERF_TYPE_SOFTWARE || !name)
-		list_add_rcu(&pmu->entry, &pmus);
-	else
-		list_add_tail_rcu(&pmu->entry, &pmus);
-
+	list_add_rcu(&pmu->entry, &pmus);
 	atomic_set(&pmu->exclusive_cnt, 0);
 	ret = 0;
 unlock:
@@ -11503,8 +11502,7 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type)
 	put_device(pmu->dev);
 
 free_idr:
-	if (pmu->type != PERF_TYPE_SOFTWARE)
-		idr_remove(&pmu_idr, pmu->type);
+	idr_remove(&pmu_idr, pmu->type);
 
 free_pdc:
 	free_percpu(pmu->pmu_disable_count);
@@ -11525,8 +11523,7 @@ void perf_pmu_unregister(struct pmu *pmu)
 	synchronize_rcu();
 
 	free_percpu(pmu->pmu_disable_count);
-	if (pmu->type != PERF_TYPE_SOFTWARE)
-		idr_remove(&pmu_idr, pmu->type);
+	idr_remove(&pmu_idr, pmu->type);
 	if (pmu_bus_running) {
 		if (pmu->nr_addr_filters)
 			device_remove_file(pmu->dev, &dev_attr_nr_addr_filters);
@@ -11601,6 +11598,12 @@ static struct pmu *perf_init_event(struct perf_event *event)
 
 	idx = srcu_read_lock(&pmus_srcu);
 
+	/*
+	 * Save original type before calling pmu->event_init() since certain
+	 * pmus overwrites event->attr.type to forward event to another pmu.
+	 */
+	event->orig_type = event->attr.type;
+
 	/* Try parent's PMU first: */
 	if (event->parent && event->parent->pmu) {
 		pmu = event->parent->pmu;
@@ -13640,8 +13643,8 @@ void __init perf_event_init(void)
 	perf_event_init_all_cpus();
 	init_srcu_struct(&pmus_srcu);
 	perf_pmu_register(&perf_swevent, "software", PERF_TYPE_SOFTWARE);
-	perf_pmu_register(&perf_cpu_clock, NULL, -1);
-	perf_pmu_register(&perf_task_clock, NULL, -1);
+	perf_pmu_register(&perf_cpu_clock, "cpu_clock", -1);
+	perf_pmu_register(&perf_task_clock, "task_clock", -1);
 	perf_tp_register();
 	perf_event_init_cpu(smp_processor_id());
 	register_reboot_notifier(&perf_reboot_notifier);
@@ -13684,7 +13687,7 @@ static int __init perf_event_sysfs_init(void)
 		goto unlock;
 
 	list_for_each_entry(pmu, &pmus, entry) {
-		if (!pmu->name || pmu->type < 0)
+		if (pmu->skip_sysfs_dev)
 			continue;
 
 		ret = pmu_dev_alloc(pmu);
-- 
2.40.0

Re: [PATCH v3 1/3] perf/core: Rework forwarding of {task|cpu}-clock events

[Peter Zijlstra]

On Tue, Apr 25, 2023 at 07:52:03PM +0530, Ravi Bangoria wrote:
> Currently, PERF_TYPE_SOFTWARE is treated specially since task-clock and
> cpu-clock events are interfaced through it but internally gets forwarded
> to their own pmus.
> 
> Rework this by overwriting event->attr.type in perf_swevent_init() which
> will cause perf_init_event() to retry with updated type and event will
> automatically get forwarded to right pmu. With the change, SW pmu no
> longer needs to be treated specially and can be included in 'pmu_idr'
> list.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  include/linux/perf_event.h | 11 ++++++
>  kernel/events/core.c       | 69 ++++++++++++++++++++------------------
>  2 files changed, 47 insertions(+), 33 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index d5628a7b5eaa..40647d707fb3 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -322,6 +322,9 @@ struct pmu {
>  	/* number of address filters this PMU can do */
>  	unsigned int			nr_addr_filters;
>  
> +	/* Skip creating pmu device and sysfs interface. */
> +	bool				skip_sysfs_dev;
> +
>  	/*
>  	 * Fully disable/enable this PMU, can be used to protect from the PMI
>  	 * as well as for lazy/batch writing of the MSRs.

Does this make sense?

---
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -295,6 +295,8 @@ struct perf_event_pmu_context;
 
 struct perf_output_handle;
 
+#define PMU_NULL_DEV	((void *)(~0))
+
 /**
  * struct pmu - generic performance monitoring unit
  */
@@ -322,9 +324,6 @@ struct pmu {
 	/* number of address filters this PMU can do */
 	unsigned int			nr_addr_filters;
 
-	/* Skip creating pmu device and sysfs interface. */
-	bool				skip_sysfs_dev;
-
 	/*
 	 * Fully disable/enable this PMU, can be used to protect from the PMI
 	 * as well as for lazy/batch writing of the MSRs.
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11113,7 +11113,7 @@ static struct pmu perf_cpu_clock = {
 	.task_ctx_nr	= perf_sw_context,
 
 	.capabilities	= PERF_PMU_CAP_NO_NMI,
-	.skip_sysfs_dev	= true,
+	.dev		= PMU_NULL_DEV,
 
 	.event_init	= cpu_clock_event_init,
 	.add		= cpu_clock_event_add,
@@ -11195,7 +11195,7 @@ static struct pmu perf_task_clock = {
 	.task_ctx_nr	= perf_sw_context,
 
 	.capabilities	= PERF_PMU_CAP_NO_NMI,
-	.skip_sysfs_dev	= true,
+	.dev		= PMU_NULL_DEV,
 
 	.event_init	= task_clock_event_init,
 	.add		= task_clock_event_add,
@@ -11442,7 +11442,7 @@ int perf_pmu_register(struct pmu *pmu, c
 	type = ret;
 	pmu->type = type;
 
-	if (pmu_bus_running && !pmu->skip_sysfs_dev) {
+	if (pmu_bus_running && !pmu->dev) {
 		ret = pmu_dev_alloc(pmu);
 		if (ret)
 			goto free_idr;
@@ -11524,7 +11524,7 @@ void perf_pmu_unregister(struct pmu *pmu
 
 	free_percpu(pmu->pmu_disable_count);
 	idr_remove(&pmu_idr, pmu->type);
-	if (pmu_bus_running) {
+	if (pmu_bus_running && pmu->dev != PMU_NULL_DEV) {
 		if (pmu->nr_addr_filters)
 			device_remove_file(pmu->dev, &dev_attr_nr_addr_filters);
 		device_del(pmu->dev);
@@ -13687,7 +13687,7 @@ static int __init perf_event_sysfs_init(
 		goto unlock;
 
 	list_for_each_entry(pmu, &pmus, entry) {
-		if (pmu->skip_sysfs_dev)
+		if (pmu->dev)
 			continue;
 
 		ret = pmu_dev_alloc(pmu);

Re: [PATCH v3 1/3] perf/core: Rework forwarding of {task|cpu}-clock events

[Ravi Bangoria]

>> @@ -322,6 +322,9 @@ struct pmu {
>>  	/* number of address filters this PMU can do */
>>  	unsigned int			nr_addr_filters;
>>  
>> +	/* Skip creating pmu device and sysfs interface. */
>> +	bool				skip_sysfs_dev;
>> +
>>  	/*
>>  	 * Fully disable/enable this PMU, can be used to protect from the PMI
>>  	 * as well as for lazy/batch writing of the MSRs.
> 
> Does this make sense?

Yes! Will respin with this included.

Thanks,
Ravi

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

[Ravi Bangoria]

Although, IBS pmus can be invoked via their own interface, indirect
IBS invocation via core pmu events 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.

Forward precise events from core pmu to IBS by overwriting 'type' and
'config' in the kernel copy of perf_event_attr. Overwriting will cause
perf_init_event() to retry with updated 'type' and 'config', which will
automatically forward event to IBS pmu.

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

Searching for the right pmu by iterating over all pmus is no longer
required since all pmus now *must* be present in the 'pmu_idr' list.
So, remove linear searching code.

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        |  2 +-
 arch/x86/events/amd/ibs.c         | 53 +++++++++++++++----------------
 arch/x86/include/asm/perf_event.h |  2 ++
 kernel/events/core.c              | 10 ------
 4 files changed, 29 insertions(+), 38 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index bccea57dee81..abadd5f23425 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -374,7 +374,7 @@ static int amd_pmu_hw_config(struct perf_event *event)
 
 	/* pass precise event sampling to ibs: */
 	if (event->attr.precise_ip && get_ibs_caps())
-		return -ENOENT;
+		return forward_event_to_ibs(event);
 
 	if (has_branch_stack(event) && !x86_pmu.lbr_nr)
 		return -EOPNOTSUPP;
diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 64582954b5f6..371014802191 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -190,7 +190,7 @@ static struct perf_ibs *get_ibs_pmu(int type)
 }
 
 /*
- * Use IBS for precise event sampling:
+ * core pmu config -> IBS config
  *
  *  perf record -a -e cpu-cycles:p ...    # use ibs op counting cycle count
  *  perf record -a -e r076:p ...          # same as -e cpu-cycles:p
@@ -199,25 +199,9 @@ static struct perf_ibs *get_ibs_pmu(int type)
  * IbsOpCntCtl (bit 19) of IBS Execution Control Register (IbsOpCtl,
  * MSRC001_1033) is used to select either cycle or micro-ops counting
  * mode.
- *
- * The rip of IBS samples has skid 0. Thus, IBS supports precise
- * levels 1 and 2 and the PERF_EFLAGS_EXACT is set. In rare cases the
- * rip is invalid when IBS was not able to record the rip correctly.
- * We clear PERF_EFLAGS_EXACT and take the rip from pt_regs then.
- *
  */
-static int perf_ibs_precise_event(struct perf_event *event, u64 *config)
+static int core_pmu_ibs_config(struct perf_event *event, u64 *config)
 {
-	switch (event->attr.precise_ip) {
-	case 0:
-		return -ENOENT;
-	case 1:
-	case 2:
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
-
 	switch (event->attr.type) {
 	case PERF_TYPE_HARDWARE:
 		switch (event->attr.config) {
@@ -243,22 +227,37 @@ static int perf_ibs_precise_event(struct perf_event *event, u64 *config)
 	return -EOPNOTSUPP;
 }
 
+/*
+ * The rip of IBS samples has skid 0. Thus, IBS supports precise
+ * levels 1 and 2 and the PERF_EFLAGS_EXACT is set. In rare cases the
+ * rip is invalid when IBS was not able to record the rip correctly.
+ * We clear PERF_EFLAGS_EXACT and take the rip from pt_regs then.
+ */
+int forward_event_to_ibs(struct perf_event *event)
+{
+	u64 config = 0;
+
+	if (!event->attr.precise_ip || event->attr.precise_ip > 2)
+		return -EOPNOTSUPP;
+
+	if (!core_pmu_ibs_config(event, &config)) {
+		event->attr.type = perf_ibs_op.pmu.type;
+		event->attr.config = config;
+	}
+	return -ENOENT;
+}
+
 static int perf_ibs_init(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
 	struct perf_ibs *perf_ibs;
 	u64 max_cnt, config;
-	int ret;
 
 	perf_ibs = get_ibs_pmu(event->attr.type);
-	if (perf_ibs) {
-		config = event->attr.config;
-	} else {
-		perf_ibs = &perf_ibs_op;
-		ret = perf_ibs_precise_event(event, &config);
-		if (ret)
-			return ret;
-	}
+	if (!perf_ibs)
+		return -ENOENT;
+
+	config = event->attr.config;
 
 	if (event->pmu != &perf_ibs->pmu)
 		return -ENOENT;
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 8fc15ed5e60b..fc86248215e2 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 forward_event_to_ibs(struct perf_event *event);
 #else
 static inline u32 get_ibs_caps(void) { return 0; }
+static inline int forward_event_to_ibs(struct perf_event *event) { return -ENOENT; }
 #endif
 
 #ifdef CONFIG_PERF_EVENTS
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 151299940d9a..232121a6d1e2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11648,16 +11648,6 @@ static struct pmu *perf_init_event(struct perf_event *event)
 		goto unlock;
 	}
 
-	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) {
-			pmu = ERR_PTR(ret);
-			goto unlock;
-		}
-	}
 fail:
 	pmu = ERR_PTR(-ENOENT);
 unlock:
-- 
2.40.0

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

[Peter Zijlstra]

On Tue, Apr 25, 2023 at 07:52:04PM +0530, Ravi Bangoria wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 151299940d9a..232121a6d1e2 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11648,16 +11648,6 @@ static struct pmu *perf_init_event(struct perf_event *event)
>  		goto unlock;
>  	}
>  
> -	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) {
> -			pmu = ERR_PTR(ret);
> -			goto unlock;
> -		}
> -	}
>  fail:
>  	pmu = ERR_PTR(-ENOENT);
>  unlock:

Would it make sense to put the above and the below cleanup in a separate
patch after this?

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11631,25 +11631,24 @@ static struct pmu *perf_init_event(struc
 	rcu_read_lock();
 	pmu = idr_find(&pmu_idr, type);
 	rcu_read_unlock();
-	if (pmu) {
-		if (event->attr.type != type && type != PERF_TYPE_RAW &&
-		    !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
-			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 (!pmu)
+		goto unlock;
 
-		if (ret)
-			pmu = ERR_PTR(ret);
+	ret = -ENOENT;
+	if (event->attr.type != type && type != PERF_TYPE_RAW &&
+	    !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
+		goto fail;
 
-		goto unlock;
+	ret = perf_try_init_event(pmu, event);
+	if (ret == -ENOENT && event->attr.type != type && !extended_type) {
+		type = event->attr.type;
+		goto again;
 	}
 
 fail:
-	pmu = ERR_PTR(-ENOENT);
+	if (ret)
+		pmu = ERR_PTR(ret);
+
 unlock:
 	srcu_read_unlock(&pmus_srcu, idx);
 

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

[Ravi Bangoria]

On 02-May-23 9:07 PM, Peter Zijlstra wrote:
> On Tue, Apr 25, 2023 at 07:52:04PM +0530, Ravi Bangoria wrote:
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 151299940d9a..232121a6d1e2 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -11648,16 +11648,6 @@ static struct pmu *perf_init_event(struct perf_event *event)
>>  		goto unlock;
>>  	}
>>  
>> -	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) {
>> -			pmu = ERR_PTR(ret);
>> -			goto unlock;
>> -		}
>> -	}
>>  fail:
>>  	pmu = ERR_PTR(-ENOENT);
>>  unlock:
> 
> Would it make sense to put the above and the below cleanup in a separate
> patch after this?

Sure. I'll cleanup and respin.

Thanks,
Ravi

[PATCH v3 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 with 'precise_ip'
set to 1. Add a simple event open test for all these events.

Without kernel fix:
  $ sudo ./perf test -vv 76
   76: AMD 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 ----
  AMD IBS via core pmu: FAILED!

With kernel fix:
  $ sudo ./perf test -vv 76
   76: AMD 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 ----
  AMD IBS via core pmu: Ok

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

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index fb9ac5dc4079..ff7234653503 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 += amd-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/amd-ibs-via-core-pmu.c b/tools/perf/tests/amd-ibs-via-core-pmu.c
new file mode 100644
index 000000000000..6f6eb2d84fde
--- /dev/null
+++ b/tools/perf/tests/amd-ibs-via-core-pmu.c
@@ -0,0 +1,72 @@
+// 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__amd_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");
+		}
+
+		if (fd > 0)
+			close(fd);
+	}
+
+	return ret;
+}
+
+DEFINE_SUITE("AMD IBS via core pmu", amd_ibs_via_core_pmu);
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 35cc3807cc9e..1805a4fae762 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__amd_ibs_via_core_pmu,
 	NULL,
 };
 
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 9a0f3904e53d..65589d40638d 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(amd_ibs_via_core_pmu);
 
 /*
  * PowerPC and S390 do not support creation of instruction breakpoints using the
-- 
2.40.0

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

[Ian Rogers]

On Tue, Apr 25, 2023 at 7:23 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> IBS pmu can be invoked via fixed set of core pmu events with 'precise_ip'
> set to 1. Add a simple event open test for all these events.
>
> Without kernel fix:
>   $ sudo ./perf test -vv 76
>    76: AMD 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 ----
>   AMD IBS via core pmu: FAILED!
>
> With kernel fix:
>   $ sudo ./perf test -vv 76
>    76: AMD 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 ----
>   AMD IBS via core pmu: Ok
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>

Thanks Ravi, as the test is AMD specific I think it makes sense to place it in:
tools/perf/arch/x86/tests
and then to update the test list in:
tools/perf/arch/x86/tests/arch-tests.c

Thanks,
Ian

> ---
>  tools/perf/tests/Build                  |  1 +
>  tools/perf/tests/amd-ibs-via-core-pmu.c | 72 +++++++++++++++++++++++++
>  tools/perf/tests/builtin-test.c         |  1 +
>  tools/perf/tests/tests.h                |  1 +
>  4 files changed, 75 insertions(+)
>  create mode 100644 tools/perf/tests/amd-ibs-via-core-pmu.c
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index fb9ac5dc4079..ff7234653503 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 += amd-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/amd-ibs-via-core-pmu.c b/tools/perf/tests/amd-ibs-via-core-pmu.c
> new file mode 100644
> index 000000000000..6f6eb2d84fde
> --- /dev/null
> +++ b/tools/perf/tests/amd-ibs-via-core-pmu.c
> @@ -0,0 +1,72 @@
> +// 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__amd_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");
> +               }
> +
> +               if (fd > 0)
> +                       close(fd);
> +       }
> +
> +       return ret;
> +}
> +
> +DEFINE_SUITE("AMD IBS via core pmu", amd_ibs_via_core_pmu);
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 35cc3807cc9e..1805a4fae762 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__amd_ibs_via_core_pmu,
>         NULL,
>  };
>
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 9a0f3904e53d..65589d40638d 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(amd_ibs_via_core_pmu);
>
>  /*
>   * PowerPC and S390 do not support creation of instruction breakpoints using the
> --
> 2.40.0
>

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

[Ravi Bangoria]

On 30-Apr-23 2:39 AM, Ian Rogers wrote:
> On Tue, Apr 25, 2023 at 7:23 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>
>> IBS pmu can be invoked via fixed set of core pmu events with 'precise_ip'
>> set to 1. Add a simple event open test for all these events.
>>
>> Without kernel fix:
>>   $ sudo ./perf test -vv 76
>>    76: AMD 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 ----
>>   AMD IBS via core pmu: FAILED!
>>
>> With kernel fix:
>>   $ sudo ./perf test -vv 76
>>    76: AMD 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 ----
>>   AMD IBS via core pmu: Ok
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> 
> Thanks Ravi, as the test is AMD specific I think it makes sense to place it in:
> tools/perf/arch/x86/tests
> and then to update the test list in:
> tools/perf/arch/x86/tests/arch-tests.c

Sure, will respin.

Thanks,
Ravi