Message-ID: <20100311124608.GG31354@elte.hu>
Date:	Thu, 11 Mar 2010 13:46:08 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Corey Ashford <cjashfor@...ux.vnet.ibm.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [RFC] [PATCH 1/1] perf: add support for arch-dependent symbolic
 event names to "perf stat"


* Corey Ashford <cjashfor@...ux.vnet.ibm.com> wrote:

> On 3/3/2010 6:30 PM, Corey Ashford wrote:
> >For your review, this patch adds support for arch-dependent symbolic
> >event names to the "perf stat" tool, and could be expanded to other
> >"perf *" commands fairly easily, I suspect.
> >
> >To support arch-dependent event names without adding arch-dependent code
> >to perf, I added a callout mechanism whereby perf will look for the
> >environment variable: PERF_ARCH_DEP_LIB, and if it exists, it will try
> >to open it as a shared object. If that succeeds, it looks for the symbol
> >"parse_arch_dep_event". If that exists, that function will be called by
> >parse_events() before all of the other event parsing functions in
> >parse-events.c. It is passed the same arguments as the other
> >parse_*_event functions, namely the event string and a pointer to an
> >event attribute structure.
> >
> >As the code existed, "perf stat" would print out the count results, but
> >for raw events (which is how arch-dependent events are supported in
> >perf_events), it would just print out a raw code. This is not
> >acceptable, especially when a symbolic name was placed on the command
> >line. So I changed the code to save away the event name that was passed
> >on the command line, rather than doing a reverse translation to an event
> >string based on the event type and config fields of the attr structure.
> >In this way, there's no need for a reverse translation function in the
> >arch-dependent library; only a event string->attr struct function is
> >needed.
> >
> >I could well be missing something, but I don't understand why reverse
> >translation is ever needed in perf, as long as the tool keeps track of
> >the original event strings.
> 
> A couple of follow-up comments on this patch:
> 
> This functionality was designed to provide a generalized interface to an 
> external event name -> attr struct library, such as libpfm4. libpfm4 has an 
> interface that nearly exactly matches parse_*_event() profiles, so it's 
> quite easy to write a small wrapper function to call libpfm4's function.
> 
> Ingo Molnar discussed adding some visibility to the arch-dependent event 
> names through some other interface, such as through /sys/devices/pmus 
> perhaps, but that discussion is a long way (as far as I know) from having 
> something usable today.  So you could think of this external library 
> approach to be a stop-gap until something better is developed.  When/if that 
> new event naming mechanism becomes available, we can easily remove this 
> external library support from perf.

I'm quite much against stop-gap measures like this - they tend to become 
tomorrow's impossible-to-remove quirk.

If you want extensible events you can already do it by providing an ftrace 
tracepoint event via TRACE_EVENT. They are easy to add and ad-hoc, and are 
supported throughout by perf.

That could be librarized further by providing an /eventfs or /proc/events 
interface to enumerate them.

Or if you want to extend the perf events namespace ABI you can send patches 
for that as well. (It's not a big issue if a particular event is currently 
only supported on Power for example - as long as you make a good effort naming 
and structuring it in a reasonably generic way.)

Thanks,

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

Powered by blists - more mailing lists