Date:	Tue, 24 Apr 2012 11:04:27 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	"Yan, Zheng" <zheng.z.yan@...el.com>
Cc:	a.p.zijlstra@...llo.nl, mingo@...e.hu, andi@...stfloor.org,
	eranian@...gle.com, linux-kernel@...r.kernel.org,
	ming.m.lin@...el.com
Subject: Re: [PATCH 4/6] perf tool: Parse general events from sysfs

On Sun, Apr 01, 2012 at 09:41:33AM +0800, Yan, Zheng wrote:
> From: "Yan, Zheng" <zheng.z.yan@...el.com>
> 
> PMU can export general events to sysfs, for example:
> 
> /sys/bus/event_source/devices/uncore_nhm/events

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..

- 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..

wbr,
jirka


> └── CLOCKTICKS
> 
> Then we can specify the event as "<pmu>/<event>/", for example:
> 
> perf stat -a -C 0 -e "uncore_nhm/CLOCKTICKS/" sleep 1
> 
> Signed-off-by: Zheng Yan <zheng.z.yan@...el.com>
> ---
>  tools/perf/util/parse-events.c |   24 ++++++--
>  tools/perf/util/parse-events.h |    6 +-
>  tools/perf/util/parse-events.y |    6 +-
>  tools/perf/util/pmu.c          |  140 +++++++++++++++++++++++++++++++++++++++-
>  tools/perf/util/pmu.h          |   11 +++-
>  5 files changed, 172 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 5b3a0ef..fdac544 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -24,7 +24,7 @@ struct event_symbol {
>  };
>  
>  int parse_events_parse(struct list_head *list, struct list_head *list_tmp,
> -		       int *idx);
> +			int *idx, char **newcfg);
>  
>  #define CHW(x) .type = PERF_TYPE_HARDWARE, .config = PERF_COUNT_HW_##x
>  #define CSW(x) .type = PERF_TYPE_SOFTWARE, .config = PERF_COUNT_SW_##x
> @@ -648,8 +648,8 @@ int parse_events_add_numeric(struct list_head *list, int *idx,
>  			 (char *) __event_name(type, config));
>  }
>  
> -int parse_events_add_pmu(struct list_head *list, int *idx,
> -			 char *name, struct list_head *head_config)
> +int parse_events_add_pmu(struct list_head *list, char **newcfg,
> +			int *idx, char *name, struct list_head *head_config)
>  {
>  	struct perf_event_attr attr;
>  	struct perf_pmu *pmu;
> @@ -666,9 +666,12 @@ 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))
> +	if (perf_pmu__config(pmu, head_config, &attr, newcfg))
>  		return -EINVAL;
>  
> +	if (newcfg && *newcfg)
> +		return 0;
> +
>  	return add_event(list, idx, &attr, (char *) "pmu");
>  }
>  
> @@ -753,14 +756,25 @@ int parse_events(struct perf_evlist *evlist, const char *str, int unset __used)
>  	LIST_HEAD(list_tmp);
>  	YY_BUFFER_STATE buffer;
>  	int ret, idx = evlist->nr_entries;
> +	char *newcfg = NULL;
>  
>  	buffer = parse_events__scan_string(str);
>  
> -	ret = parse_events_parse(&list, &list_tmp, &idx);
> +	ret = parse_events_parse(&list, &list_tmp, &idx, &newcfg);
>  
>  	parse_events__flush_buffer(buffer);
>  	parse_events__delete_buffer(buffer);
>  
> +	if (!ret && newcfg) {
> +		buffer = parse_events__scan_string(newcfg);
> +
> +		ret = parse_events_parse(&list, &list_tmp, &idx, NULL);
> +
> +		parse_events__flush_buffer(buffer);
> +		parse_events__delete_buffer(buffer);
> +		free(newcfg);
> +	}
> +
>  	if (!ret) {
>  		int entries = idx - evlist->nr_entries;
>  		perf_evlist__splice_list_tail(evlist, &list, entries);
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index ca069f8..24924fd 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -74,13 +74,13 @@ int parse_events_add_cache(struct list_head *list, int *idx,
>  			   char *type, char *op_result1, char *op_result2);
>  int parse_events_add_breakpoint(struct list_head *list, int *idx,
>  				void *ptr, char *type);
> -int parse_events_add_pmu(struct list_head *list, int *idx,
> -			 char *pmu , struct list_head *head_config);
> +int parse_events_add_pmu(struct list_head *list, char **newcfg,
> +			 int *idx, char *pmu , struct list_head *head_config);
>  void parse_events_update_lists(struct list_head *list_event,
>  			       struct list_head *list_all);
>  void parse_events_error(struct list_head *list_all,
>  			struct list_head *list_event,
> -			int *idx, char const *msg);
> +			int *idx, char **newcfg, char const *msg);
>  
>  void print_events(const char *event_glob);
>  void print_events_type(u8 type);
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index d9637da..5f569f2 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -3,6 +3,7 @@
>  %parse-param {struct list_head *list_all}
>  %parse-param {struct list_head *list_event}
>  %parse-param {int *idx}
> +%parse-param {char **newcfg}
>  
>  %{
>  
> @@ -82,7 +83,7 @@ event_def: event_pmu |
>  event_pmu:
>  PE_NAME '/' event_config '/'
>  {
> -	ABORT_ON(parse_events_add_pmu(list_event, idx, $1, $3));
> +	ABORT_ON(parse_events_add_pmu(list_event, newcfg, idx, $1, $3));
>  	parse_events__free_terms($3);
>  }
>  
> @@ -194,7 +195,7 @@ PE_NAME
>  {
>  	struct parse_events__term *term;
>  
> -	ABORT_ON(parse_events__new_term(&term, PARSE_EVENTS__TERM_TYPE_NUM,
> +	ABORT_ON(parse_events__new_term(&term, PARSE_EVENTS__TERM_TYPE_STR,
>  		 $1, NULL, 1));
>  	$$ = term;
>  }
> @@ -224,6 +225,7 @@ sep_slash_dc: '/' | ':' |
>  void parse_events_error(struct list_head *list_all __used,
>  			struct list_head *list_event __used,
>  			int *idx __used,
> +			char **newcfg __used,
>  			char const *msg __used)
>  {
>  }
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index cb08a11..71244b6 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -80,6 +80,89 @@ static int pmu_format(char *name, struct list_head *format)
>  	return 0;
>  }
>  
> +static int perf_pmu__new_event(struct list_head *list, char *name, FILE *file)
> +{
> +	struct perf_pmu__event *event;
> +	char buf[256];
> +	int ret;
> +
> +	ret = fread(buf, 1, sizeof(buf), file);
> +	if (ret == 0)
> +		return -EINVAL;
> +
> +	event = zalloc(sizeof(*event));
> +	if (!event)
> +		return -ENOMEM;
> +
> +	event->name = strdup(name);
> +	event->config = strndup(buf, ret);
> +
> +	list_add_tail(&event->list, list);
> +	return 0;
> +}
> +
> +/*
> + * Process all the sysfs attributes located under the directory
> + * specified in 'dir' parameter.
> + */
> +static int pmu_events_parse(char *dir, struct list_head *head)
> +{
> +	struct dirent *evt_ent;
> +	DIR *event_dir;
> +	int ret = 0;
> +
> +	event_dir = opendir(dir);
> +	if (!event_dir)
> +		return -EINVAL;
> +
> +	while (!ret && (evt_ent = readdir(event_dir))) {
> +		char path[PATH_MAX];
> +		char *name = evt_ent->d_name;
> +		FILE *file;
> +
> +		if (!strcmp(name, ".") || !strcmp(name, ".."))
> +			continue;
> +
> +		snprintf(path, PATH_MAX, "%s/%s", dir, name);
> +
> +		ret = -EINVAL;
> +		file = fopen(path, "r");
> +		if (!file)
> +			break;
> +		ret = perf_pmu__new_event(head, name, file);
> +		fclose(file);
> +	}
> +
> +	closedir(event_dir);
> +	return ret;
> +}
> +
> +/*
> + * Reading the pmu event definition, which should be located at:
> + * /sys/bus/event_source/devices/<dev>/events as sysfs group attributes.
> + */
> +static int pmu_events(char *name, struct list_head *events)
> +{
> +	struct stat st;
> +	char path[PATH_MAX];
> +	const char *sysfs;
> +
> +	sysfs = sysfs_find_mountpoint();
> +	if (!sysfs)
> +		return -1;
> +
> +	snprintf(path, PATH_MAX,
> +		 "%s/bus/event_source/devices/%s/events", sysfs, name);
> +
> +	if (stat(path, &st) < 0)
> +		return -1;
> +
> +	if (pmu_events_parse(path, events))
> +		return -1;
> +
> +	return 0;
> +}
> +
>  /*
>   * Reading/parsing the default pmu type value, which should be
>   * located at:
> @@ -118,6 +201,7 @@ static struct perf_pmu *pmu_lookup(char *name)
>  {
>  	struct perf_pmu *pmu;
>  	LIST_HEAD(format);
> +	LIST_HEAD(events);
>  	__u32 type;
>  
>  	/*
> @@ -135,8 +219,12 @@ static struct perf_pmu *pmu_lookup(char *name)
>  	if (!pmu)
>  		return NULL;
>  
> +	pmu_events(name, &events);
> +
>  	INIT_LIST_HEAD(&pmu->format);
> +	INIT_LIST_HEAD(&pmu->events);
>  	list_splice(&format, &pmu->format);
> +	list_splice(&events, &pmu->events);
>  	pmu->name = strdup(name);
>  	pmu->type = type;
>  	return pmu;
> @@ -262,16 +350,62 @@ static int pmu_config(struct list_head *formats, struct perf_event_attr *attr,
>  	return 0;
>  }
>  
> +
> +static struct perf_pmu__event*
> +pmu_find_event(struct list_head *events, char *name)
> +{
> +	struct perf_pmu__event *event;
> +
> +	list_for_each_entry(event, events, list)
> +		if (!strcmp(event->name, name))
> +			return event;
> +
> +	return NULL;
> +}
> +
> +static int pmu_event(struct perf_pmu *pmu, struct list_head *head_terms,
> +		     char **newcfg)
> +{
> +	struct parse_events__term *term;
> +	struct perf_pmu__event *event;
> +	char *buf;
> +
> +	if (!list_is_singular(head_terms))
> +		return -EINVAL;
> +
> +	term = list_entry(head_terms->next, struct parse_events__term, list);
> +
> +	if (term->type != PARSE_EVENTS__TERM_TYPE_STR || term->val.str)
> +		return -EINVAL;
> +
> +	event = pmu_find_event(&pmu->events, term->config);
> +	if (!event)
> +		return -EINVAL;
> +
> +	buf = malloc(strlen(pmu->name) + strlen(event->config) + 3);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	sprintf(buf, "%s/%s/", pmu->name, event->config);
> +	*newcfg = buf;
> +
> +	return 0;
> +}
> +
>  /*
>   * Configures event's 'attr' parameter based on the:
>   * 1) users input - specified in terms parameter
>   * 2) pmu format definitions - specified by pmu parameter
>   */
> -int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
> -		     struct list_head *head_terms)
> +int perf_pmu__config(struct perf_pmu *pmu, struct list_head *head_terms,
> +		     struct perf_event_attr *attr, char **newcfg)
>  {
> +	int ret;
>  	attr->type = pmu->type;
> -	return pmu_config(&pmu->format, attr, head_terms);
> +	ret = pmu_config(&pmu->format, attr, head_terms);
> +	if (ret == -EINVAL && newcfg)
> +		ret = pmu_event(pmu, head_terms, newcfg);
> +	return ret;
>  }
>  
>  int perf_pmu__new_format(struct list_head *list, char *name,
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 68c0db9..2052b3f 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -19,16 +19,23 @@ struct perf_pmu__format {
>  	struct list_head list;
>  };
>  
> +struct perf_pmu__event {
> +	char *name;
> +	char *config;
> +	struct list_head list;
> +};
> +
>  struct perf_pmu {
>  	char *name;
>  	__u32 type;
>  	struct list_head format;
> +	struct list_head events;
>  	struct list_head list;
>  };
>  
>  struct perf_pmu *perf_pmu__find(char *name);
> -int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
> -		     struct list_head *head_terms);
> +int perf_pmu__config(struct perf_pmu *pmu, struct list_head *head_terms,
> +		     struct perf_event_attr *attr, char **newcfg);
>  
>  int perf_pmu_wrap(void);
>  void perf_pmu_error(struct list_head *list, char *name, char const *msg);
> -- 
> 1.7.7.6
> 
> --
> 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/
--
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