On 30/06/15 16:23, Adrian Hunter wrote:
> 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.
How is this?
From: Adrian Hunter <adrian.hun...@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.hun...@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 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/
