| Message ID | feb10873fe9e4e10b5ffbbe8e296c8a45632e3c2.1710257512.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: Clean up common uncore boilerplate | expand |
在 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. Code added discard this option. And code tells me that no try. Target PMU is doubtless. > if (!try_module_get(pmu->module)) > return -ENODEV; >
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; >
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; + if (!try_module_get(pmu->module)) return -ENODEV;
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(+)