[PATCH 1/2] perf/x86/intel: Only check the group flag for X86 leader

[PATCH 1/2] perf/x86/intel: Only check the group flag for X86 leader

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

A warning in intel_pmu_lbr_counters_reorder() may be triggered by below
perf command.

perf record -e "{cpu-clock,cycles/call-graph="lbr"/}" -- sleep 1

It's because the group is mistakenly treated as a branch counter group.

The hw.flags of the leader are used to determine whether a group is a
branch counters group. However, the hw.flags is only available for a
hardware event. The field to store the flags is a union type. For a
software event, it's a hrtimer. The corresponding bit may be set if the
leader is a software event.

For a branch counter group and other groups that have a group flag
(e.g., topdown, PEBS counters snapshotting, and ACR), the leader must
be a X86 event. Check the X86 event before checking the flag.

There may be an alternative way to fix the issue by moving the hw.flags
out of the union type. It should work for now. But it's still possible
that the flags will be used by other types of events later. As long as
that type of event is used as a leader, a similar issue will be
triggered. So the alternative way is dropped.

Reported-by: Luo Gengkun <luogengkun@huaweicloud.com>
Closes: https://lore.kernel.org/lkml/20250412091423.1839809-1-luogengkun@huaweicloud.com/
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 arch/x86/events/perf_event.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 902bc42a6cfe..cc7717a1d6f3 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -110,19 +110,26 @@ static inline bool is_topdown_event(struct perf_event *event)
 	return is_metric_event(event) || is_slots_event(event);
 }
 
+int is_x86_event(struct perf_event *event);
+
+static inline bool check_leader_group(struct perf_event *leader, int flags)
+{
+	return is_x86_event(leader) ? !!(leader->hw.flags & flags) : false;
+}
+
 static inline bool is_branch_counters_group(struct perf_event *event)
 {
-	return event->group_leader->hw.flags & PERF_X86_EVENT_BRANCH_COUNTERS;
+	return check_leader_group(event->group_leader, PERF_X86_EVENT_BRANCH_COUNTERS);
 }
 
 static inline bool is_pebs_counter_event_group(struct perf_event *event)
 {
-	return event->group_leader->hw.flags & PERF_X86_EVENT_PEBS_CNTR;
+	return check_leader_group(event->group_leader, PERF_X86_EVENT_PEBS_CNTR);
 }
 
 static inline bool is_acr_event_group(struct perf_event *event)
 {
-	return event->group_leader->hw.flags & PERF_X86_EVENT_ACR;
+	return check_leader_group(event->group_leader, PERF_X86_EVENT_ACR);
 }
 
 struct amd_nb {
@@ -1129,7 +1136,6 @@ static struct perf_pmu_format_hybrid_attr format_attr_hybrid_##_name = {\
 	.pmu_type	= _pmu,						\
 }
 
-int is_x86_event(struct perf_event *event);
 struct pmu *x86_get_pmu(unsigned int cpu);
 extern struct x86_pmu x86_pmu __read_mostly;
 
-- 
2.38.1

[PATCH 2/2] perf/x86: Optimize the is_x86_event

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

The current is_x86_event has to go through the hybrid_pmus list to find
the matched pmu, then check if it's a X86 PMU and a X86 event. It's not
necessary.

The X86 PMU has a unique type ID on a non-hybrid machine, and a unique
capability type. They are good enough to do the check.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 arch/x86/events/core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index b0ef07d14c83..1aaa77875c5c 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -757,15 +757,16 @@ void x86_pmu_enable_all(int added)
 
 int is_x86_event(struct perf_event *event)
 {
-	int i;
-
-	if (!is_hybrid())
-		return event->pmu == &pmu;
-
-	for (i = 0; i < x86_pmu.num_hybrid_pmus; i++) {
-		if (event->pmu == &x86_pmu.hybrid_pmu[i].pmu)
-			return true;
-	}
+	/*
+	 * For a non-hybrid platforms, the type of X86 pmu is
+	 * always PERF_TYPE_RAW.
+	 * For a hybrid platform, the PERF_PMU_CAP_EXTENDED_HW_TYPE
+	 * is a unique capability for the X86 PMU.
+	 * Use them to detect a X86 event.
+	 */
+	if (event->pmu->type == PERF_TYPE_RAW ||
+	    event->pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE)
+		return true;
 
 	return false;
 }
