On 30/06/15 13:56, Ingo Molnar wrote:
>
> * Adrian Hunter <adrian.hun...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
