From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752596AbbF3N03 (ORCPT ); Tue, 30 Jun 2015 09:26:29 -0400 Received: from mga11.intel.com ([192.55.52.93]:3693 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752873AbbF3N0U (ORCPT ); Tue, 30 Jun 2015 09:26:20 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,377,1432623600"; d="scan'208";a="755840155" Message-ID: <55929861.3050000@intel.com> Date: Tue, 30 Jun 2015 16:23:45 +0300 From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Ingo Molnar CC: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , David Ahern , Namhyung Kim , Peter Zijlstra , Stephane Eranian , Arnaldo Carvalho de Melo Subject: Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS References: <1435356140-31212-1-git-send-email-acme@kernel.org> <20150630045852.GB31981@gmail.com> <55924B2D.7000903@intel.com> <20150630105648.GA25749@gmail.com> In-Reply-To: <20150630105648.GA25749@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 30/06/15 13:56, Ingo Molnar wrote: > > * Adrian Hunter 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 [] [] >>> or: perf record [] -- [] >>> >>> -e, --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 [] [] or: perf record [] -- [] -e, --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.