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

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

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.