Re: [PATCH 02/19] perf/hisilicon: Fix group validation

[Robin Murphy <robin.murphy@arm.com>]

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.murphy@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
>>