[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20120424090427.GA2125@m.brq.redhat.com>
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