Re: [PATCHv4 00/27] perf stat: Introduce --per-thread option

[Arnaldo Carvalho de Melo]

Em Tue, Jun 23, 2015 at 09:22:00AM +0200, Jiri Olsa escreveu:
> On Mon, Jun 22, 2015 at 08:06:00PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jun 23, 2015 at 12:36:01AM +0200, Jiri Olsa escreveu:
> > > adding the possibility to display stat data per thread.

> > > Allowing following commands and output:

> > >   $ perf stat  -e cycles,instructions --per-thread -p 30190,30242

> > While testing Adrian's Intel PT patchkit I realised we have --per-thread
> > in 'record', wonder if using a long option with the exact same name but
> > different meanings for 'stat' and 'record'  would cause confusion...
 
> I think the name fits for both stat and record.. and both are doing different

For record it is vague, for stat, it seems to fit.

For record it really should be --mmap-per-thread, but then we start
getting what may seem overly long options, but then, its an oddball
'record' option...

We have perf_evsel__open_per_thread(), that is used in builtin-stat.c,
and that means: Open a descriptor per thread, but then,
perf_target.per_thread only means, hey optimize things when setting up
the perf_event_attr, which so far means just use it as one of the things
to decide if PERF_SAMPLE_TIME should be set.

Just raising some concerns about consistency...

> thing, so both --per-thread are also different.. and well documented ;-)

The fact that something is documented doesn't avoid the initial
expectation that since it has the same name, and that its part of a
collection of tools, that it should have the same meaning, which means
maybe the existing option, being the oddball, should be changed to
better reflect what it asks the tool to do.

In fact, when we don't have to read documentation and things make sense,
that is the ideal, huh?

We should be more careful with this to avoid breaking too many
expectations when switching from one tool to another in the same
collection.
 
What others have to say? Ingo?

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/