| Message ID | 1546878450-20341-6-git-send-email-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | perf/core: Generalise event exclusion checking | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 33 lines checked |
On Mon, Jan 07, 2019 at 04:27:22PM +0000, Andrew Murray wrote: > @@ -393,9 +386,8 @@ __hw_perf_event_init(struct perf_event *event) > /* > * Check whether we need to exclude the counter from certain modes. > */ > + if (armpmu->set_event_filter && > + armpmu->set_event_filter(hwc, &event->attr)) { > pr_debug("ARM performance counters do not support " > "mode exclusion\n"); > return -EOPNOTSUPP; This then requires all set_event_filter() implementations to check all the various exclude options; also, set_event_filter() failing then returns with -EOPNOTSUPP instead of the -EINVAL the CAP_NO_EXCLUDE generates, which is again inconsitent. If I look at (the very first git-grep found me) armv7pmu_set_event_filter(), then I find it returning -EPERM (again inconsistent but irrelevant because the actual value is not preserved) for exclude_idle. But it doesn't seem to check exclude_host at all for example. > @@ -867,6 +859,9 @@ int armpmu_register(struct arm_pmu *pmu) > if (ret) > return ret; > > + if (!pmu->set_event_filter) > + pmu->pmu.capabilities |= PERF_PMU_CAP_NO_EXCLUDE; > + > ret = perf_pmu_register(&pmu->pmu, pmu->name, -1); > if (ret) > goto out_destroy; > -- > 2.7.4 >
On Tue, Jan 08, 2019 at 11:28:02AM +0100, Peter Zijlstra wrote: > On Mon, Jan 07, 2019 at 04:27:22PM +0000, Andrew Murray wrote: > > @@ -393,9 +386,8 @@ __hw_perf_event_init(struct perf_event *event) > > /* > > * Check whether we need to exclude the counter from certain modes. > > */ > > + if (armpmu->set_event_filter && > > + armpmu->set_event_filter(hwc, &event->attr)) { > > pr_debug("ARM performance counters do not support " > > "mode exclusion\n"); > > return -EOPNOTSUPP; > > This then requires all set_event_filter() implementations to check all > the various exclude options; Yes but this isn't a new requirement, this hunk uses the absence of set_event_filter to blanket indicate that no exclusion flags are supported. > also, set_event_filter() failing then > returns with -EOPNOTSUPP instead of the -EINVAL the CAP_NO_EXCLUDE > generates, which is again inconsitent. Yes, it's not ideal - but a step in the right direction. I wanted to limit user visible changes as much as possible, where I've identified them I've noted it in the commit log. > > If I look at (the very first git-grep found me) > armv7pmu_set_event_filter(), then I find it returning -EPERM (again > inconsistent but irrelevant because the actual value is not preserved) > for exclude_idle. > > But it doesn't seem to check exclude_host at all for example. Yes I found lots of examples like this across the tree whilst doing this work. However I decided to initially start with simply removing duplicated code as a result of adding this flag and attempting to preserve existing functionality. I thought that if I add missing checks then the patchset will get much bigger and be harder to merge. I would like to do this though as another non-cross-arch series. Can we limit this patch series to the minimal changes required to fully use PERF_PMU_CAP_NO_EXCLUDE and then attempt to fix these existing problems in subsequent patch sets? Thanks, Andrew Murray > > > @@ -867,6 +859,9 @@ int armpmu_register(struct arm_pmu *pmu) > > if (ret) > > return ret; > > > > + if (!pmu->set_event_filter) > > + pmu->pmu.capabilities |= PERF_PMU_CAP_NO_EXCLUDE; > > + > > ret = perf_pmu_register(&pmu->pmu, pmu->name, -1); > > if (ret) > > goto out_destroy; > > -- > > 2.7.4 > >
On Tue, Jan 08, 2019 at 01:07:41PM +0000, Andrew Murray wrote: > Yes I found lots of examples like this across the tree whilst doing this > work. However I decided to initially start with simply removing duplicated > code as a result of adding this flag and attempting to preserve existing > functionality. I thought that if I add missing checks then the patchset > will get much bigger and be harder to merge. I would like to do this though > as another non-cross-arch series. > > Can we limit this patch series to the minimal changes required to fully > use PERF_PMU_CAP_NO_EXCLUDE and then attempt to fix these existing problems > in subsequent patch sets? Ok, but it would've been nice to see that mentioned somewhere.
On Tue, Jan 08, 2019 at 02:10:31PM +0100, Peter Zijlstra wrote: > On Tue, Jan 08, 2019 at 01:07:41PM +0000, Andrew Murray wrote: > > > Yes I found lots of examples like this across the tree whilst doing this > > work. However I decided to initially start with simply removing duplicated > > code as a result of adding this flag and attempting to preserve existing > > functionality. I thought that if I add missing checks then the patchset > > will get much bigger and be harder to merge. I would like to do this though > > as another non-cross-arch series. > > > > Can we limit this patch series to the minimal changes required to fully > > use PERF_PMU_CAP_NO_EXCLUDE and then attempt to fix these existing problems > > in subsequent patch sets? > > Ok, but it would've been nice to see that mentioned somewhere. I'll update the cover leter on any next revision. I'll try to be clearer next time with my intentions. Andrew Murray
On Tue, Jan 08, 2019 at 01:13:57PM +0000, Andrew Murray wrote: > On Tue, Jan 08, 2019 at 02:10:31PM +0100, Peter Zijlstra wrote: > > On Tue, Jan 08, 2019 at 01:07:41PM +0000, Andrew Murray wrote: > > > > > Yes I found lots of examples like this across the tree whilst doing this > > > work. However I decided to initially start with simply removing duplicated > > > code as a result of adding this flag and attempting to preserve existing > > > functionality. I thought that if I add missing checks then the patchset > > > will get much bigger and be harder to merge. I would like to do this though > > > as another non-cross-arch series. > > > > > > Can we limit this patch series to the minimal changes required to fully > > > use PERF_PMU_CAP_NO_EXCLUDE and then attempt to fix these existing problems > > > in subsequent patch sets? > > > > Ok, but it would've been nice to see that mentioned somewhere. > > I'll update the cover leter on any next revision. I'll try to be clearer next > time with my intentions. Could you maybe include it in the relevant patches too; like for example the ARM one where we rely on set_event_filter() to DTRT. So with the changelogs and subjects fixed I can take these patches and then you can get on with cleaning up the individual drivers.
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c index d0b7dd8..eec75b9 100644 --- a/drivers/perf/arm_pmu.c +++ b/drivers/perf/arm_pmu.c @@ -357,13 +357,6 @@ static irqreturn_t armpmu_dispatch_irq(int irq, void *dev) } static int -event_requires_mode_exclusion(struct perf_event_attr *attr) -{ - return attr->exclude_idle || attr->exclude_user || - attr->exclude_kernel || attr->exclude_hv; -} - -static int __hw_perf_event_init(struct perf_event *event) { struct arm_pmu *armpmu = to_arm_pmu(event->pmu); @@ -393,9 +386,8 @@ __hw_perf_event_init(struct perf_event *event) /* * Check whether we need to exclude the counter from certain modes. */ - if ((!armpmu->set_event_filter || - armpmu->set_event_filter(hwc, &event->attr)) && - event_requires_mode_exclusion(&event->attr)) { + if (armpmu->set_event_filter && + armpmu->set_event_filter(hwc, &event->attr)) { pr_debug("ARM performance counters do not support " "mode exclusion\n"); return -EOPNOTSUPP; @@ -867,6 +859,9 @@ int armpmu_register(struct arm_pmu *pmu) if (ret) return ret; + if (!pmu->set_event_filter) + pmu->pmu.capabilities |= PERF_PMU_CAP_NO_EXCLUDE; + ret = perf_pmu_register(&pmu->pmu, pmu->name, -1); if (ret) goto out_destroy;