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

[Adrian Hunter <adrian.hunter@intel.com>]

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