| Message ID | 20200115105855.13395-2-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Return EINVAL when precise_ip perf events are requested on Arm | expand |
Hi James, On Wed, Jan 15, 2020 at 10:58:55AM +0000, James Clark wrote: > ARM PMU events can be delivered with arbitrary skid, and there's > nothing the kernel can do to prevent this. Given that, the PMU > cannot support precise_ip != 0. > > Also update comment to state that attr.config field is used to > set the event type rather than event_id which doesn't exist. "Also..." is usually a good sign that you should split up the patch. In this case, you're touching a UAPI header with a questionable clarification, so I'd definitely rather see that handled separately. > Signed-off-by: James Clark <james.clark@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Tan Xiaojun <tanxiaojun@huawei.com> > Cc: Al Grant <al.grant@arm.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/perf/arm_pmu.c | 3 +++ > include/uapi/linux/perf_event.h | 4 ++-- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c > index df352b334ea7..4ddbdb93b3b6 100644 > --- a/drivers/perf/arm_pmu.c > +++ b/drivers/perf/arm_pmu.c > @@ -102,6 +102,9 @@ armpmu_map_event(struct perf_event *event, > u64 config = event->attr.config; > int type = event->attr.type; > > + if (event->attr.precise) > + return -EINVAL; You're right that this is a user-visible change, and I'm pretty nervous about it to be honest with you. Perhaps a better way would be to expose something under sysfs, a bit like the caps directory for the SPE PMU, which identifies the fields of the attr structure that the driver does not ignore. I think doing this as an Arm-PMU specific thing initially would be fine, but it would be even better to have something where a driver can tell perf core about the parts it responds to and have this stuff populated automatically. The current design makes it inevitable that PMU drivers will have issues like the one you point out in the cover letter. Thoughts? Will
> On Wed, Jan 15, 2020 at 10:58:55AM +0000, James Clark wrote: > > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c > > index df352b334ea7..4ddbdb93b3b6 100644 > > --- a/drivers/perf/arm_pmu.c > > +++ b/drivers/perf/arm_pmu.c > > @@ -102,6 +102,9 @@ armpmu_map_event(struct perf_event *event, > > u64 config = event->attr.config; > > int type = event->attr.type; > > > > + if (event->attr.precise) > > + return -EINVAL; Also, does this field even exist? Guessing you mean 'precise_ip', but then that means this hasn't even seen a compiler :( Will
Hi Will, Yes you're right, I've somehow managed to post a version of the patch from before I built and tested it. I will repost it shortly with the other change split out as well. Thanks James On 17/01/2020 13:05, Will Deacon wrote: >> On Wed, Jan 15, 2020 at 10:58:55AM +0000, James Clark wrote: >>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c >>> index df352b334ea7..4ddbdb93b3b6 100644 >>> --- a/drivers/perf/arm_pmu.c >>> +++ b/drivers/perf/arm_pmu.c >>> @@ -102,6 +102,9 @@ armpmu_map_event(struct perf_event *event, >>> u64 config = event->attr.config; >>> int type = event->attr.type; >>> >>> + if (event->attr.precise) >>> + return -EINVAL; > > Also, does this field even exist? Guessing you mean 'precise_ip', but > then that means this hasn't even seen a compiler :( > > Will >
On Fri, Jan 17, 2020 at 12:39:21PM +0000, Will Deacon wrote: > Perhaps a better way would be to expose something under sysfs, a bit like > the caps directory for the SPE PMU, which identifies the fields of the attr > structure that the driver does not ignore. I think doing this as an Arm-PMU > specific thing initially would be fine, but it would be even better to have > something where a driver can tell perf core about the parts it responds to > and have this stuff populated automatically. The current design makes it > inevitable that PMU drivers will have issues like the one you point out in > the cover letter. > > Thoughts? We have PERF_PMU_CAP_ flags we could extend to that purpose.
Hi Peter,
Do you mean something like this?
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 43d1d4945433..f74acd085bea 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10812,6 +10812,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
goto err_pmu;
}
+ if (event->attr.precise_ip &&
+ !(pmu->capabilities & PERF_PMU_CAP_PRECISE_IP)) {
+ err = -EOPNOTSUPP;
+ goto err_pmu;
+ }
+
err = exclusive_event_init(event);
if (err)
goto err_pmu;
Or should it only be done via sysfs to not break userspace?
If this was done via sysfs would it be possible to express that some events support
some attributes and others don't? It seems like the "caps" folder couldn't be
that fine grained.
On 17/01/2020 14:01, Peter Zijlstra wrote:
> On Fri, Jan 17, 2020 at 12:39:21PM +0000, Will Deacon wrote:
>
>> Perhaps a better way would be to expose something under sysfs, a bit like
>> the caps directory for the SPE PMU, which identifies the fields of the attr
>> structure that the driver does not ignore. I think doing this as an Arm-PMU
>> specific thing initially would be fine, but it would be even better to have
>> something where a driver can tell perf core about the parts it responds to
>> and have this stuff populated automatically. The current design makes it
>> inevitable that PMU drivers will have issues like the one you point out in
>> the cover letter.
>>
>> Thoughts?
>
> We have PERF_PMU_CAP_ flags we could extend to that purpose.
>
On Fri, Jan 17, 2020 at 03:00:37PM +0000, James Clark wrote: > Hi Peter, > > Do you mean something like this? Yes. > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 43d1d4945433..f74acd085bea 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -10812,6 +10812,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu, > goto err_pmu; > } > > + if (event->attr.precise_ip && > + !(pmu->capabilities & PERF_PMU_CAP_PRECISE_IP)) { > + err = -EOPNOTSUPP; > + goto err_pmu; > + } > + > err = exclusive_event_init(event); > if (err) > goto err_pmu; > > > Or should it only be done via sysfs to not break userspace? So we've added checks like this in the past and gotten away with it. Do you already know of some userspace that will break due to it? An alternative approach is adding a sysctl like kernel.perf_nostrict which would disable this or something, that way 'old' userspace has a chicken bit.
Hi James,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on arm-soc/for-next]
[also build test ERROR on tip/perf/core linux/master linus/master v5.5-rc6 next-20200117]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/James-Clark/Return-EINVAL-when-precise_ip-perf-events-are-requested-on-Arm/20200116-195500
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git for-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers//perf/arm_pmu.c: In function 'armpmu_map_event':
>> drivers//perf/arm_pmu.c:105:18: error: 'struct perf_event_attr' has no member named 'precise'; did you mean 'precise_ip'?
if (event->attr.precise)
^~~~~~~
precise_ip
vim +105 drivers//perf/arm_pmu.c
92
93 int
94 armpmu_map_event(struct perf_event *event,
95 const unsigned (*event_map)[PERF_COUNT_HW_MAX],
96 const unsigned (*cache_map)
97 [PERF_COUNT_HW_CACHE_MAX]
98 [PERF_COUNT_HW_CACHE_OP_MAX]
99 [PERF_COUNT_HW_CACHE_RESULT_MAX],
100 u32 raw_event_mask)
101 {
102 u64 config = event->attr.config;
103 int type = event->attr.type;
104
> 105 if (event->attr.precise)
106 return -EINVAL;
107
108 if (type == event->pmu->type)
109 return armpmu_map_raw_event(raw_event_mask, config);
110
111 switch (type) {
112 case PERF_TYPE_HARDWARE:
113 return armpmu_map_hw_event(event_map, config);
114 case PERF_TYPE_HW_CACHE:
115 return armpmu_map_cache_event(cache_map, config);
116 case PERF_TYPE_RAW:
117 return armpmu_map_raw_event(raw_event_mask, config);
118 }
119
120 return -ENOENT;
121 }
122
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
On Fri, Jan 17, 2020 at 04:16:58PM +0100, Peter Zijlstra wrote: > On Fri, Jan 17, 2020 at 03:00:37PM +0000, James Clark wrote: > > Hi Peter, > > > > Do you mean something like this? > > Yes. > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index 43d1d4945433..f74acd085bea 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -10812,6 +10812,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu, > > goto err_pmu; > > } > > > > + if (event->attr.precise_ip && > > + !(pmu->capabilities & PERF_PMU_CAP_PRECISE_IP)) { > > + err = -EOPNOTSUPP; > > + goto err_pmu; > > + } > > + > > err = exclusive_event_init(event); > > if (err) > > goto err_pmu; > > > > > > Or should it only be done via sysfs to not break userspace? > > So we've added checks like this in the past and gotten away with it. Do > you already know of some userspace that will break due to it? > > An alternative approach is adding a sysctl like kernel.perf_nostrict > which would disable this or something, that way 'old' userspace has a > chicken bit. Could we allocate a "strict" bit from perf_event_attr::__reserved_1, and update drivers to expose a whitelist of fields they support? Then the core could do something like: if (attr->strict && !pmu_check_whitelist(pmu, attr)) return -EOPNOTSUPP; ... and we could also expose the whitelist somewhere in sysfs. Thanks, Mark,
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c index df352b334ea7..4ddbdb93b3b6 100644 --- a/drivers/perf/arm_pmu.c +++ b/drivers/perf/arm_pmu.c @@ -102,6 +102,9 @@ armpmu_map_event(struct perf_event *event, u64 config = event->attr.config; int type = event->attr.type; + if (event->attr.precise) + return -EINVAL; + if (type == event->pmu->type) return armpmu_map_raw_event(raw_event_mask, config); diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index 377d794d3105..3501b2eb168a 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -38,8 +38,8 @@ enum perf_type_id { }; /* - * Generalized performance event event_id types, used by the - * attr.event_id parameter of the sys_perf_event_open() + * Generalized hardware performance event types, used by the + * attr.config parameter of the sys_perf_event_open() * syscall: */ enum perf_hw_id {
ARM PMU events can be delivered with arbitrary skid, and there's nothing the kernel can do to prevent this. Given that, the PMU cannot support precise_ip != 0. Also update comment to state that attr.config field is used to set the event type rather than event_id which doesn't exist. Signed-off-by: James Clark <james.clark@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Tan Xiaojun <tanxiaojun@huawei.com> Cc: Al Grant <al.grant@arm.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/perf/arm_pmu.c | 3 +++ include/uapi/linux/perf_event.h | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-)