Received-SPF: pass (google.com: domain of linux-kernel+bounces-103247-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <7ebe3c5f-577b-4c67-b8b9-14e8ed6429bf@arm.com>
Date: Thu, 14 Mar 2024 12:34:30 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 02/10] perf: Add capability for common event support
Content-Language: en-GB
To: =?UTF-8?B?WWFuZyBKaWFsb25nIOadqOS9s+m+mQ==?= <jialong.yang@shingroup.cn>,
 Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
 Arnaldo Carvalho de Melo <acme@kernel.org>,
 Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Will Deacon <will@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
 Adrian Hunter <adrian.hunter@intel.com>, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, x86@kernel.org,
 linux-perf-users@vger.kernel.org
References: <cover.1710257512.git.robin.murphy@arm.com>
 <feb10873fe9e4e10b5ffbbe8e296c8a45632e3c2.1710257512.git.robin.murphy@arm.com>
 <94DB0209EA3D1C5F+b646f31e-15a1-4753-8309-687b5860863e@shingroup.cn>
From: Robin Murphy <robin.murphy@arm.com>
In-Reply-To: <94DB0209EA3D1C5F+b646f31e-15a1-4753-8309-687b5860863e@shingroup.cn>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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;
>