public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	linux-kernel@vger.kernel.org, Jiri Olsa <jolsa@redhat.com>,
	David Ahern <dsahern@gmail.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Stephane Eranian <eranian@google.com>,
	Arnaldo Carvalho de Melo <acme@redhat.com>
Subject: Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS
Date: Tue, 30 Jun 2015 12:56:48 +0200	[thread overview]
Message-ID: <20150630105648.GA25749@gmail.com> (raw)
In-Reply-To: <55924B2D.7000903@intel.com>


* 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.

> > 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.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?

Thanks,

	Ingo

  reply	other threads:[~2015-06-30 10:56 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-26 22:02 [GIT PULL 0/8] perf/pt -> Intel PT/BTS Arnaldo Carvalho de Melo
2015-06-26 22:02 ` [PATCH 1/8] perf auxtrace: Add Intel PT as an AUX area tracing type Arnaldo Carvalho de Melo
2015-06-26 22:02 ` [PATCH 2/8] perf tools: Add Intel PT packet decoder Arnaldo Carvalho de Melo
2015-06-26 22:02 ` [PATCH 3/8] perf tools: Add Intel PT instruction decoder Arnaldo Carvalho de Melo
2015-06-26 22:02 ` [PATCH 4/8] perf tools: Add Intel PT log Arnaldo Carvalho de Melo
2015-06-26 22:02 ` [PATCH 5/8] perf tools: Add Intel PT decoder Arnaldo Carvalho de Melo
2015-06-26 22:02 ` [PATCH 6/8] perf tools: Add Intel PT support Arnaldo Carvalho de Melo
2015-06-26 22:02 ` [PATCH 7/8] perf tools: Take Intel PT into use Arnaldo Carvalho de Melo
2015-06-26 22:02 ` [PATCH 8/8] perf tools: Add Intel BTS support Arnaldo Carvalho de Melo
2015-06-30  4:58 ` [GIT PULL 0/8] perf/pt -> Intel PT/BTS Ingo Molnar
2015-06-30  7:54   ` Adrian Hunter
2015-06-30 10:56     ` Ingo Molnar [this message]
2015-06-30 13:23       ` Adrian Hunter
2015-06-30 14:39         ` Arnaldo Carvalho de Melo
2015-07-01  8:19         ` Adrian Hunter
2015-07-01 12:47           ` Adrian Hunter
2015-07-02  9:28             ` Ingo Molnar
2015-07-02  9:43           ` Ingo Molnar
2015-07-02 12:35             ` Adrian Hunter
2015-07-02 13:02               ` Arnaldo Carvalho de Melo
2015-07-02 19:00               ` Ingo Molnar
2015-07-03  9:08                 ` Adrian Hunter
2015-07-03 14:31             ` Alexander Shishkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150630105648.GA25749@gmail.com \
    --to=mingo@kernel.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=acme@kernel.org \
    --cc=acme@redhat.com \
    --cc=adrian.hunter@intel.com \
    --cc=dsahern@gmail.com \
    --cc=eranian@google.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=namhyung@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox