[PATCH 0/1] Return EINVAL when precise_ip perf events are requested on Arm

[PATCH 0/1] Return EINVAL when precise_ip perf events are requested on Arm

[James Clark]

Hi,

Since we're adding support for SPE in user space Perf, we've encountered an issue
where we would like some more feedback if SPE isn't available for an event.

At the moment there is a patch for perf where you can enable SPE by doing this:

    perf record -r branch-misses:p ...

Perf will have a hard coded list of events that can use SPE when ":p" is specified
and open the SPE PMU instead of the specified one. But if the event isn't in that
list, then Perf will attempt to open the normal event with precise_ip = 1.
That will succeed at the moment, but we'd like the kernel to say it's not supported
so there is a chance of showing a warning to the user.

This isn't just relevant to Perf though, there may be other tools that are already
setting this.

Therefore I'm looking for feedback on whether this would break backwards
compatibility with user space tools that are already setting precise_ip and
expecting it to not error out on Arm.

This change would also be beneficial for the case where if in the (distant) future
we do add some kind of precise support, there will be a chance of userspace
determining what is supported and what isn't.

Thanks
James

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

James Clark (1):
  Return EINVAL when precise_ip perf events are requested on Arm

 drivers/perf/arm_pmu.c          | 3 +++
 include/uapi/linux/perf_event.h | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.24.0

[PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[James Clark]

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(-)

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 {
-- 
2.24.0

Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[Will Deacon]

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

Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[Will Deacon]

> 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

Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[James Clark]

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
> 

Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[Peter Zijlstra]

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.

Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[James Clark]

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.
> 

Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[Peter Zijlstra]

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.

Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72712 bytes --]

Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

[Mark Rutland]

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,