Re: [PATCH 02/10] perf: Add capability for common event support

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

On 2024-03-14 8:09 am, Yang Jialong 杨佳龙 wrote:
> 
> 
> 在 2024/3/13 1:34, Robin Murphy 写道:
>> Many PMUs do not support common hardware/cache/etc. events and only
>> handle their own PMU-specific events. Since this only depends on
>> matching the event and PMU types, it's a prime candidate for a core
>> capability to save more event_init boilerplate in drivers.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   include/linux/perf_event.h | 1 +
>>   kernel/events/core.c       | 5 +++++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index d2a15c0c6f8a..983201f21dd2 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -291,6 +291,7 @@ struct perf_event_pmu_context;
>>   #define PERF_PMU_CAP_NO_EXCLUDE            0x0040
>>   #define PERF_PMU_CAP_AUX_OUTPUT            0x0080
>>   #define PERF_PMU_CAP_EXTENDED_HW_TYPE        0x0100
>> +#define PERF_PMU_CAP_NO_COMMON_EVENTS        0x0200
>>   struct perf_output_handle;
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index f0f0f71213a1..7ad80826c218 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -11649,6 +11649,11 @@ static int perf_try_init_event(struct pmu 
>> *pmu, struct perf_event *event)
>>       struct perf_event_context *ctx = NULL;
>>       int ret;
>> +    /* Short-circuit if we know the PMU won't want this event */
>> +    if (pmu->capabilities & PERF_PMU_CAP_NO_COMMON_EVENTS &&
>> +        event->attr.type != pmu->type)
>> +        return -ENOENT;
>> +
> 
>          /*
>           * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE
>           * are often aliases for PERF_TYPE_RAW.
>           */
>          type = event->attr.type;
>          if (type == PERF_TYPE_HARDWARE || type == PERF_TYPE_HW_CACHE) {
>                  type = event->attr.config >> PERF_PMU_TYPE_SHIFT;
>                  if (!type) {
>                          type = PERF_TYPE_RAW;
>                  } else {
>                          extended_type = true;
>                          event->attr.config &= PERF_HW_EVENT_MASK;
>                  }
>          }
> 
> again:
>          rcu_read_lock();
>          pmu = idr_find(&pmu_idr, type);
>          rcu_read_unlock();
>          if (pmu) {
> Above code tells me it's possible that 'pmu->type != event->attr.type' 
> is true when event->attr.type equals to PERF_TYPE_HARDWARE or 
> PERF_TYPE_HW_CACHE, and pmu->type should equal to event->attr.config >> 
> PERF_PMU_TYPE_SHIFT.
> 
> We find the target pmu by event->attr.config >> PERF_PMU_TYPE_SHIFT.

And if that PMU doesn't actually support PERF_TYPE_HARDWARE or 
PERF_TYPE_HW_CACHE then it would reject the event, if the very next 
lines didn't already do that:

	if (event->attr.type != type && type != PERF_TYPE_RAW &&
	    !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
		goto fail;

Either way it should be clear that there's no change of functionality 
here since the flow into perf_try_init_event() itself is untouched.

> Code added discard this option.

It would already be nonsensical for a driver to advertise 
PERF_PMU_CAP_EXTENDED_HW_TYPE to say it supports extended hardware 
events, but then reject all hardware events with a "event->attr.type != 
pmu->type" check in its event_init. Reworking the latter condition into 
PERF_PMU_CAP_NO_COMMON_EVENTS doesn't change that.

Thanks,
Robin.

> 
> And code tells me that no try. Target PMU is doubtless.
> 
> 
> 
> 
>>       if (!try_module_get(pmu->module))
>>           return -ENODEV;
>