Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1174441 > unrolled thread

Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS Ingo Molnar <mingo@kernel.org> - 2015-06-30 13:00 +0200
    Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS Adrian Hunter <adrian.hunter@intel.com> - 2015-07-01 10:30 +0200
      Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS Ingo Molnar <mingo@kernel.org> - 2015-07-02 11:30 +0200
      Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS Ingo Molnar <mingo@kernel.org> - 2015-07-02 11:50 +0200
        Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS Adrian Hunter <adrian.hunter@intel.com> - 2015-07-02 14:40 +0200
          Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-07-02 15:10 +0200
          Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS Adrian Hunter <adrian.hunter@intel.com> - 2015-07-03 11:20 +0200
        Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-07-03 16:40 +0200

#1174441 — Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> > Yeah, so I did a 'newbie test':
> > 
> > I pulled the tree and saw that it has a tools/perf/Documentation/intel-bts.txt 
> > file and started reading it.
> > 
> > Based on its text:
> > 
> >   The Intel BTS kernel driver creates a new PMU for Intel BTS.  The perf record
> >   option is:
> > 
> >         -e intel_bts//
> > 
> >   Currently Intel BTS is limited to per-thread tracing so the --per-thread option
> >   is also needed.
> > 
> > I tried the following command which failed:
> > 
> >   triton:~/tip> perf record -e intel_bts// --per-thread sleep 1
> >   invalid or unsupported event: 'intel_bts//'
> >   Run 'perf list' for a list of valid events
> > 
> >    usage: perf record [<options>] [<command>]
> >       or: perf record [<options>] -- <command> [<options>]
> > 
> >       -e, --event <event>   event selector. use 'perf list' to list available events
> > 
> > That's a really ... unhelpful message. If I typoed something I want to know that. 
> > If the kernel does not support something, I want to know about that too. Tooling 
> > telling me: "maybe you typoed something, maybe it's not supported, I really don't 
> > care" is not very productive.
> 
> That is not entirely true. The message says "Run 'perf list' for a list of valid 
> events" which will tell you if the event is valid. So you can tell the 
> difference between a typo and unsupported event.

Yeah, but my point is: why doesn't the tool do this disambiguation for me? Tools 
are hard enough to use as-is already, no need to put artificial roadblocks in the 
path of first time users.

> > So this was with a distro kernel, and in the hope that I'm missing some magic 
> > new kernel feature, I tried it the latest -tip kernel, but it still gives me 
> > the same failure.
> > 
> > So the test newbie user got stuck after wasting some time.
> > 
> > Me as a kernel developer could probably figure it out, but that's not the 
> > point: if newbies cannot discover and use our new features then it's as if 
> > they didn't exist, and I'm not pulling non-existent features! ;-)
> > 
> > Could we please improve all this?
> 
> 'perf list' shows the event wasn't supported, so I am not sure what more the 
> "newbie" could expect.  Do you have any suggestions?

So I think a first time user would expect a clear message from the computer: what 
was wrong with what he wrote and what should he do to fix it.

Btw., here's the 'perf list' output from a system running the latest -tip kernel:

  vega:~> uname -a
  Linux vega 4.1.0-02935-g390ad45394a3-dirty #567 SMP PREEMPT Mon Jun 29 11:44:48 CEST 2015 x86_64 x86_64 x86_64 GNU/Linux
  vega:~> perf list | grep -i bts
  vega:~> 

so is there any kernel feature dependency? It's unclear. If yes, it should be 
mentioned in the document, and in the tooling output as well. If not then we have 
a bug somewhere.

I.e. you need to smooth the first time user's rocky path to first use as much as 
technically possible. Every single such helping step will literally double the 
number of users who will be able to successfully make use of the new feature.

As a positive example take a look at the newbie's road to 'perf trace':

  vega:~> trace
  Error:  No permissions to read /sys/kernel/debug/tracing/events/raw_syscalls/sys_(enter|exit)
  Hint:   Try 'sudo mount -o remount,mode=755 /sys/kernel/debug'

