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

[Robin Murphy]

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