perf/arm_smmuv3: Omit the two judgements which done in framework

  'event->attr.type != event->pmu->type' has been done in
core.c::perf_init_event() ,core.c::perf_event_modify_attr(), etc.

This PMU is an uncore one. The core framework has disallowed
uncore-task events. So the judgement to event->cpu < 0 is no mean.

The two judgements have been done in kernel/events/core.c

Signed-off-by: JiaLong.Yang <jialong.yang@shingroup.cn>
---
 drivers/perf/arm_smmuv3_pmu.c | 8 --------
 1 file changed, 8 deletions(-)
  

On Thu, Dec 21, 2023 at 05:38:01PM +0800, JiaLong.Yang wrote:
> 'event->attr.type != event->pmu->type' has been done in
> core.c::perf_init_event() ,core.c::perf_event_modify_attr(), etc.
>
> This PMU is an uncore one. The core framework has disallowed
> uncore-task events. So the judgement to event->cpu < 0 is no mean.

It would be great to refer to the changes which added those checks to
the perf core code. From reading the code myself, I can't convince myself
that perf_try_init_event() won't call into the driver.

> 
> The two judgements have been done in kernel/events/core.c
> 
> Signed-off-by: JiaLong.Yang <jialong.yang@shingroup.cn>
> ---
>  drivers/perf/arm_smmuv3_pmu.c | 8 --------
>  1 file changed, 8 deletions(-)

It looks like _many_ perf drivers have these checks, so if they really
aren't needed, we can clean this up bveyond SMMU. However, as I said
above, I'm not quite convinced we can drop them.

Will
  

On 2024-02-09 4:09 pm, Will Deacon wrote:
> On Thu, Dec 21, 2023 at 05:38:01PM +0800, JiaLong.Yang wrote:
>> 'event->attr.type != event->pmu->type' has been done in
>> core.c::perf_init_event() ,core.c::perf_event_modify_attr(), etc.
>>
>> This PMU is an uncore one. The core framework has disallowed
>> uncore-task events. So the judgement to event->cpu < 0 is no mean.
> 
> It would be great to refer to the changes which added those checks to
> the perf core code. From reading the code myself, I can't convince myself
> that perf_try_init_event() won't call into the driver.
> 
>>
>> The two judgements have been done in kernel/events/core.c
>>
>> Signed-off-by: JiaLong.Yang <jialong.yang@shingroup.cn>
>> ---
>>   drivers/perf/arm_smmuv3_pmu.c | 8 --------
>>   1 file changed, 8 deletions(-)
> 
> It looks like _many_ perf drivers have these checks, so if they really
> aren't needed, we can clean this up bveyond SMMU. However, as I said
> above, I'm not quite convinced we can drop them.

Right, I think the logic prevents events with a specific PMU type being 
offered to other PMUs, but as far as I'm aware doesn't apply the other 
way round to stop generic events (PERF_TYPE_HARDWARE etc.) being offered 
to all PMUs, so it's those that system PMUs need to reject.

It's been on my wishlist for a long time to have a capability flag to 
say "I don't handle generic events, please only ever give me events of 
my exact type" so we *can* truly factor this into the core.

Thanks,
Robin.
  

在 2024/2/10 0:32, Robin Murphy 写道:
> On 2024-02-09 4:09 pm, Will Deacon wrote:
>> On Thu, Dec 21, 2023 at 05:38:01PM +0800, JiaLong.Yang wrote:
>>> 'event->attr.type != event->pmu->type' has been done in
>>> core.c::perf_init_event() ,core.c::perf_event_modify_attr(), etc.
>>>
>>> This PMU is an uncore one. The core framework has disallowed
>>> uncore-task events. So the judgement to event->cpu < 0 is no mean.
>>
>> It would be great to refer to the changes which added those checks to
>> the perf core code. From reading the code myself, I can't convince myself
>> that perf_try_init_event() won't call into the driver.
>>
>>>
>>> The two judgements have been done in kernel/events/core.c
>>>
>>> Signed-off-by: JiaLong.Yang <jialong.yang@shingroup.cn>
>>> ---
>>>   drivers/perf/arm_smmuv3_pmu.c | 8 --------
>>>   1 file changed, 8 deletions(-)
>>
>> It looks like _many_ perf drivers have these checks, so if they really
>> aren't needed, we can clean this up bveyond SMMU. However, as I said
>> above, I'm not quite convinced we can drop them.
> 
> Right, I think the logic prevents events with a specific PMU type being 
> offered to other PMUs, but as far as I'm aware doesn't apply the other 
> way round to stop generic events (PERF_TYPE_HARDWARE etc.) being offered 
> to all PMUs, so it's those that system PMUs need to reject. >
> It's been on my wishlist for a long time to have a capability flag to 
> say "I don't handle generic events, please only ever give me events of 
> my exact type" so we *can* truly factor this into the core.