Aha, useful message, I need to run this as root:

  # trace

     0.000 ( 0.000 ms): sleep/28926  ... [continued]: nanosleep()) = 0
     0.051 ( 0.007 ms): sleep/28926 close(fd: 1                                                           ) = 0
     0.063 ( 0.005 ms): sleep/28926 close(fd: 2                                                           ) = 0
     0.072 ( 0.000 ms): sleep/28926 exit_group(                                      

Ok?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1175089

On 30/06/15 16:23, Adrian Hunter wrote:
> On 30/06/15 13:56, Ingo Molnar wrote:
>>
>> * Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>>>> Yeah, so I did a 'newbie test':
>>>>
>>>> I pulled the tree and saw that it has a tools/perf/Documentation/intel-bts.txt 
>>>> file and started reading it.
>>>>
>>>> Based on its text:
>>>>
>>>>   The Intel BTS kernel driver creates a new PMU for Intel BTS.  The perf record
>>>>   option is:
>>>>
>>>>         -e intel_bts//
>>>>
>>>>   Currently Intel BTS is limited to per-thread tracing so the --per-thread option
>>>>   is also needed.
>>>>
>>>> I tried the following command which failed:
>>>>
>>>>   triton:~/tip> perf record -e intel_bts// --per-thread sleep 1
>>>>   invalid or unsupported event: 'intel_bts//'
>>>>   Run 'perf list' for a list of valid events
>>>>
>>>>    usage: perf record [<options>] [<command>]
>>>>       or: perf record [<options>] -- <command> [<options>]
>>>>
>>>>       -e, --event <event>   event selector. use 'perf list' to list available events
>>>>
>>>> That's a really ... unhelpful message. If I typoed something I want to know that. 
>>>> If the kernel does not support something, I want to know about that too. Tooling 
>>>> telling me: "maybe you typoed something, maybe it's not supported, I really don't 
>>>> care" is not very productive.
>>>
>>> That is not entirely true. The message says "Run 'perf list' for a list of valid 
>>> events" which will tell you if the event is valid. So you can tell the 
>>> difference between a typo and unsupported event.
>>
>> Yeah, but my point is: why doesn't the tool do this disambiguation for me? Tools 
>> are hard enough to use as-is already, no need to put artificial roadblocks in the 
>> path of first time users.
> 
> That applies to all events e.g.
> 
> # perf record -e sched:sched_swotch sleep 1
> invalid or unsupported event: 'sched:sched_swotch'                                                                                       
> Run 'perf list' for a list of valid events                                                                                               
>                                                                                                                                          
>  usage: perf record [<options>] [<command>]                                                                                              
>     or: perf record [<options>] -- <command> [<options>]                                                                                 
>                                                                                                                                          
>     -e, --event <event>   event selector. use 'perf list' to list available events    
> 
> So it is a general problem.
> 
>>
>>>> So this was with a distro kernel, and in the hope that I'm missing some magic 
>>>> new kernel feature, I tried it the latest -tip kernel, but it still gives me 
>>>> the same failure.
>>>>
>>>> So the test newbie user got stuck after wasting some time.
>>>>
>>>> Me as a kernel developer could probably figure it out, but that's not the 
>>>> point: if newbies cannot discover and use our new features then it's as if 
>>>> they didn't exist, and I'm not pulling non-existent features! ;-)
>>>>
>>>> Could we please improve all this?
>>>
>>> 'perf list' shows the event wasn't supported, so I am not sure what more the 
>>> "newbie" could expect.  Do you have any suggestions?
>>
>> So I think a first time user would expect a clear message from the computer: what 
>> was wrong with what he wrote and what should he do to fix it.
>>
>> Btw., here's the 'perf list' output from a system running the latest -tip kernel:
>>
>>   vega:~> uname -a
>>   Linux vega 4.1.0-02935-g390ad45394a3-dirty #567 SMP PREEMPT Mon Jun 29 11:44:48 CEST 2015 x86_64 x86_64 x86_64 GNU/Linux
>>   vega:~> perf list | grep -i bts
>>   vega:~> 
>>
>> so is there any kernel feature dependency? It's unclear. If yes, it should be 
>> mentioned in the document, and in the tooling output as well. If not then we have 
>> a bug somewhere.
> 
> I am not aware of any dependencies, apart from perf events itself.
> 
> Are you sure you compiled perf tools with the new patches ;-)
> And it is an Intel CPU?
> 
>>
>> I.e. you need to smooth the first time user's rocky path to first use as much as 
>> technically possible. Every single such helping step will literally double the 
>> number of users who will be able to successfully make use of the new feature.
>>
>> As a positive example take a look at the newbie's road to 'perf trace':
>>
>>   vega:~> trace
>>   Error:  No permissions to read /sys/kernel/debug/tracing/events/raw_syscalls/sys_(enter|exit)
>>   Hint:   Try 'sudo mount -o remount,mode=755 /sys/kernel/debug'
>>
>> Aha, useful message, I need to run this as root:
>>
>>   # trace
>>
>>      0.000 ( 0.000 ms): sleep/28926  ... [continued]: nanosleep()) = 0
>>      0.051 ( 0.007 ms): sleep/28926 close(fd: 1                                                           ) = 0
>>      0.063 ( 0.005 ms): sleep/28926 close(fd: 2                                                           ) = 0
>>      0.072 ( 0.000 ms): sleep/28926 exit_group(                                      
>>
>> Ok?
> 
> Could do something like the following:
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 09f8d2357108..5ab8fee89361 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -666,8 +666,13 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
>  	struct perf_evsel *evsel;
>  
>  	pmu = perf_pmu__find(name);
> -	if (!pmu)
> +	if (!pmu) {
> +		if ((!strcmp(name, "intel_bts") || !strcmp(name, "intel_pt")) &&
> +		    data->error)
> +			if (asprintf(&data->error->str, "%s is not supported by the running kernel", name) < 0)
> +				return -ENOMEM;
>  		return -EINVAL;
> +	}
>  
>  	if (pmu->default_config) {
>  		memcpy(&attr, pmu->default_config,
> 
> Could then add checks for Intel hardware and bts CPU feature flag.

How is this?

From: Adrian Hunter <adrian.hunter@intel.com>
Date: Wed, 1 Jul 2015 11:14:50 +0300
Subject: [PATCH] perf tools: Add error messages for missing intel_bts and
 intel_pt support

Add error messages to assist users in determining why there is
no intel_bts or intel_pt support.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/arch/x86/util/header.c | 15 ++++++++++++++
 tools/perf/util/header.h          |  3 ++-
 tools/perf/util/parse-events.c    | 41 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
index 146d12a1cec0..afc5bdfd2d15 100644
--- a/tools/perf/arch/x86/util/header.c
+++ b/tools/perf/arch/x86/util/header.c
@@ -57,3 +57,18 @@ get_cpuid(char *buffer, size_t sz)
 	}
 	return -1;
 }
+
+int have_intel_cpu(void)
+{
+	char buffer[64];
+	int ret;
+
+	ret = get_cpuid(buffer, sizeof(buffer));
+	if (ret)
+		return -1;
+
+	if (!strncmp(buffer, "GenuineIntel,", 13))
+		return 1;
+
+	return 0;
+}
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index d4d57962c591..f6eab49d13d1 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -153,8 +153,9 @@ bool is_perf_magic(u64 magic);
 int write_padded(int fd, const void *bf, size_t count, size_t count_aligned);
 
 /*
- * arch specific callback
+ * arch specific callbacks
  */
 int get_cpuid(char *buffer, size_t sz);
+int have_intel_cpu(void);
 
 #endif /* __PERF_HEADER_H */
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 09f8d2357108..23fb777b40fa 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -656,6 +656,45 @@ static char *pmu_event_name(struct list_head *head_terms)
 	return NULL;
 }
 
