Re: [PATCH 4/6] perf tool: Parse general events from sysfs

[Jiri Olsa <jolsa@redhat.com>]

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