-- 
2.38.1

Re: [PATCH 1/2] perf/x86/intel: Only check the group flag for X86 leader

[Peter Zijlstra]

On Wed, Apr 23, 2025 at 03:10:14PM -0700, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> A warning in intel_pmu_lbr_counters_reorder() may be triggered by below
> perf command.
> 
> perf record -e "{cpu-clock,cycles/call-graph="lbr"/}" -- sleep 1
> 
> It's because the group is mistakenly treated as a branch counter group.
> 
> The hw.flags of the leader are used to determine whether a group is a
> branch counters group. However, the hw.flags is only available for a
> hardware event. The field to store the flags is a union type. For a
> software event, it's a hrtimer. The corresponding bit may be set if the
> leader is a software event.
> 
> For a branch counter group and other groups that have a group flag
> (e.g., topdown, PEBS counters snapshotting, and ACR), the leader must
> be a X86 event. Check the X86 event before checking the flag.
> 
> There may be an alternative way to fix the issue by moving the hw.flags
> out of the union type. It should work for now. But it's still possible
> that the flags will be used by other types of events later. As long as
> that type of event is used as a leader, a similar issue will be
> triggered. So the alternative way is dropped.
> 
> Reported-by: Luo Gengkun <luogengkun@huaweicloud.com>
> Closes: https://lore.kernel.org/lkml/20250412091423.1839809-1-luogengkun@huaweicloud.com/
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>

Can I get a Fixes tag for this such that I can stick it in urgent?

Re: [PATCH 1/2] perf/x86/intel: Only check the group flag for X86 leader

[Liang, Kan]

On 2025-04-24 5:40 a.m., Peter Zijlstra wrote:
> On Wed, Apr 23, 2025 at 03:10:14PM -0700, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> A warning in intel_pmu_lbr_counters_reorder() may be triggered by below
>> perf command.
>>
>> perf record -e "{cpu-clock,cycles/call-graph="lbr"/}" -- sleep 1
>>
>> It's because the group is mistakenly treated as a branch counter group.
>>
>> The hw.flags of the leader are used to determine whether a group is a
>> branch counters group. However, the hw.flags is only available for a
>> hardware event. The field to store the flags is a union type. For a
>> software event, it's a hrtimer. The corresponding bit may be set if the
>> leader is a software event.
>>
>> For a branch counter group and other groups that have a group flag
>> (e.g., topdown, PEBS counters snapshotting, and ACR), the leader must
>> be a X86 event. Check the X86 event before checking the flag.
>>
>> There may be an alternative way to fix the issue by moving the hw.flags
>> out of the union type. It should work for now. But it's still possible
>> that the flags will be used by other types of events later. As long as
>> that type of event is used as a leader, a similar issue will be
>> triggered. So the alternative way is dropped.
>>
>> Reported-by: Luo Gengkun <luogengkun@huaweicloud.com>
>> Closes: https://lore.kernel.org/lkml/20250412091423.1839809-1-luogengkun@huaweicloud.com/
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> 
> Can I get a Fixes tag for this such that I can stick it in urgent?
>

I have split the patch series and sent a V2.
https://lore.kernel.org/lkml/20250424134718.311934-1-kan.liang@linux.intel.com/

There is a fix for the non-precise events counters-snapshotting, which
is buried in the LKML.
https://lore.kernel.org/lkml/20250204210514.4089680-1-kan.liang@linux.intel.com/
I've also added it into the above V2 with a fixes tag.
Please take a look. If it looks good for you, please stick it in urgent
as well.

Thanks,
Kan

Re: [PATCH 1/2] perf/x86/intel: Only check the group flag for X86 leader

[Peter Zijlstra]

On Thu, Apr 24, 2025 at 09:50:09AM -0400, Liang, Kan wrote:

> I have split the patch series and sent a V2.
> https://lore.kernel.org/lkml/20250424134718.311934-1-kan.liang@linux.intel.com/
> 
> There is a fix for the non-precise events counters-snapshotting, which
> is buried in the LKML.
> https://lore.kernel.org/lkml/20250204210514.4089680-1-kan.liang@linux.intel.com/
> I've also added it into the above V2 with a fixes tag.
> Please take a look. If it looks good for you, please stick it in urgent
> as well.


Thanks, let me go see..