+int __weak have_intel_cpu(void)
+{
+	return 0;
+}
+
+static int pmu_not_found_error(char *name, struct parse_events_error *err)
+{
+	int ret;
+
+	if (!err)
+		goto out;
+
+	if (!strcmp(name, "intel_bts")) {
+		ret = have_intel_cpu();
+		if (ret < 0)
+			goto out;
+		if (!ret) {
+			err->str = strdup("intel_bts requires an Intel CPU");
+			goto out;
+		}
+		err->str = strdup("kernel does not support intel_bts (requires 64-bit v4.1 kernel or later and BTS hardware support)");
+		goto out;
+	}
+
+	if (!strcmp(name, "intel_pt")) {
+		ret = have_intel_cpu();
+		if (ret < 0)
+			goto out;
+		if (!ret) {
+			err->str = strdup("intel_pt requires an Intel CPU (Core 5th generation or later)");
+			goto out;
+		}
+		err->str = strdup("kernel does not support intel_pt (requires v4.1 kernel or later and 5th generation Intel Core processor or later)");
+		goto out;
+	}
+out:
+	return -EINVAL;
+}
+
 int parse_events_add_pmu(struct parse_events_evlist *data,
 			 struct list_head *list, char *name,
 			 struct list_head *head_config)
@@ -667,7 +706,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 
 	pmu = perf_pmu__find(name);
 	if (!pmu)
-		return -EINVAL;
+		return pmu_not_found_error(name, data->error);
 
 	if (pmu->default_config) {
 		memcpy(&attr, pmu->default_config,
-- 
1.9.1



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1175867

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> I am going to need to know what hardware it is and cpu feature flags i.e.
> /proc/cpuinfo
> 
> Also ls /sys/bus/event_source/devices

Sorry about the delay, the merge window is busier than usual ...

So I tried it on:

  1) Intel box, old kernel, new tools: threw an error
  2)   AMD box, new kernel, new tools: threw an error
  3)   AMD box, old kernel, new tools: threw an error

failure was expected in all 3 cases, but it threw the exact same error with no 
real indication about what the problem was, while the real problem was different 
for each case:

  1)  good CPU,  bad kernel
  2)   bad CPU, good kernel
  3)   bad CPU,  bad kernel

I'd expect the 'good CPU, good kernel' combination to work:

  4) Intel box, new kernel, new tools

