lkml.org 
[lkml]   [2012]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/6] perf tool: Parse general events from sysfs
On Wed, Apr 25, 2012 at 01:47:46PM +0800, Yan, Zheng wrote:
> On 04/24/2012 05:04 PM, Jiri Olsa wrote:
> > hi,
> > so the point is to have an alias support for pmu event definition.
> > Seems like good idea to improve usability/readability, I have some
> > general comments though..
> >
> Thank you very much for reviewing this.
> > - you suggest to have sysfs files having contents like:
> > event=0x2c,umask=0xf
> > when we were adding the formats stuff into sysfs, we had to cut off
> > the sysfs file contents to bare minimum to obey the sysfs rule:
> > single file = single value
> > so you might have some troubles pushing that through.. not sure ;)
> >
> > - I haven't read the whole patchset, but seems like the "events"
> > directory is now specific to a 'Intel uncore pmu'. If thats the
> > case I think there should be generic way for each pmu to define
> > this stuff.
> >
> > - as for the tools/pmu.c change I'd like to see more consistent
> > way of parsing this, than via 'newcfg' variable.. but none
> > is comming to me so far ;) I'll think about that..
> >
> For this point, how about making the parser re-entrantable like the patch
> attached below does.

yep, looks cleaner ;) some comments below

>
> Regards
> Yan, Zheng
>
> ---
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index e98e14c..a8b47d2 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -728,11 +728,13 @@ $(OUTPUT)perf.o perf.spec \
> # These two need to be here so that when O= is not used they take precedence

SNIP

> +static int __parse_events(const char *str, int *idx, struct list_head *list);
>
> #define CHW(x) .type = PERF_TYPE_HARDWARE, .config = PERF_COUNT_HW_##x
> #define CSW(x) .type = PERF_TYPE_SOFTWARE, .config = PERF_COUNT_SW_##x
> @@ -651,8 +653,11 @@ int parse_events_add_numeric(struct list_head *list, int *idx,
> int parse_events_add_pmu(struct list_head *list, int *idx,
> char *name, struct list_head *head_config)
> {
> + LIST_HEAD(event);

Are there all available events under ...event_source/devices/%s/events or
just some of them ?

In case of the latter, maybe the 'aliases/alias' would be better name for
that.. ending up with rename in this source ;)


> struct perf_event_attr attr;
> struct perf_pmu *pmu;
> + char *config;
> + int ret;
>
> pmu = perf_pmu__find(name);
> if (!pmu)
> @@ -666,10 +671,21 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
> */
> config_attr(&attr, head_config, 0);
>
> - if (perf_pmu__config(pmu, &attr, head_config))
> - return -EINVAL;
> + ret = perf_pmu__config(pmu, &attr, head_config);
> + if (!ret)
> + return add_event(list, idx, &attr, (char *) "pmu");
> +
> + ret = perf_pmu__alias(pmu, &config, head_config);
> + if (ret)
> + return ret;


I think it's ok for pmu object to contain list of aliases loaded
from sysfs (if that change will go throught ;) ), but I'd rather
see the pmu alias interface to be more generic like:

config = perf_pmu__alias(pmu, alias);

The function would just return the event config string if the record
is found for the alias.

...and the parse_events_add_pmu function would then take care of
the rest.. the final 'pmu/config' string and have it parsed...


thanks,
jirka


\
 
 \ /
  Last update: 2012-04-25 15:05    [from the cache]
©2003-2011 Jasper Spaans