LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/1] Return EINVAL when precise_ip perf events are requested on Arm
@ 2020-01-15 10:58 James Clark
  2020-01-15 10:58 ` [PATCH 1/1] " James Clark
  0 siblings, 1 reply; 9+ messages in thread
From: James Clark @ 2020-01-15 10:58 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: nd, James Clark, Will Deacon, Mark Rutland, Peter Zijlstra,
	Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
	Jiri Olsa, Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm
  2020-01-15 10:58 [PATCH 0/1] Return EINVAL when precise_ip perf events are requested on Arm James Clark
@ 2020-01-15 10:58 ` " James Clark
  2020-01-17 12:39   ` Will Deacon
  2020-01-18 14:11   ` kbuild test robot
  0 siblings, 2 replies; 9+ messages in thread
From: James Clark @ 2020-01-15 10:58 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: nd, James Clark, Will Deacon, Mark Rutland, Peter Zijlstra,
	Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
	Jiri Olsa, Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm
  2020-01-15 10:58 ` [PATCH 1/1] " James Clark
@ 2020-01-17 12:39   ` Will Deacon
  2020-01-17 13:05     ` Will Deacon
  2020-01-17 14:01     ` Peter Zijlstra
  2020-01-18 14:11   ` kbuild test robot
  1 sibling, 2 replies; 9+ messages in thread
From: Will Deacon @ 2020-01-17 12:39 UTC (permalink / raw)
  To: James Clark
  Cc: linux-arm-kernel, nd, Mark Rutland, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
	Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm
  2020-01-17 12:39   ` Will Deacon
@ 2020-01-17 13:05     ` Will Deacon
  2020-01-17 13:11       ` James Clark
  2020-01-17 14:01     ` Peter Zijlstra
  1 sibling, 1 reply; 9+ messages in thread
From: Will Deacon @ 2020-01-17 13:05 UTC (permalink / raw)
  To: James Clark
  Cc: linux-arm-kernel, nd, Mark Rutland, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
	Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm
  2020-01-17 13:05     ` Will Deacon
@ 2020-01-17 13:11       ` James Clark
  0 siblings, 0 replies; 9+ messages in thread
From: James Clark @ 2020-01-17 13:11 UTC (permalink / raw)
  To: Will Deacon
  Cc: linux-arm-kernel, nd, Mark Rutland, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
	Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm
  2020-01-17 12:39   ` Will Deacon
  2020-01-17 13:05     ` Will Deacon
@ 2020-01-17 14:01     ` Peter Zijlstra
  2020-01-17 15:00       ` James Clark
  1 sibling, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2020-01-17 14:01 UTC (permalink / raw)
  To: Will Deacon
  Cc: James Clark, linux-arm-kernel, nd, Mark Rutland, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
	Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm
  2020-01-17 14:01     ` Peter Zijlstra
@ 2020-01-17 15:00       ` James Clark
  2020-01-17 15:16         ` Peter Zijlstra
  0 siblings, 1 reply; 9+ messages in thread
From: James Clark @ 2020-01-17 15:00 UTC (permalink / raw)
  To: Peter Zijlstra, Will Deacon
  Cc: linux-arm-kernel, nd, Mark Rutland, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
	Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm
  2020-01-17 15:00       ` James Clark
@ 2020-01-17 15:16         ` Peter Zijlstra
  0 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2020-01-17 15:16 UTC (permalink / raw)
  To: James Clark
  Cc: Will Deacon, linux-arm-kernel, nd, Mark Rutland, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
	Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm
  2020-01-15 10:58 ` [PATCH 1/1] " James Clark
  2020-01-17 12:39   ` Will Deacon
@ 2020-01-18 14:11   ` kbuild test robot
  1 sibling, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2020-01-18 14:11 UTC (permalink / raw)
  To: James Clark
  Cc: kbuild-all, linux-arm-kernel, nd, James Clark, Will Deacon,
	Mark Rutland, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
	Tan Xiaojun, Al Grant, Namhyung Kim, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 10:58 [PATCH 0/1] Return EINVAL when precise_ip perf events are requested on Arm James Clark
2020-01-15 10:58 ` [PATCH 1/1] " James Clark
2020-01-17 12:39   ` Will Deacon
2020-01-17 13:05     ` Will Deacon
2020-01-17 13:11       ` James Clark
2020-01-17 14:01     ` Peter Zijlstra
2020-01-17 15:00       ` James Clark
2020-01-17 15:16         ` Peter Zijlstra
2020-01-18 14:11   ` kbuild test robot

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git