Re: [PATCH 0/4] perf events: Add realtime clock event and timehistoption -v2

[Frederic Weisbecker]

On Mon, Feb 21, 2011 at 06:16:41PM -0700, David Ahern wrote:
> 
> 
> On 02/21/11 15:21, Frederic Weisbecker wrote:
> >> All of the changes to perf-report are related strictly to this feature -
> >> generating the timestamp and printing the sample including walking the
> >> callchain.
> > 
> > This is the actual drawback: it's only useful for your feature. My wish
> > is to have something more broadly useful. And support for callchains or
> > other things like this in perf script is desired and has been requested
> > by the past.
> 
> I don't agree with the conclusion. The ability to take a realtime-clock
> sample + a perf_clock time stamps and prepend it to a dump of perf
> samples is not limited to perf-report. To date, it's the only command
> setup to use it - or maybe a better phrasing is that it is the command I
> most use today.

Sure it's useful. But it doesn't integrate well.

With your proposition we have two seperate tools to look at events
sequentially: perf report to look at wall clock, normal clock, ip and sym.
And perf script to look at normal clock, pid, cpu and trace.

This split is simply a mistake. How tracing users are going to join
both outputs?

This is even more sad if you consider that the events that most need to
be displayed sequentially are the tracepoints. Sure we want to treat every
events the same, but if tracepoints traces can't be added up with
these new features, your thing is just half useful.

I don't see any good reason for that.

 
> perf-script is new. I created this timehist patch back in August and
> submitted it to linux-perf-users at the end of November. The version I
> sent today is the 3rd incarnation.

perf-script is not new, it was there in november already, although named
perf trace at that time.

> 
> If it is a question of code location then I can move the function
> additions from builtin-report.c to util/timehist.c. Then as other
> commands are ready they can pick it up.

It's not a question of file, it's a question on where it integrates
logically and if it does coherently.

> > 
> >> perf-script needs to have features added to it:
> >>   1. working with all samples,
> > 
> > Why do you need that? You seem to be only interested in tracepoint
> > events.
> 
> I am *not* interested in tracepoints at all -- at least so far. I have
> rarely used them, mainly just trying out the options to see what is
> available and how much data comes pouring in. (tracepoints are not
> available on the kernel version we use.)

Fine, that doesn't change much anyway. If you follow my idea on passing
the whole event to the process_event() callback, there is no big things
to change compared to integrate that to perf report.

For event that don't have raw trace, just don't print raw trace, that's
the only difference.

I'm not even asking you to handle that for scripts, the python and perf
part can ignore that. It just needs to be handled for the print_event()
case.

It's barely a bit more in your diff compared to the perf-report
case.


> Rather, I have been focused on the S/W events such as the context-switch
> event and H/W counters. In fact, one request I have received on my end
> is for a time history of H/W counters - record periodically during
> specific tests and dump the samples with timestamps.

Fine.

> > 
> > Sure I would appreciate that perf script can support any event as a bonus
> > but that doesn't seem mandatory here.
> > 
> >>   2. support for callchains,
> > 
> > What does it take more than what you did in perf report, namely
> > calling perf_session_resolve_callchain and walking the cursor?
> 
> It's not rocket science, nonetheless its code that needs to be brought
> into perf-script.

No difference compared to perf report integration then.

> >>   3. more?
> > 
> > ?
> 
> There always seems to be unexpected gotchas that have to be dealt with.

Sure, I suspect we are not talking about hundred of lines of code though.
--
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/