... but haven't tried it yet.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1175873

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> How is this?
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Wed, 1 Jul 2015 11:14:50 +0300
> Subject: [PATCH] perf tools: Add error messages for missing intel_bts and
>  intel_pt support
> 
> Add error messages to assist users in determining why there is
> no intel_bts or intel_pt support.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/arch/x86/util/header.c | 15 ++++++++++++++
>  tools/perf/util/header.h          |  3 ++-
>  tools/perf/util/parse-events.c    | 41 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> index 146d12a1cec0..afc5bdfd2d15 100644
> --- a/tools/perf/arch/x86/util/header.c
> +++ b/tools/perf/arch/x86/util/header.c
> @@ -57,3 +57,18 @@ get_cpuid(char *buffer, size_t sz)
>  	}
>  	return -1;
>  }
> +
> +int have_intel_cpu(void)
> +{
> +	char buffer[64];
> +	int ret;
> +
> +	ret = get_cpuid(buffer, sizeof(buffer));
> +	if (ret)
> +		return -1;
> +
> +	if (!strncmp(buffer, "GenuineIntel,", 13))
> +		return 1;
> +
> +	return 0;
> +}
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index d4d57962c591..f6eab49d13d1 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -153,8 +153,9 @@ bool is_perf_magic(u64 magic);
>  int write_padded(int fd, const void *bf, size_t count, size_t count_aligned);
>  
>  /*
> - * arch specific callback
> + * arch specific callbacks
>   */
>  int get_cpuid(char *buffer, size_t sz);
> +int have_intel_cpu(void);
>  
>  #endif /* __PERF_HEADER_H */
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 09f8d2357108..23fb777b40fa 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -656,6 +656,45 @@ static char *pmu_event_name(struct list_head *head_terms)
>  	return NULL;
>  }
>  
> +int __weak have_intel_cpu(void)
> +{
> +	return 0;
> +}
> +
> +static int pmu_not_found_error(char *name, struct parse_events_error *err)
> +{
> +	int ret;
> +
> +	if (!err)
> +		goto out;
> +
> +	if (!strcmp(name, "intel_bts")) {
> +		ret = have_intel_cpu();
> +		if (ret < 0)
> +			goto out;
> +		if (!ret) {
> +			err->str = strdup("intel_bts requires an Intel CPU");
> +			goto out;
> +		}
> +		err->str = strdup("kernel does not support intel_bts (requires 64-bit v4.1 kernel or later and BTS hardware support)");
> +		goto out;
> +	}
> +
> +	if (!strcmp(name, "intel_pt")) {
> +		ret = have_intel_cpu();
> +		if (ret < 0)
> +			goto out;
> +		if (!ret) {
> +			err->str = strdup("intel_pt requires an Intel CPU (Core 5th generation or later)");
> +			goto out;
> +		}
> +		err->str = strdup("kernel does not support intel_pt (requires v4.1 kernel or later and 5th generation Intel Core processor or later)");
> +		goto out;
> +	}
> +out:
> +	return -EINVAL;
> +}
> +
>  int parse_events_add_pmu(struct parse_events_evlist *data,
>  			 struct list_head *list, char *name,
>  			 struct list_head *head_config)
> @@ -667,7 +706,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
>  
>  	pmu = perf_pmu__find(name);
>  	if (!pmu)
> -		return -EINVAL;
> +		return pmu_not_found_error(name, data->error);
>  
>  	if (pmu->default_config) {
>  		memcpy(&attr, pmu->default_config,

So I really think we need an extended error reporting feature on the perf kernel 
side, so that a 'natural' error (plus a string) is reported back to tooling, 
instead of the current -EINVAL.

No need to do it for everything, doing it for BTS and related functionality would 
be a good first step to start this.

If you are interested you could try this, or I can try to write something (after 
the merge window).

So the idea would be to convert such opaque error returns:

        if (attr->config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
            !attr->freq && hwc->sample_period == 1) {
                /* BTS is not supported by this architecture. */
                if (!x86_pmu.bts_active)
                        return -EOPNOTSUPP;

into:

			return perf_err(event, -EOPNOTSUPP, "The BTS hardware feature is not available on this CPU.");

Where perf_err() is a function that on one hand returns -EOPNOTSUPP - so 'legacy' 
error handling works as before: the syscall will return -EOPNOTSUPP.

But if a new 'extended error reporting' feature bit is set in the perf_attr, then 
perf_err() also does the following:

 - it copies the error string either back to a user-space pointer which is in the 
   perf_attr (plus a max length field)

 - or an alternative implementation would be to puts the string into the event's 
   ring buffer, with a special (new) event marker - where it can be recovered by 
   tooling.

(I think the first approach is better, because it would work fine for events 
without ring-buffers as well.)

Old tooling won't have the feature flag set and won't have the string pointer in 
perf_attr, so nothing will happen on that case.

New tooling that supports 'extended error reporting' has the feature flag set and 
the kernel will copy the error string into the provided user-space string buffer, 
where tooling could use that string to generate more meaningful error messages:

  perf syscall error: The BTS hardware feature is not available on this CPU.

Does this make sense to you?

Now an additional complication is the fact that BTS can now also be a separate 
PMU, listed under /sys/bus/event_source/devices/intel_bts/.

If it's not listed there, we don't know the exact reason: is it not available 
because it's an old kernel? Or is it the wrong CPU?

We could solve that by extending the sysfs interface and adding an "error" file to 
the PMU directory: which would contain the reason why the driver was not created.

I.e. if the BTS driver was not created, we'd still have 
/sys/bus/event_source/devices/intel_bts/error (and no other file), which gives 
tooling a good way to discover why a particular PMU is not available. This adds a 
tiny bit more RAM overhead, but it's for the better I think, because tooling could 
be a lot more certain about what the capabilities of the kernel are.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1175943

On 02/07/15 12:43, Ingo Molnar wrote:
> 
> * Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
>> How is this?
>>
>> From: Adrian Hunter <adrian.hunter@intel.com>
>> Date: Wed, 1 Jul 2015 11:14:50 +0300
>> Subject: [PATCH] perf tools: Add error messages for missing intel_bts and
>>  intel_pt support
>>
>> Add error messages to assist users in determining why there is
>> no intel_bts or intel_pt support.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  tools/perf/arch/x86/util/header.c | 15 ++++++++++++++
>>  tools/perf/util/header.h          |  3 ++-
>>  tools/perf/util/parse-events.c    | 41 ++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 57 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
>> index 146d12a1cec0..afc5bdfd2d15 100644
>> --- a/tools/perf/arch/x86/util/header.c
>> +++ b/tools/perf/arch/x86/util/header.c
>> @@ -57,3 +57,18 @@ get_cpuid(char *buffer, size_t sz)
>>  	}
>>  	return -1;
>>  }
>> +
>> +int have_intel_cpu(void)
>> +{
>> +	char buffer[64];
>> +	int ret;
>> +
>> +	ret = get_cpuid(buffer, sizeof(buffer));
>> +	if (ret)
>> +		return -1;
>> +
>> +	if (!strncmp(buffer, "GenuineIntel,", 13))
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
>> index d4d57962c591..f6eab49d13d1 100644
>> --- a/tools/perf/util/header.h
>> +++ b/tools/perf/util/header.h
>> @@ -153,8 +153,9 @@ bool is_perf_magic(u64 magic);
>>  int write_padded(int fd, const void *bf, size_t count, size_t count_aligned);
>>  
>>  /*
>> - * arch specific callback
>> + * arch specific callbacks
>>   */
>>  int get_cpuid(char *buffer, size_t sz);
>> +int have_intel_cpu(void);
>>  
>>  #endif /* __PERF_HEADER_H */
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 09f8d2357108..23fb777b40fa 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -656,6 +656,45 @@ static char *pmu_event_name(struct list_head *head_terms)
>>  	return NULL;
>>  }
>>  
>> +int __weak have_intel_cpu(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int pmu_not_found_error(char *name, struct parse_events_error *err)
>> +{
>> +	int ret;
>> +
>> +	if (!err)
>> +		goto out;
>> +
>> +	if (!strcmp(name, "intel_bts")) {
>> +		ret = have_intel_cpu();
>> +		if (ret < 0)
>> +			goto out;
>> +		if (!ret) {
>> +			err->str = strdup("intel_bts requires an Intel CPU");
>> +			goto out;
>> +		}
>> +		err->str = strdup("kernel does not support intel_bts (requires 64-bit v4.1 kernel or later and BTS hardware support)");
>> +		goto out;
>> +	}
>> +
>> +	if (!strcmp(name, "intel_pt")) {
>> +		ret = have_intel_cpu();
>> +		if (ret < 0)
>> +			goto out;
>> +		if (!ret) {
>> +			err->str = strdup("intel_pt requires an Intel CPU (Core 5th generation or later)");
>> +			goto out;
>> +		}
>> +		err->str = strdup("kernel does not support intel_pt (requires v4.1 kernel or later and 5th generation Intel Core processor or later)");
>> +		goto out;
>> +	}
>> +out:
>> +	return -EINVAL;
>> +}
>> +
>>  int parse_events_add_pmu(struct parse_events_evlist *data,
>>  			 struct list_head *list, char *name,
>>  			 struct list_head *head_config)
>> @@ -667,7 +706,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
>>  
>>  	pmu = perf_pmu__find(name);
>>  	if (!pmu)
>> -		return -EINVAL;
>> +		return pmu_not_found_error(name, data->error);
>>  
>>  	if (pmu->default_config) {
>>  		memcpy(&attr, pmu->default_config,
> 
> So I really think we need an extended error reporting feature on the perf kernel 
> side, so that a 'natural' error (plus a string) is reported back to tooling, 
> instead of the current -EINVAL.
> 
> No need to do it for everything, doing it for BTS and related functionality would 
> be a good first step to start this.
> 
> If you are interested you could try this, or I can try to write something (after 
> the merge window).

Don't have time sorry.

> 
> So the idea would be to convert such opaque error returns:
> 
>         if (attr->config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
>             !attr->freq && hwc->sample_period == 1) {
>                 /* BTS is not supported by this architecture. */
>                 if (!x86_pmu.bts_active)
>                         return -EOPNOTSUPP;
> 
> into:
> 
> 			return perf_err(event, -EOPNOTSUPP, "The BTS hardware feature is not available on this CPU.");
> 
> Where perf_err() is a function that on one hand returns -EOPNOTSUPP - so 'legacy' 
> error handling works as before: the syscall will return -EOPNOTSUPP.
> 
> But if a new 'extended error reporting' feature bit is set in the perf_attr, then 
> perf_err() also does the following:
> 
>  - it copies the error string either back to a user-space pointer which is in the 
>    perf_attr (plus a max length field)
> 
>  - or an alternative implementation would be to puts the string into the event's 
>    ring buffer, with a special (new) event marker - where it can be recovered by 
>    tooling.
> 
> (I think the first approach is better, because it would work fine for events 
> without ring-buffers as well.)
> 
> Old tooling won't have the feature flag set and won't have the string pointer in 
> perf_attr, so nothing will happen on that case.
> 
> New tooling that supports 'extended error reporting' has the feature flag set and 
> the kernel will copy the error string into the provided user-space string buffer, 
> where tooling could use that string to generate more meaningful error messages:
> 
>   perf syscall error: The BTS hardware feature is not available on this CPU.
> 
> Does this make sense to you?

Normally the warts of syscalls are hidden by libraries.  I am not sure a
library couldn't do just as well with the advantage that it would work for
old kernels too.  A library could also have information about multiple
architectures, not just the one that is actually running e.g. your BTS
example won't work on ARM.

A library could provide a similar API to the one you described.  Either it
just does the syscall and returns, or if extended error information is
requested, it probes the API with various options, checks
perf_event_paranoid, and generally uses whatever information it can to best
figure out what went wrong.

> 
> Now an additional complication is the fact that BTS can now also be a separate 
> PMU, listed under /sys/bus/event_source/devices/intel_bts/.
> 
> If it's not listed there, we don't know the exact reason: is it not available 
> because it's an old kernel? Or is it the wrong CPU?
> 
> We could solve that by extending the sysfs interface and adding an "error" file to 
> the PMU directory: which would contain the reason why the driver was not created.
> 
> I.e. if the BTS driver was not created, we'd still have 
> /sys/bus/event_source/devices/intel_bts/error (and no other file), which gives 
> tooling a good way to discover why a particular PMU is not available. This adds a 
> tiny bit more RAM overhead, but it's for the better I think, because tooling could 
> be a lot more certain about what the capabilities of the kernel are.

That presumes the user understands what is or is not available on their
architecture, because on a non-x86 architecture they still get nothing
x86-specific.

It also runs a bit against the way config works e.g. even if the PMU driver
is config'ed out it still has to provide a sysfs interface.

So, isn't the patch I proposed sufficient for now?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1175959

Em Thu, Jul 02, 2015 at 03:35:17PM +0300, Adrian Hunter escreveu:
> On 02/07/15 12:43, Ingo Molnar wrote:
> > Old tooling won't have the feature flag set and won't have the string pointer in 
> > perf_attr, so nothing will happen on that case.
> > 
> > New tooling that supports 'extended error reporting' has the feature flag set and 
> > the kernel will copy the error string into the provided user-space string buffer, 
> > where tooling could use that string to generate more meaningful error messages:
> > 
> >   perf syscall error: The BTS hardware feature is not available on this CPU.
> > 
> > Does this make sense to you?
 
> Normally the warts of syscalls are hidden by libraries.  I am not sure a
> library couldn't do just as well with the advantage that it would work for
> old kernels too.  A library could also have information about multiple
> architectures, not just the one that is actually running e.g. your BTS
> example won't work on ARM.
 
> A library could provide a similar API to the one you described.  Either it
> just does the syscall and returns, or if extended error information is
> requested, it probes the API with various options, checks
> perf_event_paranoid, and generally uses whatever information it can to best
> figure out what went wrong.

Agreed, that is what tools/perf/util/evsel.c does, e.g.:

static struct {
        bool sample_id_all;
        bool exclude_guest;
        bool mmap2;
        bool cloexec;
        bool clockid;
        bool clockid_wrong;
} perf_missing_features;

And in __perf_evsel__open():

        /*
         * Must probe features in the order they were added to the
         * perf_event_attr interface.
         */
        if (!perf_missing_features.clockid_wrong && evsel->attr.use_clockid) {
                perf_missing_features.clockid_wrong = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.clockid && evsel->attr.use_clockid) {
                perf_missing_features.clockid = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
                perf_missing_features.cloexec = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {
                perf_missing_features.mmap2 = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.exclude_guest &&
                   (evsel->attr.exclude_guest || evsel->attr.exclude_host)) {
                perf_missing_features.exclude_guest = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.sample_id_all) {
                perf_missing_features.sample_id_all = true;
                goto retry_sample_id;
        }

So it does already probe for features available in the kernel and reacts
as best as imagined so far to kernels lacking those features.

The whole point here is: who writes the library? Those who need it to
support some feature they want to have merged?

We can use the existing infrastructure to the fullest extent possible,
but at some point we need to extend it, who does it?
 
> > Now an additional complication is the fact that BTS can now also be a separate 
> > PMU, listed under /sys/bus/event_source/devices/intel_bts/.

> > If it's not listed there, we don't know the exact reason: is it not available 
> > because it's an old kernel? Or is it the wrong CPU?

> > We could solve that by extending the sysfs interface and adding an "error" file to 
> > the PMU directory: which would contain the reason why the driver was not created.

> > I.e. if the BTS driver was not created, we'd still have 
> > /sys/bus/event_source/devices/intel_bts/error (and no other file), which gives 
> > tooling a good way to discover why a particular PMU is not available. This adds a 
> > tiny bit more RAM overhead, but it's for the better I think, because tooling could 
> > be a lot more certain about what the capabilities of the kernel are.
 
> That presumes the user understands what is or is not available on their
> architecture, because on a non-x86 architecture they still get nothing
> x86-specific.
 
> It also runs a bit against the way config works e.g. even if the PMU driver
> is config'ed out it still has to provide a sysfs interface.
 
> So, isn't the patch I proposed sufficient for now?

I haven't tested it so far, but it seems to improve things, given the
current infrastructure.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176425

On 02/07/15 22:00, Ingo Molnar wrote:
> 
> * Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
>>> So I really think we need an extended error reporting feature on the perf 
>>> kernel side, so that a 'natural' error (plus a string) is reported back to 
>>> tooling, instead of the current -EINVAL.
>>>
>>> No need to do it for everything, doing it for BTS and related functionality 
>>> would be a good first step to start this.
>>>
>>> If you are interested you could try this, or I can try to write something 
>>> (after the merge window).
>>
>> Don't have time sorry.
> 
> So who on the Intel side has time to finish Intel/PT support properly?
> 
> As a first step I think we need to disable the Intel/PT kernel side - it was
> merged with the express promise that proper tooling would come along promptly,
> but if that's not happening due to lack of resources, then the kernel side is
> obviously in limbo as well.

I am afraid I took your "If you are interested you could try this" literally.

So you are saying you will apply the patches if I develop the extended error
string feature for the perf_event_open syscall?

> 
>>> Does this make sense to you?
>>
>> Normally the warts of syscalls are hidden by libraries.  I am not sure a library 
>> couldn't do just as well with the advantage that it would work for old kernels 
>> too.  A library could also have information about multiple architectures, not 
>> just the one that is actually running e.g. your BTS example won't work on ARM.
> 
> The method I suggested works _both_ with old and new kernels, with the new kernel 
> simply giving better error feedback.
> 
> And these are not warts really, it's simply a threshold issue: above a certain 
> complexity of features it makes sense to introduce a qualitatively better error 
> reporting interface. When I tested Intel PT support I happened to go past that 
> threshold and it became obvious that we need it.
> 
>> A library could provide a similar API to the one you described.  Either it just 
>> does the syscall and returns, or if extended error information is requested, it 
>> probes the API with various options, checks perf_event_paranoid, and generally 
>> uses whatever information it can to best figure out what went wrong.
> 
> So I think eventually a proper libperf will crystalize out of perf's syscall 
> wrappers (I suggested it for a long time), but this has very little impact on the 
> end user who doesn't care whether it's done in perf, or in some intermediate 
> library.
> 
> So it can be done in the current perf code just as much - when libperf gets 
> introduced it will be factored out into tools/lib/perf/ without much problem and 
> then other tools can use it too.
> 
>>> I.e. if the BTS driver was not created, we'd still have 
>>> /sys/bus/event_source/devices/intel_bts/error (and no other file), which gives 
>>> tooling a good way to discover why a particular PMU is not available. This 
>>> adds a tiny bit more RAM overhead, but it's for the better I think, because 
>>> tooling could be a lot more certain about what the capabilities of the kernel 
>>> are.
>>
>> That presumes the user understands what is or is not available on their 
>> architecture, because on a non-x86 architecture they still get nothing 
>> x86-specific.
>>
>> It also runs a bit against the way config works e.g. even if the PMU driver is 
>> config'ed out it still has to provide a sysfs interface.
> 
> I partially agree - but it's really a problem that we currently only get two 
> states:
> 
>   - the PMU driver is there in sysfs
>   - the PMU driver is not there in sysfs
> 
> while we really want to a lot more about why it's not there - and it's the kernel 
> that knows this best, not tooling.
> 
> So maybe instead of having a separate directory, we could have a sysfs file that 
> listed all the PMU drivers that the kernel knows about, with a status line that 
> tells us whether they are:
> 
>   - not configured
>   - configured but runtime disabled due to reason X or Y or Z
>   - configured and enabled
> 
> OTOH having the directories with a single file in them is a close substitute, and 
> better meets the sysfs 'one file, one value' principle.

One file, one value can be done.

What about a directory:

	/sys/bus/event_source/known_pmus/

that contains a subdirectory for each pmu e.g.

	/sys/bus/event_source/known_pmus/intel_pt/

which contains at least 1 file:

	/sys/bus/event_source/known_pmus/intel_pt/status

That makes it easy to add more information by adding new files.

So the core would create and manage the sysfs directories and files.  The
default status would be "Not supported on this architecture". Arch and PMU
drivers would just need to tell the core what their status is.

> 
>> So, isn't the patch I proposed sufficient for now?
> 
> Well, no, because it really does the check in the wrong place and introduces 
> possible friction between what tooling thinks is the supported environment and 
> what the kernel thinks - and thus postpones the problem.
> 
> I could live with this current solution as the initial version if a subsequent 
> effort fixes it up properly by adding much better error reporting infrastructure, 
> but the 'no time' comment makes me doubt the whole Intel/PT feature set.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176621

Ingo Molnar <mingo@kernel.org> writes:

> So I really think we need an extended error reporting feature on the perf kernel 
> side, so that a 'natural' error (plus a string) is reported back to tooling, 
> instead of the current -EINVAL.
>
> No need to do it for everything, doing it for BTS and related functionality would 
> be a good first step to start this.
>
> If you are interested you could try this, or I can try to write something (after 
> the merge window).
>
> So the idea would be to convert such opaque error returns:
>
>         if (attr->config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
>             !attr->freq && hwc->sample_period == 1) {
>                 /* BTS is not supported by this architecture. */
>                 if (!x86_pmu.bts_active)
>                         return -EOPNOTSUPP;
>
> into:
>
> 			return perf_err(event, -EOPNOTSUPP, "The BTS hardware feature is not available on this CPU.");

So I poked around this a bit and came up with the patch below to give
this topic some more momentum.

Your average error will then be like this:

#define PERF_MODNAME "perf/x86"

...

               /* BTS is currently only allowed for user-mode. */
               if (!attr->exclude_kernel)
                       return perf_err(event, -EOPNOTSUPP,
                                       "BTS sampling not allowed for kernel space");

which in userspace will translate into:

---cut---
$ perf record -e branches:uk -c1 ls
kernel says: {
        "code": -95,
        "module": "perf/x86",
        "message": "BTS sampling not allowed for kernel space"
}

Error:
No hardware sampling interrupt available.
No APIC? If so then you can boot the kernel with the "lapic" boot parameter to force-enable it.
---cut---

The way I hacked it into tools/perf, it still prints out the old error
message (which, to prove the point once again, is neither here nor
there).

The patch below attaches the error to struct perf_event or copies it
directly to user's buffer in case if we don't have an event at the point
of error. Maybe a slightly easier way of doing it would be to strap it
to task_struct instead, but let's not stir the pot too much this time
around.

Also, to add an extra spin, the report is formatted as a JSON object so
that we could include both human-readable and machine-readable bits.

From a7de169ef2ca5425cd57286bfb61bfd5f8d15738 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Fri, 3 Jul 2015 17:12:54 +0300
Subject: [PATCH] perf: Introduce extended syscall error reporting

It has been pointed several times out that perf syscall error reporting
leaves a lot to be desired [1].

This patch introduces a fairly simple extension that allows call sites
to annotate their error codes with arbitrary strings, which will then
be copied to userspace (if they asked for it) along with the module
name that produced the error message in JSON format. This way, we can
provide both human-readable and machine-parsable information to user and
leave room for extensions in the future.

[1] http://marc.info/?l=linux-kernel&m=141470811013082

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/perf_event.h      | 38 ++++++++++++++++++++++++
 include/uapi/linux/perf_event.h |  8 ++++-
 kernel/events/core.c            | 66 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 108 insertions(+), 4 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1b82d44b0a..15bbef478f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -56,6 +56,37 @@ struct perf_guest_info_callbacks {
 #include <linux/cgroup.h>
 #include <asm/local.h>
 
+#ifndef PERF_MODNAME
+#define PERF_MODNAME KBUILD_MODNAME
+#endif
+
+/*
+ * Extended error reporting: annotate an error code with a string
+ * and a module name to help users diagnase problems with their
+ * attributes and whatnot.
+ */
+struct perf_err_site {
+	const char		*message;
+	const char		*owner;
+	const int		code;
+};
+
+#ifdef CONFIG_PERF_EVENTS
+
+#define __perf_err(__e, __c, __m) ({			\
+	static struct perf_err_site __err_site = {	\
+		.message	= (__m),		\
+		.owner		= PERF_MODNAME,		\
+		.code		= (__c),		\
+	};						\
+	(__e) = &__err_site;				\
+	(__c);						\
+})
+
+#define perf_err(__evt, __c, __m) ({ __perf_err((__evt)->error, (__c), (__m)); })
+
+#endif
+
 struct perf_callchain_entry {
 	__u64				nr;
 	__u64				ip[PERF_MAX_STACK_DEPTH];
@@ -447,6 +478,13 @@ struct perf_event {
 	struct list_head		owner_entry;
 	struct task_struct		*owner;
 
+	/*
+	 * Extended error reporting
+	 */
+	const struct perf_err_site	*error;
+	void __user			*error_buffer;
+	size_t				error_buffer_size;
+
 	/* mmap bits */
 	struct mutex			mmap_mutex;
 	atomic_t			mmap_count;
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index d97f84c080..d1ae1a079c 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -264,6 +264,7 @@ enum perf_event_read_format {
 					/* add: sample_stack_user */
 #define PERF_ATTR_SIZE_VER4	104	/* add: sample_regs_intr */
 #define PERF_ATTR_SIZE_VER5	112	/* add: aux_watermark */
+#define PERF_ATTR_SIZE_VER6	120	/* add: perf_err */
 
 /*
  * Hardware event_id to monitor via a performance monitoring event:
@@ -374,7 +375,12 @@ struct perf_event_attr {
 	 * Wakeup watermark for AUX area
 	 */
 	__u32	aux_watermark;
-	__u32	__reserved_2;	/* align to __u64 */
+
+	/*
+	 * Extended error reporting buffer
+	 */
+	__u32	perf_err_size;
+	__u64	perf_err;
 };
 
 #define perf_flags(attr)	(*(&(attr)->read_format + 1))
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8e13f3e54e..fd345c96d1 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -49,6 +49,60 @@
 
 #include <asm/irq_regs.h>
 
+static bool extended_reporting_enabled(struct perf_event_attr *attr)
+{
+	if (attr->size >= PERF_ATTR_SIZE_VER6 &&
+	    attr->perf_err_size > 0)
+		return true;
+
+	return false;
+}
+
+/*
+ * Provide a JSON formatted error report to the user if they asked for it.
+ */
+static void __perf_error_report(struct perf_event_attr *attr,
+				const struct perf_err_site *err_site)
+{
+	void *buffer;
+
+	if (!err_site || !extended_reporting_enabled(attr))
+		return;
+
+	buffer = kasprintf(GFP_KERNEL,
+			   "{\n"
+			   "\t\"code\": %d,\n"
+			   "\t\"module\": \"%s\",\n"
+			   "\t\"message\": \"%s\"\n"
+			   "}\n",
+			   err_site->code, err_site->owner, err_site->message);
+	if (!buffer)
+		return;
+
+	(void)copy_to_user((void __user *)attr->perf_err, buffer,
+			   attr->perf_err_size);
+	kfree(buffer);
+}
+
+/*
+ * Synchronous version of perf_err(), for the paths where we don't have
+ * an event.
+ */
+#define perf_err_attr(__attr, __c, __m) ({		\
+	struct perf_err_site *__site;			\
+	__perf_err(__site, (__c), (__m));		\
+	__perf_error_report(__attr, __site);		\
+	(__c);						\
+})
+
+/*
+ * Report an error before returning from a syscall
+ */
+static void perf_error_report(struct perf_event *event)
+{
+	__perf_error_report(&event->attr, event->error);
+}
+
 static struct workqueue_struct *perf_wq;
 
 typedef int (*remote_function_f)(void *);
@@ -3594,6 +3648,8 @@ static void _free_event(struct perf_event *event)
  */
 static void free_event(struct perf_event *event)
 {
+	perf_error_report(event);
+
 	if (WARN(atomic_long_cmpxchg(&event->refcount, 1, 0) != 1,
 				"unexpected event refcount: %ld; ptr=%p\n",
 				atomic_long_read(&event->refcount), event)) {
@@ -3661,6 +3717,8 @@ static void put_event(struct perf_event *event)
 	if (!atomic_long_dec_and_test(&event->refcount))
 		return;
 
+	perf_error_report(event);
+
 	if (!is_kernel_event(event))
 		perf_remove_from_owner(event);
 
@@ -7634,6 +7692,7 @@ err_ns:
 		perf_detach_cgroup(event);
 	if (event->ns)
 		put_pid_ns(event->ns);
+	perf_error_report(event);
 	kfree(event);
 
 	return ERR_PTR(err);
@@ -7912,15 +7971,16 @@ SYSCALL_DEFINE5(perf_event_open,
 
 	if (!attr.exclude_kernel) {
 		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
-			return -EACCES;
+			return perf_err_attr(&attr, -EACCES,
+					     "kernel tracing forbidden for the unprivileged");
 	}
 
 	if (attr.freq) {
 		if (attr.sample_freq > sysctl_perf_event_sample_rate)
-			return -EINVAL;
+			return perf_err_attr(&attr, -EINVAL, "sample_freq too high");
 	} else {
 		if (attr.sample_period & (1ULL << 63))
-			return -EINVAL;
+			return perf_err_attr(&attr, -EINVAL, "sample_period too high");
 	}
 
 	/*
-- 
2.1.4



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web