On 2025-08-26 12:15 pm, Mark Rutland wrote:
On Wed, Aug 13, 2025 at 06:00:54PM +0100, Robin Murphy wrote:
The group validation logic shared by the HiSilicon HNS3/PCIe drivers is
a bit off, in that given a software group leader, it will consider that
event *in place of* the actual new event being opened. At worst this
could theoretically allow an unschedulable group if the software event
config happens to look like one of the hardware siblings.
The uncore framework avoids that particular issue,
What is "the uncore framework"? I'm not sure exactly what you're
referring to, nor how that composes with the problem described above.
Literally that hisi_uncore_pmu.c is actually a framework for half a
dozen individual sub-drivers rather than a "driver" itself per se, but I
suppose that detail doesn't strictly matter at this level.
but all 3 also share the common issue of not preventing racy access to
the sibling list,
Can you please elaborate on this racy access to the silbing list? I'm
not sure exactly what you're referring to.
Hmm, yes, I guess an actual race is probably impossible since if we're
still in the middle of opening the group leader event then we haven't
yet allocated the fd that userspace would need to start adding siblings,
even if it tried to guess. I leaned on "racy" as a concise way to infer
"when it isn't locked (even though the reasons for that are more
subtle)" repeatedly over several patches - after all, the overall theme
of this series is that I dislike repetitive boilerplate :)
I'll dedicate some time for polishing commit messages for v2, especially
the common context for these "part 1" patches per your feedback on patch #1.
and some redundant checks which can be cleaned up.
Signed-off-by: Robin Murphy <robin.mur...@arm.com>
---
drivers/perf/hisilicon/hisi_pcie_pmu.c | 17 ++++++-----------
drivers/perf/hisilicon/hisi_uncore_pmu.c | 23 +++++++----------------
drivers/perf/hisilicon/hns3_pmu.c | 17 ++++++-----------
3 files changed, 19 insertions(+), 38 deletions(-)
diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c
b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index c5394d007b61..3b0b2f7197d0 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -338,21 +338,16 @@ static bool hisi_pcie_pmu_validate_event_group(struct
perf_event *event)
int counters = 1;
int num;
- event_group[0] = leader;
- if (!is_software_event(leader)) {
- if (leader->pmu != event->pmu)
- return false;
+ if (leader == event)
+ return true;
- if (leader != event && !hisi_pcie_pmu_cmp_event(leader, event))
- event_group[counters++] = event;
- }
+ event_group[0] = event;
+ if (leader->pmu == event->pmu && !hisi_pcie_pmu_cmp_event(leader,
event))
+ event_group[counters++] = leader;
Looking at this, the existing logic to share counters (which
hisi_pcie_pmu_cmp_event() is trying to permit) looks to be bogus, given
that the start/stop callbacks will reprogram the HW counters (and hence
can fight with one another).
Yeah, this had a dodgy smell when I first came across it, but after
doing all the digging I think it does actually work out - the trick
seems to be the group_leader check in hisi_pcie_pmu_get_event_idx(),
with the implication the PMU is going to be stopped while scheduling
in/out the whole group, so assuming hisi_pcie_pmu_del() doesn't clear
the counter value in hardware (even though the first call nukes the rest
of the event configuration), then the events should stay in sync.
It does seem somewhat nonsensical to have multiple copies of the same
event in the same group, but I imagine it could happen with some sort of
scripted combination of metrics, and supporting it at this level saves
needing explicit deduplication further up. So even though my initial
instinct was to rip it out too, in the end I concluded that that doesn't
seem justified.
Thanks,
Robin.
I suspect that can be removed *entirely*, and this can be simplified
down to allocating N counters, without a quadratic event comparison. We
don't try to share counters in other PMU drivers, and there was no
rationale for trying to do this when this wa introduced in commit:
8404b0fbc7fbd42e ("drivers/perf: hisi: Add driver for HiSilicon PCIe PMU")
The 'link' tag in that comment goes to v13, which doesn't link to prior
postings, so I'm not going to dig further.
Mark.
for_each_sibling_event(sibling, event->group_leader) {
- if (is_software_event(sibling))
- continue;
-
if (sibling->pmu != event->pmu)
- return false;
+ continue;
for (num = 0; num < counters; num++) {
/*
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c
b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index a449651f79c9..3c531b36cf25 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -101,26 +101,17 @@ static bool hisi_validate_event_group(struct perf_event
*event)
/* Include count for the event */
int counters = 1;
- if (!is_software_event(leader)) {
- /*
- * We must NOT create groups containing mixed PMUs, although
- * software events are acceptable
- */
- if (leader->pmu != event->pmu)
- return false;
+ if (leader == event)
+ return true;
- /* Increment counter for the leader */
- if (leader != event)
- counters++;
- }
+ /* Increment counter for the leader */
+ if (leader->pmu == event->pmu)
+ counters++;
for_each_sibling_event(sibling, event->group_leader) {
- if (is_software_event(sibling))
- continue;
- if (sibling->pmu != event->pmu)
- return false;
/* Increment counter for each sibling */
- counters++;
+ if (sibling->pmu == event->pmu)
+ counters++;
}
/* The group can not count events more than the counters in the HW */
diff --git a/drivers/perf/hisilicon/hns3_pmu.c
b/drivers/perf/hisilicon/hns3_pmu.c
index c157f3572cae..382e469257f9 100644
--- a/drivers/perf/hisilicon/hns3_pmu.c
+++ b/drivers/perf/hisilicon/hns3_pmu.c
@@ -1058,21 +1058,16 @@ static bool hns3_pmu_validate_event_group(struct
perf_event *event)
int counters = 1;
int num;
- event_group[0] = leader;
- if (!is_software_event(leader)) {
- if (leader->pmu != event->pmu)
- return false;
+ if (leader == event)
+ return true;
- if (leader != event && !hns3_pmu_cmp_event(leader, event))
- event_group[counters++] = event;
- }
+ event_group[0] = event;
+ if (leader->pmu == event->pmu && !hns3_pmu_cmp_event(leader, event))
+ event_group[counters++] = leader;
for_each_sibling_event(sibling, event->group_leader) {
- if (is_software_event(sibling))
- continue;
-
if (sibling->pmu != event->pmu)
- return false;
+ continue;
for (num = 0; num < counters; num++) {
/*
--
2.39.2.101.g768bb238c484.dirty