It's core framework's responsible to differ generic events and others, 
or uncore pmu and core pmu.
Here we have flag PERF_TYPE_HARDWARE, PERF_TYPE_HW_CACHE, 
PERF_PMU_CAP_EXTENDED_HW_TYPE doing this.

again:
         rcu_read_lock();
         pmu = idr_find(&pmu_idr, type);
         rcu_read_unlock();
         if (pmu) {
                 if (event->attr.type != type && type != PERF_TYPE_RAW &&
                     !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
                         goto fail; /* generic event with no ability pmu */
This can avoid driver code (event->attr.type != event->pmu->type).

And, code:
if (pmu->task_ctx_nr == perf_invalid_context && (task || cgroup_fd != -1)) {
	err = -EINVAL;
	goto err_pmu;
}
can promise not uncore-task event.

We should confirm that uncore event should be cpu >= 0.

> 
> Thanks,
> Robin.
>
  

On 19/02/2024 6:37 am, Yang Jialong 杨佳龙 wrote:
> 
> 
> 在 2024/2/10 0:32, Robin Murphy 写道:
>> On 2024-02-09 4:09 pm, Will Deacon wrote:
>>> On Thu, Dec 21, 2023 at 05:38:01PM +0800, JiaLong.Yang wrote:
>>>> 'event->attr.type != event->pmu->type' has been done in
>>>> core.c::perf_init_event() ,core.c::perf_event_modify_attr(), etc.
>>>>
>>>> This PMU is an uncore one. The core framework has disallowed
>>>> uncore-task events. So the judgement to event->cpu < 0 is no mean.
>>>
>>> It would be great to refer to the changes which added those checks to
>>> the perf core code. From reading the code myself, I can't convince 
>>> myself
>>> that perf_try_init_event() won't call into the driver.
>>>
>>>>
>>>> The two judgements have been done in kernel/events/core.c
>>>>
>>>> Signed-off-by: JiaLong.Yang <jialong.yang@shingroup.cn>
>>>> ---
>>>>   drivers/perf/arm_smmuv3_pmu.c | 8 --------
>>>>   1 file changed, 8 deletions(-)
>>>
>>> It looks like _many_ perf drivers have these checks, so if they really
>>> aren't needed, we can clean this up bveyond SMMU. However, as I said
>>> above, I'm not quite convinced we can drop them.
>>
>> Right, I think the logic prevents events with a specific PMU type 
>> being offered to other PMUs, but as far as I'm aware doesn't apply the 
>> other way round to stop generic events (PERF_TYPE_HARDWARE etc.) being 
>> offered to all PMUs, so it's those that system PMUs need to reject. >
>> It's been on my wishlist for a long time to have a capability flag to 
>> say "I don't handle generic events, please only ever give me events of 
>> my exact type" so we *can* truly factor this into the core.
> 
> 
> It's core framework's responsible to differ generic events and others, 
> or uncore pmu and core pmu.
> Here we have flag PERF_TYPE_HARDWARE, PERF_TYPE_HW_CACHE, 
> PERF_PMU_CAP_EXTENDED_HW_TYPE doing this.
> 
> again:
>          rcu_read_lock();
>          pmu = idr_find(&pmu_idr, type);
>          rcu_read_unlock();
>          if (pmu) {
>                  if (event->attr.type != type && type != PERF_TYPE_RAW &&
>                      !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
>                          goto fail; /* generic event with no ability pmu */
> This can avoid driver code (event->attr.type != event->pmu->type).

Now consider the other case below that, where "type" has not matched 
anything registered, so "pmu" is NULL, and the event is then offered to 
*every* registered PMU to see if anyone accepts it. Note that even CPU 
PMUs don't always register as PERF_TYPE_RAW, and in particular arm_pmu 
doesn't.

Thanks,
Robin.
  

在 2024/2/27 2:57, Robin Murphy 写道:
> On 19/02/2024 6:37 am, Yang Jialong 杨佳龙 wrote:
>>
>>
>> 在 2024/2/10 0:32, Robin Murphy 写道:
>>> On 2024-02-09 4:09 pm, Will Deacon wrote:
>>>> On Thu, Dec 21, 2023 at 05:38:01PM +0800, JiaLong.Yang wrote:
>>>>> 'event->attr.type != event->pmu->type' has been done in
>>>>> core.c::perf_init_event() ,core.c::perf_event_modify_attr(), etc.
>>>>>
>>>>> This PMU is an uncore one. The core framework has disallowed
>>>>> uncore-task events. So the judgement to event->cpu < 0 is no mean.
>>>>
>>>> It would be great to refer to the changes which added those checks to
>>>> the perf core code. From reading the code myself, I can't convince 
>>>> myself
>>>> that perf_try_init_event() won't call into the driver.
>>>>
>>>>>
>>>>> The two judgements have been done in kernel/events/core.c
>>>>>
>>>>> Signed-off-by: JiaLong.Yang <jialong.yang@shingroup.cn>
>>>>> ---
>>>>>   drivers/perf/arm_smmuv3_pmu.c | 8 --------
>>>>>   1 file changed, 8 deletions(-)
>>>>
>>>> It looks like _many_ perf drivers have these checks, so if they really
>>>> aren't needed, we can clean this up bveyond SMMU. However, as I said
>>>> above, I'm not quite convinced we can drop them.
>>>
>>> Right, I think the logic prevents events with a specific PMU type 
>>> being offered to other PMUs, but as far as I'm aware doesn't apply 
>>> the other way round to stop generic events (PERF_TYPE_HARDWARE etc.) 
>>> being offered to all PMUs, so it's those that system PMUs need to 
>>> reject. >
>>> It's been on my wishlist for a long time to have a capability flag to 
>>> say "I don't handle generic events, please only ever give me events 
>>> of my exact type" so we *can* truly factor this into the core.
>>
>>
>> It's core framework's responsible to differ generic events and others, 
>> or uncore pmu and core pmu.
>> Here we have flag PERF_TYPE_HARDWARE, PERF_TYPE_HW_CACHE, 
>> PERF_PMU_CAP_EXTENDED_HW_TYPE doing this.
>>
>> again:
>>          rcu_read_lock();
>>          pmu = idr_find(&pmu_idr, type);
>>          rcu_read_unlock();
>>          if (pmu) {
>>                  if (event->attr.type != type && type != PERF_TYPE_RAW &&
>>                      !(pmu->capabilities & 
>> PERF_PMU_CAP_EXTENDED_HW_TYPE))
>>                          goto fail; /* generic event with no ability 
>> pmu */
>> This can avoid driver code (event->attr.type != event->pmu->type).
> 
> Now consider the other case below that, where "type" has not matched 
> anything registered, so "pmu" is NULL, and the event is then offered to 
> *every* registered PMU to see if anyone accepts it. Note that even CPU 
> PMUs don't always register as PERF_TYPE_RAW, and in particular arm_pmu 
> doesn't.
> 
arm_pmu has flag PERF_PMU_CAP_EXTENDED_HW_TYPE.

 From below command, we can say that generic events is related to the 
pmus with type PERF_TYPE_RAW.
~/real_kernel $ grep -nr "PERF_TYPE_HARDWARE" arch/ drivers/ |cut -d: 
-f1|xargs egrep -c "perf_pmu_register.*PERF_TYPE_RAW"
arch/alpha/kernel/perf_event.c:1
arch/alpha/kernel/perf_event.c:1
arch/arc/kernel/perf_event.c:1
arch/csky/kernel/perf_event.c:1
arch/loongarch/kernel/perf_event.c:1
arch/loongarch/kernel/perf_event.c:1
arch/mips/kernel/perf_event_mipsxx.c:1
arch/mips/kernel/perf_event_mipsxx.c:1
arch/powerpc/perf/8xx-pmu.c:1
arch/powerpc/perf/core-fsl-emb.c:1
arch/powerpc/perf/core-book3s.c:1
arch/riscv/kvm/vcpu_pmu.c:0
arch/s390/kernel/perf_cpum_sf.c:1
arch/s390/kernel/perf_cpum_cf.c:0
arch/s390/kernel/perf_cpum_cf.c:0
arch/s390/kernel/perf_cpum_cf.c:0
arch/s390/kernel/perf_cpum_cf.c:0
arch/sh/kernel/perf_event.c:1
arch/sh/kernel/perf_event.c:1
arch/sparc/kernel/perf_event.c:1
arch/x86/events/amd/ibs.c:0
arch/x86/events/intel/core.c:0
arch/x86/events/core.c:1
arch/xtensa/kernel/perf_event.c:1
drivers/perf/arm_pmu.c:0
drivers/perf/arm_pmu.c:0
drivers/perf/riscv_pmu.c:0
drivers/perf/riscv_pmu_legacy.c:1
drivers/perf/arm_pmuv3.c:0
drivers/perf/riscv_pmu_sbi.c:1

 From this, we can say that generic events is related to the pmus with 
capability PERF_PMU_CAP_EXTENDED_HW_TYPE.
~/real_kernel $ grep -nr "PERF_TYPE_HARDWARE" arch/ drivers/ |cut -d: 
-f1|xargs grep -c "PERF_PMU_CAP_EXTENDED_HW_TYPE"
arch/alpha/kernel/perf_event.c:0
arch/alpha/kernel/perf_event.c:0
arch/arc/kernel/perf_event.c:0
arch/csky/kernel/perf_event.c:0
arch/loongarch/kernel/perf_event.c:0
arch/loongarch/kernel/perf_event.c:0
arch/mips/kernel/perf_event_mipsxx.c:0
arch/mips/kernel/perf_event_mipsxx.c:0
arch/powerpc/perf/8xx-pmu.c:0
arch/powerpc/perf/core-fsl-emb.c:0
arch/powerpc/perf/core-book3s.c:0
arch/riscv/kvm/vcpu_pmu.c:0
arch/s390/kernel/perf_cpum_sf.c:0
arch/s390/kernel/perf_cpum_cf.c:0
arch/s390/kernel/perf_cpum_cf.c:0
arch/s390/kernel/perf_cpum_cf.c:0
arch/s390/kernel/perf_cpum_cf.c:0
arch/sh/kernel/perf_event.c:0
arch/sh/kernel/perf_event.c:0
arch/sparc/kernel/perf_event.c:0
arch/x86/events/amd/ibs.c:0
arch/x86/events/intel/core.c:0
arch/x86/events/core.c:1
arch/xtensa/kernel/perf_event.c:0
drivers/perf/arm_pmu.c:2
drivers/perf/arm_pmu.c:2
drivers/perf/riscv_pmu.c:0
drivers/perf/riscv_pmu_legacy.c:0
drivers/perf/arm_pmuv3.c:0
drivers/perf/riscv_pmu_sbi.c:0

****************************
Something bad belows:
~/real_kernel $ grep -nr "PERF_TYPE_HARDWARE" arch/ drivers/ |cut -d: 
-f1|xargs egrep -c "perf_pmu_register.*-1"
arch/alpha/kernel/perf_event.c:0
arch/alpha/kernel/perf_event.c:0
arch/arc/kernel/perf_event.c:0
arch/csky/kernel/perf_event.c:0
arch/loongarch/kernel/perf_event.c:0
arch/loongarch/kernel/perf_event.c:0
arch/mips/kernel/perf_event_mipsxx.c:0
arch/mips/kernel/perf_event_mipsxx.c:0
arch/powerpc/perf/8xx-pmu.c:0
arch/powerpc/perf/core-fsl-emb.c:0
arch/powerpc/perf/core-book3s.c:0
arch/riscv/kvm/vcpu_pmu.c:0
arch/s390/kernel/perf_cpum_sf.c:0
arch/s390/kernel/perf_cpum_cf.c:2
arch/s390/kernel/perf_cpum_cf.c:2
arch/s390/kernel/perf_cpum_cf.c:2
arch/s390/kernel/perf_cpum_cf.c:2
arch/sh/kernel/perf_event.c:0
arch/sh/kernel/perf_event.c:0
arch/sparc/kernel/perf_event.c:0
arch/x86/events/amd/ibs.c:1
arch/x86/events/intel/core.c:0
arch/x86/events/core.c:0
arch/xtensa/kernel/perf_event.c:0
drivers/perf/arm_pmu.c:1
drivers/perf/arm_pmu.c:1
drivers/perf/riscv_pmu.c:0
drivers/perf/riscv_pmu_legacy.c:0
drivers/perf/arm_pmuv3.c:0
drivers/perf/riscv_pmu_sbi.c:0
The pmus::event_init handled PERF_TYPE_HARDWARE, but they was registered 
with perf_pmu_register(*, *, -1) without CAP_EXTENDED_HW_TPYE.

Can we unify them with PERF_TYPE_RAW or CAP_EXTENDED_HW_TYPE? Maybe the 
latter is well. So we can assign idr PERF_TYPE_HARDWARE and 
PERF_TYPE_HW_CACHE a known pmu in perf_pmu_register just like:
if(is_support_generic(pmu)) {
	ret = idr_alloc(&pmu_idr, pmu, PERF_TYPE_HARDWAR, PERF_TYPE_HARDWARE + 
1, GFP_KERNEL);
	if(ret < 0)
		goto free_pdc;
	ret = idr_alloc(&pmu_idr, pmu, PERF_TYPE_HW_CACHE, PERF_TYPE_HW_CACHE + 
1, GFP_KERNEL);
	if(ret < 0)
		goto free_pdc;
}
We will has a easy and rational way to write code in 
core.c::perf_init_event and perf_try_init_event about generic events. We 
can judge cpu < 0 in perf_try_init_event for uncore pmus.
> Thanks,
> Robin.
>