* [PATCH V6 1/3] Revert "perf tools: Default to cpu// for events v5"
2014-09-11 19:08 [PATCH V6 0/3] perf tools: pmu event new style format fix kan.liang
@ 2014-09-11 19:08 ` kan.liang
2014-09-11 19:08 ` [PATCH V6 2/3] perf tools: parse the pmu event prefix and surfix kan.liang
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: kan.liang @ 2014-09-11 19:08 UTC (permalink / raw)
To: acme, jolsa; +Cc: linux-kernel, ak, Kan Liang
From: Kan Liang <kan.liang@intel.com>
This reverts commit 50e200f07948 ("perf tools: Default to cpu// for
events v5")
The fixup cannot handle the case that
new style format(which without //) mixed with
other different formats.
For example,
group events with new style format: {mem-stores,mem-loads}
some hardware event + new style event: cycles,mem-loads
Cache event + new style event: LLC-loads,mem-loads
Raw event + new style event:
cpu/event=0xc8,umask=0x08/,mem-loads
old style event and new stytle mixture: mem-stores,cpu/mem-loads/
Signed-off-by: Kan Liang <kan.liang@intel.com>
---
tools/perf/util/include/linux/string.h | 1 -
tools/perf/util/parse-events.c | 30 +-----------------------------
tools/perf/util/string.c | 24 ------------------------
3 files changed, 1 insertion(+), 54 deletions(-)
diff --git a/tools/perf/util/include/linux/string.h b/tools/perf/util/include/linux/string.h
index 97a8007..6f19c54 100644
--- a/tools/perf/util/include/linux/string.h
+++ b/tools/perf/util/include/linux/string.h
@@ -1,4 +1,3 @@
#include <string.h>
void *memdup(const void *src, size_t len);
-int str_append(char **s, int *len, const char *a);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 61be3e6..b9174bc 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -6,7 +6,7 @@
#include "parse-options.h"
#include "parse-events.h"
#include "exec_cmd.h"
-#include "linux/string.h"
+#include "string.h"
#include "symbol.h"
#include "cache.h"
#include "header.h"
@@ -864,32 +864,6 @@ int parse_events_name(struct list_head *list, char *name)
return 0;
}
-static int parse_events__scanner(const char *str, void *data, int start_token);
-
-static int parse_events_fixup(int ret, const char *str, void *data,
- int start_token)
-{
- char *o = strdup(str);
- char *s = NULL;
- char *t = o;
- char *p;
- int len = 0;
-
- if (!o)
- return ret;
- while ((p = strsep(&t, ",")) != NULL) {
- if (s)
- str_append(&s, &len, ",");
- str_append(&s, &len, "cpu/");
- str_append(&s, &len, p);
- str_append(&s, &len, "/");
- }
- free(o);
- if (!s)
- return -ENOMEM;
- return parse_events__scanner(s, data, start_token);
-}
-
static int parse_events__scanner(const char *str, void *data, int start_token)
{
YY_BUFFER_STATE buffer;
@@ -910,8 +884,6 @@ static int parse_events__scanner(const char *str, void *data, int start_token)
parse_events__flush_buffer(buffer, scanner);
parse_events__delete_buffer(buffer, scanner);
parse_events_lex_destroy(scanner);
- if (ret && !strchr(str, '/'))
- ret = parse_events_fixup(ret, str, data, start_token);
return ret;
}
diff --git a/tools/perf/util/string.c b/tools/perf/util/string.c
index 2553e5b..4b0ff22 100644
--- a/tools/perf/util/string.c
+++ b/tools/perf/util/string.c
@@ -387,27 +387,3 @@ void *memdup(const void *src, size_t len)
return p;
}
-
-/**
- * str_append - reallocate string and append another
- * @s: pointer to string pointer
- * @len: pointer to len (initialized)
- * @a: string to append.
- */
-int str_append(char **s, int *len, const char *a)
-{
- int olen = *s ? strlen(*s) : 0;
- int nlen = olen + strlen(a) + 1;
- if (*len < nlen) {
- *len = *len * 2;
- if (*len < nlen)
- *len = nlen;
- *s = realloc(*s, *len);
- if (!*s)
- return -ENOMEM;
- if (olen == 0)
- **s = 0;
- }
- strcat(*s, a);
- return 0;
-}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH V6 2/3] perf tools: parse the pmu event prefix and surfix
2014-09-11 19:08 [PATCH V6 0/3] perf tools: pmu event new style format fix kan.liang
2014-09-11 19:08 ` [PATCH V6 1/3] Revert "perf tools: Default to cpu// for events v5" kan.liang
@ 2014-09-11 19:08 ` kan.liang
2014-09-14 13:23 ` Jiri Olsa
2014-09-11 19:08 ` [PATCH V6 3/3] perf tools: Add support to new style format of kernel PMU event kan.liang
2014-09-14 13:23 ` [PATCH V6 0/3] perf tools: pmu event new style format fix Jiri Olsa
3 siblings, 1 reply; 10+ messages in thread
From: kan.liang @ 2014-09-11 19:08 UTC (permalink / raw)
To: acme, jolsa; +Cc: linux-kernel, ak, Kan Liang
From: Kan Liang <kan.liang@intel.com>
There are two types of event formats for PMU events. E.g. el-abort OR
cpu/el-abort/. However, the lexer mistakenly recognizes the simple style
format as two events.
The parse_events_pmu_check function uses bsearch to search the name in
known pmu event list. It can tell the lexer that the name is a PE_NAME
or a PMU event name prefix or a PMU event name suffix. All these
information will be used for accurately parsing kernel PMU events.
The pmu events list will be read from sysfs at runtime.
Note: Currently, the patch only want to handle the PMU event name as
"a-b" and "a". The only exception, "stalled-cycles-frontend" and
"stalled-cycles-fronted", are already hardcoded in lexer.
Signed-off-by: Kan Liang <kan.liang@intel.com>
---
tools/perf/util/parse-events.c | 112 +++++++++++++++++++++++++++++++++++++++++
tools/perf/util/parse-events.h | 14 ++++++
tools/perf/util/pmu.c | 10 ----
tools/perf/util/pmu.h | 10 ++++
4 files changed, 136 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index b9174bc..20e01d1 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -30,6 +30,9 @@ extern int parse_events_debug;
#endif
int parse_events_parse(void *data, void *scanner);
+static struct perf_pmu_event_symbol *perf_pmu_events_list;
+static int perf_pmu_events_list_num;
+
static struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
[PERF_COUNT_HW_CPU_CYCLES] = {
.symbol = "cpu-cycles",
@@ -864,6 +867,114 @@ int parse_events_name(struct list_head *list, char *name)
return 0;
}
+static int
+comp_pmu(const void *p1, const void *p2)
+{
+ struct perf_pmu_event_symbol *pmu1 =
+ (struct perf_pmu_event_symbol *) p1;
+ struct perf_pmu_event_symbol *pmu2 =
+ (struct perf_pmu_event_symbol *) p2;
+
+ return strcmp(pmu1->symbol, pmu2->symbol);
+}
+
+/*
+ * Read the pmu events list from sysfs
+ * Save it into perf_pmu_events_list
+ */
+static void perf_pmu__parse_init(void)
+{
+
+ struct perf_pmu *pmu = NULL;
+ struct perf_pmu_alias *alias;
+ int len = 0;
+
+ pmu = perf_pmu__find("cpu");
+ if ((pmu == NULL) || list_empty(&pmu->aliases)) {
+ perf_pmu_events_list_num = -1;
+ return;
+ }
+ list_for_each_entry(alias, &pmu->aliases, list) {
+ if (strchr(alias->name, '-'))
+ len++;
+ len++;
+ }
+ perf_pmu_events_list =
+ malloc(sizeof(struct perf_pmu_event_symbol) * len);
+ perf_pmu_events_list_num = len;
+
+ len = 0;
+ pmu = perf_pmu__find("cpu");
+ list_for_each_entry(alias, &pmu->aliases, list) {
+ struct perf_pmu_event_symbol *p =
+ perf_pmu_events_list + len;
+ char *tmp = strchr(alias->name, '-');
+
+ if (tmp != NULL) {
+ p->symbol = malloc(tmp - alias->name + 1);
+ strlcpy(p->symbol, alias->name,
+ tmp - alias->name + 1);
+ p->type = KERNEL_PMU_EVENT_PREFIX;
+ tmp++;
+ p++;
+ p->symbol = malloc(strlen(tmp) + 1);
+ strcpy(p->symbol, tmp);
+ p->type = KERNEL_PMU_EVENT_SUFFIX;
+ len += 2;
+ } else {
+ p->symbol = malloc(strlen(alias->name) + 1);
+ strcpy(p->symbol, alias->name);
+ p->type = KERNEL_PMU_EVENT;
+ len++;
+ }
+ }
+ qsort(perf_pmu_events_list, len,
+ sizeof(struct perf_pmu_event_symbol), comp_pmu);
+}
+
+static void perf_pmu__parse_cleanup(void)
+{
+ if (perf_pmu_events_list_num > 0) {
+ struct perf_pmu_event_symbol *p;
+ int i;
+
+ for (i = 0; i < perf_pmu_events_list_num; i++) {
+ p = perf_pmu_events_list + i;
+ free(p->symbol);
+ }
+ free(perf_pmu_events_list);
+ perf_pmu_events_list = NULL;
+ perf_pmu_events_list_num = 0;
+ }
+}
+
+enum perf_pmu_event_symbol_type
+perf_pmu__parse_check(const char *name)
+{
+ struct perf_pmu_event_symbol p, *r;
+
+ /* scan kernel pmu events from sysfs if needed */
+ if (perf_pmu_events_list_num == 0)
+ perf_pmu__parse_init();
+ /*
+ * name "cpu" could be prefix of cpu-cycles or cpu// events.
+ * cpu-cycles has been handled by hardcode.
+ * So it must be cpu// events, not kernel pmu event.
+ */
+ if ((perf_pmu_events_list_num <= 0) || !strcmp(name, "cpu"))
+ return NONE_KERNEL_PMU_EVENT;
+
+ p.symbol = malloc(strlen(name) + 1);
+ strcpy(p.symbol, name);
+ r = bsearch(&p, perf_pmu_events_list,
+ (size_t) perf_pmu_events_list_num,
+ sizeof(struct perf_pmu_event_symbol), comp_pmu);
+ free(p.symbol);
+ if (r == NULL)
+ return NONE_KERNEL_PMU_EVENT;
+ return r->type;
+}
+
static int parse_events__scanner(const char *str, void *data, int start_token)
{
YY_BUFFER_STATE buffer;
@@ -918,6 +1029,7 @@ int parse_events(struct perf_evlist *evlist, const char *str)
int ret;
ret = parse_events__scanner(str, &data, PE_START_EVENTS);
+ perf_pmu__parse_cleanup();
if (!ret) {
int entries = data.idx - evlist->nr_entries;
perf_evlist__splice_list_tail(evlist, &data.list, entries);
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index df094b4..9f064e4 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -35,6 +35,18 @@ extern int parse_filter(const struct option *opt, const char *str, int unset);
#define EVENTS_HELP_MAX (128*1024)
+enum perf_pmu_event_symbol_type {
+ NONE_KERNEL_PMU_EVENT, /* not a PMU EVENT */
+ KERNEL_PMU_EVENT, /* normal style PMU event */
+ KERNEL_PMU_EVENT_PREFIX, /* prefix of pre-suf style event */
+ KERNEL_PMU_EVENT_SUFFIX, /* suffix of pre-suf style event */
+};
+
+struct perf_pmu_event_symbol {
+ char *symbol;
+ enum perf_pmu_event_symbol_type type;
+};
+
enum {
PARSE_EVENTS__TERM_TYPE_NUM,
PARSE_EVENTS__TERM_TYPE_STR,
@@ -95,6 +107,8 @@ 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);
+enum perf_pmu_event_symbol_type
+perf_pmu__parse_check(const char *name);
void parse_events__set_leader(char *name, struct list_head *list);
void parse_events_update_lists(struct list_head *list_event,
struct list_head *list_all);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 22a4ad5..f358345 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -12,16 +12,6 @@
#include "parse-events.h"
#include "cpumap.h"
-#define UNIT_MAX_LEN 31 /* max length for event unit name */
-
-struct perf_pmu_alias {
- char *name;
- struct list_head terms; /* HEAD struct parse_events_term -> list */
- struct list_head list; /* ELEM */
- char unit[UNIT_MAX_LEN+1];
- double scale;
-};
-
struct perf_pmu_format {
char *name;
int value;
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 0f5c0a8..2020519 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -25,6 +25,16 @@ struct perf_pmu {
struct list_head list; /* ELEM */
};
+#define UNIT_MAX_LEN 31 /* max length for event unit name */
+
+struct perf_pmu_alias {
+ char *name;
+ struct list_head terms; /* HEAD struct parse_events_term -> list */
+ struct list_head list; /* ELEM */
+ char unit[UNIT_MAX_LEN+1];
+ double scale;
+};
+
struct perf_pmu *perf_pmu__find(const char *name);
int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
struct list_head *head_terms);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V6 2/3] perf tools: parse the pmu event prefix and surfix
2014-09-11 19:08 ` [PATCH V6 2/3] perf tools: parse the pmu event prefix and surfix kan.liang
@ 2014-09-14 13:23 ` Jiri Olsa
2014-10-02 17:33 ` Liang, Kan
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2014-09-14 13:23 UTC (permalink / raw)
To: kan.liang; +Cc: acme, linux-kernel, ak
On Thu, Sep 11, 2014 at 03:08:58PM -0400, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
typo in subject - s/surfix/suffix/
SNIP
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index b9174bc..20e01d1 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -30,6 +30,9 @@ extern int parse_events_debug;
> #endif
> int parse_events_parse(void *data, void *scanner);
>
> +static struct perf_pmu_event_symbol *perf_pmu_events_list;
> +static int perf_pmu_events_list_num;
please make some comment for the perf_pmu_events_list_num in here,
saying that 0 means 'ready to init', -1 means 'failed, dont try anymore',
and > 0 means 'initialized'
> +
> static struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
> [PERF_COUNT_HW_CPU_CYCLES] = {
> .symbol = "cpu-cycles",
> @@ -864,6 +867,114 @@ int parse_events_name(struct list_head *list, char *name)
> return 0;
> }
>
> +static int
> +comp_pmu(const void *p1, const void *p2)
> +{
> + struct perf_pmu_event_symbol *pmu1 =
> + (struct perf_pmu_event_symbol *) p1;
> + struct perf_pmu_event_symbol *pmu2 =
> + (struct perf_pmu_event_symbol *) p2;
please keep it on one line, like:
const struct perf_pmu_event_symbol *pmu1 = p1;
const struct perf_pmu_event_symbol *pmu2 = p2;
> +
> + return strcmp(pmu1->symbol, pmu2->symbol);
> +}
> +
> +/*
> + * Read the pmu events list from sysfs
> + * Save it into perf_pmu_events_list
> + */
> +static void perf_pmu__parse_init(void)
> +{
> +
> + struct perf_pmu *pmu = NULL;
> + struct perf_pmu_alias *alias;
> + int len = 0;
> +
> + pmu = perf_pmu__find("cpu");
> + if ((pmu == NULL) || list_empty(&pmu->aliases)) {
> + perf_pmu_events_list_num = -1;
> + return;
> + }
> + list_for_each_entry(alias, &pmu->aliases, list) {
> + if (strchr(alias->name, '-'))
> + len++;
> + len++;
> + }
> + perf_pmu_events_list =
> + malloc(sizeof(struct perf_pmu_event_symbol) * len);
please keep above on one line
> + perf_pmu_events_list_num = len;
> +
> + len = 0;
> + pmu = perf_pmu__find("cpu");
no need to lookup 'cpu' pmu again
> + list_for_each_entry(alias, &pmu->aliases, list) {
> + struct perf_pmu_event_symbol *p =
> + perf_pmu_events_list + len;
please keep above on one line
> + char *tmp = strchr(alias->name, '-');
> +
> + if (tmp != NULL) {
> + p->symbol = malloc(tmp - alias->name + 1);
> + strlcpy(p->symbol, alias->name,
> + tmp - alias->name + 1);
> + p->type = KERNEL_PMU_EVENT_PREFIX;
> + tmp++;
> + p++;
> + p->symbol = malloc(strlen(tmp) + 1);
> + strcpy(p->symbol, tmp);
> + p->type = KERNEL_PMU_EVENT_SUFFIX;
> + len += 2;
> + } else {
> + p->symbol = malloc(strlen(alias->name) + 1);
> + strcpy(p->symbol, alias->name);
> + p->type = KERNEL_PMU_EVENT;
> + len++;
I can see pattern above and missing check for failed malloc
how about using strdup and macro like:
...
#define SET_SYMBOL(str, type) \
do { \
p->symbol = str; \
if (!p->symbol) \
goto err; \
p->type = type; \
} while (0)
if (tmp != NULL) {
SET_SYMBOL(strndup(alias->name, tmp - alias->name + 1), KERNEL_PMU_EVENT_PREFIX);
p++;
SET_SYMBOL(strdup(++tmp), KERNEL_PMU_EVENT_SUFFIX);
len += 2;
} else {
SET_SYMBOL(strdup(alias->name), KERNEL_PMU_EVENT);
}
...
err:
perf_pmu__parse_cleanup
> + }
> + }
> + qsort(perf_pmu_events_list, len,
> + sizeof(struct perf_pmu_event_symbol), comp_pmu);
> +}
> +
> +static void perf_pmu__parse_cleanup(void)
> +{
> + if (perf_pmu_events_list_num > 0) {
> + struct perf_pmu_event_symbol *p;
> + int i;
> +
> + for (i = 0; i < perf_pmu_events_list_num; i++) {
> + p = perf_pmu_events_list + i;
> + free(p->symbol);
> + }
> + free(perf_pmu_events_list);
> + perf_pmu_events_list = NULL;
> + perf_pmu_events_list_num = 0;
> + }
> +}
> +
> +enum perf_pmu_event_symbol_type
> +perf_pmu__parse_check(const char *name)
> +{
> + struct perf_pmu_event_symbol p, *r;
> +
> + /* scan kernel pmu events from sysfs if needed */
> + if (perf_pmu_events_list_num == 0)
> + perf_pmu__parse_init();
> + /*
> + * name "cpu" could be prefix of cpu-cycles or cpu// events.
> + * cpu-cycles has been handled by hardcode.
> + * So it must be cpu// events, not kernel pmu event.
> + */
> + if ((perf_pmu_events_list_num <= 0) || !strcmp(name, "cpu"))
> + return NONE_KERNEL_PMU_EVENT;
> +
> + p.symbol = malloc(strlen(name) + 1);
> + strcpy(p.symbol, name);
please use strdup and check for error,
but I think you could use name directly no? like:
p.symbol = name;
> + r = bsearch(&p, perf_pmu_events_list,
> + (size_t) perf_pmu_events_list_num,
> + sizeof(struct perf_pmu_event_symbol), comp_pmu);
> + free(p.symbol);
> + if (r == NULL)
> + return NONE_KERNEL_PMU_EVENT;
> + return r->type;
return r ? r->type : NONE_KERNEL_PMU_EVENT;
> +}
> +
> static int parse_events__scanner(const char *str, void *data, int start_token)
> {
> YY_BUFFER_STATE buffer;
> @@ -918,6 +1029,7 @@ int parse_events(struct perf_evlist *evlist, const char *str)
> int ret;
>
> ret = parse_events__scanner(str, &data, PE_START_EVENTS);
> + perf_pmu__parse_cleanup();
> if (!ret) {
> int entries = data.idx - evlist->nr_entries;
> perf_evlist__splice_list_tail(evlist, &data.list, entries);
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index df094b4..9f064e4 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -35,6 +35,18 @@ extern int parse_filter(const struct option *opt, const char *str, int unset);
>
> #define EVENTS_HELP_MAX (128*1024)
>
> +enum perf_pmu_event_symbol_type {
> + NONE_KERNEL_PMU_EVENT, /* not a PMU EVENT */
> + KERNEL_PMU_EVENT, /* normal style PMU event */
> + KERNEL_PMU_EVENT_PREFIX, /* prefix of pre-suf style event */
> + KERNEL_PMU_EVENT_SUFFIX, /* suffix of pre-suf style event */
> +};
maybe following enum names go better with the enum name:
PMU_EVENT_SYMBOL_ERR
PMU_EVENT_SYMBOL
PMU_EVENT_SYMBOL_PREFIX
PMU_EVENT_SYMBOL_SUFFIX
> +
> +struct perf_pmu_event_symbol {
> + char *symbol;
> + enum perf_pmu_event_symbol_type type;
> +};
> +
> enum {
> PARSE_EVENTS__TERM_TYPE_NUM,
> PARSE_EVENTS__TERM_TYPE_STR,
> @@ -95,6 +107,8 @@ 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);
> +enum perf_pmu_event_symbol_type
> +perf_pmu__parse_check(const char *name);
> void parse_events__set_leader(char *name, struct list_head *list);
> void parse_events_update_lists(struct list_head *list_event,
> struct list_head *list_all);
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 22a4ad5..f358345 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -12,16 +12,6 @@
> #include "parse-events.h"
> #include "cpumap.h"
>
> -#define UNIT_MAX_LEN 31 /* max length for event unit name */
> -
> -struct perf_pmu_alias {
> - char *name;
> - struct list_head terms; /* HEAD struct parse_events_term -> list */
> - struct list_head list; /* ELEM */
> - char unit[UNIT_MAX_LEN+1];
> - double scale;
> -};
> -
> struct perf_pmu_format {
> char *name;
> int value;
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 0f5c0a8..2020519 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -25,6 +25,16 @@ struct perf_pmu {
> struct list_head list; /* ELEM */
> };
>
> +#define UNIT_MAX_LEN 31 /* max length for event unit name */
> +
> +struct perf_pmu_alias {
> + char *name;
> + struct list_head terms; /* HEAD struct parse_events_term -> list */
> + struct list_head list; /* ELEM */
> + char unit[UNIT_MAX_LEN+1];
> + double scale;
> +};
> +
> struct perf_pmu *perf_pmu__find(const char *name);
> int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
> struct list_head *head_terms);
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V6 2/3] perf tools: parse the pmu event prefix and surfix
2014-09-14 13:23 ` Jiri Olsa
@ 2014-10-02 17:33 ` Liang, Kan
2014-10-03 8:23 ` Jiri Olsa
0 siblings, 1 reply; 10+ messages in thread
From: Liang, Kan @ 2014-10-02 17:33 UTC (permalink / raw)
To: 'Jiri Olsa'; +Cc: acme, linux-kernel, ak
> >
> > +static int
> > +comp_pmu(const void *p1, const void *p2) {
> > + struct perf_pmu_event_symbol *pmu1 =
> > + (struct perf_pmu_event_symbol *) p1;
> > + struct perf_pmu_event_symbol *pmu2 =
> > + (struct perf_pmu_event_symbol *) p2;
>
> please keep it on one line, like:
> const struct perf_pmu_event_symbol *pmu1 = p1;
> const struct perf_pmu_event_symbol *pmu2 = p2;
>
Removing (struct perf_pmu_event_symbol *) will cause compiler error as below.
util/parse-events.c:877:39: error: initialization discards âconstâ qualifier from pointer target type [-Werror]
struct perf_pmu_event_symbol *pmu1 = p1;
If we keep (struct perf_pmu_event_symbol *), we may not keep it on one line. The line will over 80 characters. There will be error from checkpatch.pl.
> > + perf_pmu_events_list =
> > + malloc(sizeof(struct perf_pmu_event_symbol) * len);
>
> please keep above on one line
If so, the line will over 80 characters. There will be error from checkpatch.pl.
Kan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V6 2/3] perf tools: parse the pmu event prefix and surfix
2014-10-02 17:33 ` Liang, Kan
@ 2014-10-03 8:23 ` Jiri Olsa
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Olsa @ 2014-10-03 8:23 UTC (permalink / raw)
To: Liang, Kan; +Cc: acme, linux-kernel, ak, Ingo Molnar
On Thu, Oct 02, 2014 at 05:33:07PM +0000, Liang, Kan wrote:
>
>
> > >
> > > +static int
> > > +comp_pmu(const void *p1, const void *p2) {
> > > + struct perf_pmu_event_symbol *pmu1 =
> > > + (struct perf_pmu_event_symbol *) p1;
> > > + struct perf_pmu_event_symbol *pmu2 =
> > > + (struct perf_pmu_event_symbol *) p2;
> >
> > please keep it on one line, like:
> > const struct perf_pmu_event_symbol *pmu1 = p1;
> > const struct perf_pmu_event_symbol *pmu2 = p2;
> >
>
>
> Removing (struct perf_pmu_event_symbol *) will cause compiler error as below.
>
> util/parse-events.c:877:39: error: initialization discards âconstâ qualifier from pointer target type [-Werror]
> struct perf_pmu_event_symbol *pmu1 = p1;
>
> If we keep (struct perf_pmu_event_symbol *), we may not keep it on one line. The line will over 80 characters. There will be error from checkpatch.pl.
>
>
> > > + perf_pmu_events_list =
> > > + malloc(sizeof(struct perf_pmu_event_symbol) * len);
> >
> > please keep above on one line
>
> If so, the line will over 80 characters. There will be error from checkpatch.pl.
AFAIK we are not that strict on this warning.. I think the current
notion is to keep the code easy to read with reasonable breakages
of this rule
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V6 3/3] perf tools: Add support to new style format of kernel PMU event
2014-09-11 19:08 [PATCH V6 0/3] perf tools: pmu event new style format fix kan.liang
2014-09-11 19:08 ` [PATCH V6 1/3] Revert "perf tools: Default to cpu// for events v5" kan.liang
2014-09-11 19:08 ` [PATCH V6 2/3] perf tools: parse the pmu event prefix and surfix kan.liang
@ 2014-09-11 19:08 ` kan.liang
2014-09-14 13:23 ` [PATCH V6 0/3] perf tools: pmu event new style format fix Jiri Olsa
3 siblings, 0 replies; 10+ messages in thread
From: kan.liang @ 2014-09-11 19:08 UTC (permalink / raw)
To: acme, jolsa; +Cc: linux-kernel, ak, Kan Liang
From: Kan Liang <kan.liang@intel.com>
Add new rules for kernel PMU event.
Currently, the patch only want to handle the PMU event name as "a-b" and
"a".
event_pmu:
PE_KERNEL_PMU_EVENT
|
PE_PMU_EVENT_PRE '-' PE_PMU_EVENT_SUF
PE_KERNEL_PMU_EVENT token is for
cycles-ct/cycles-t/mem-loads/mem-stores.
The prefix cycles is mixed up with cpu-cycles.
loads and stores are mixed up with cache event
So they have to be hardcode in lex.
PE_PMU_EVENT_PRE and PE_PMU_EVENT_SUF tokens are for other PMU
events.
The lex looks generic identifier up in the table and return the matched
token. If there is no match, generic PE_NAME token will be return.
Using the rules, kernel PMU event could use new style format without //
so you can use
perf record -e mem-loads ...
instead of
perf record -e cpu/mem-loads/
Signed-off-by: Kan Liang <kan.liang@intel.com>
---
tools/perf/util/parse-events.l | 30 +++++++++++++++++++++++++++++-
tools/perf/util/parse-events.y | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 3432995..ea970cf 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -51,6 +51,24 @@ static int str(yyscan_t scanner, int token)
return token;
}
+static int pmu_str_check(yyscan_t scanner)
+{
+ YYSTYPE *yylval = parse_events_get_lval(scanner);
+ char *text = parse_events_get_text(scanner);
+
+ yylval->str = strdup(text);
+ switch (perf_pmu__parse_check(text)) {
+ case KERNEL_PMU_EVENT_PREFIX:
+ return PE_PMU_EVENT_PRE;
+ case KERNEL_PMU_EVENT_SUFFIX:
+ return PE_PMU_EVENT_SUF;
+ case KERNEL_PMU_EVENT:
+ return PE_KERNEL_PMU_EVENT;
+ default:
+ return PE_NAME;
+ }
+}
+
static int sym(yyscan_t scanner, int type, int config)
{
YYSTYPE *yylval = parse_events_get_lval(scanner);
@@ -178,6 +196,16 @@ alignment-faults { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_AL
emulation-faults { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_EMULATION_FAULTS); }
dummy { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_DUMMY); }
+ /*
+ * We have to handle the kernel PMU event cycles-ct/cycles-t/mem-loads/mem-stores separately.
+ * Because the prefix cycles is mixed up with cpu-cycles.
+ * loads and stores are mixed up with cache event
+ */
+cycles-ct { return str(yyscanner, PE_KERNEL_PMU_EVENT); }
+cycles-t { return str(yyscanner, PE_KERNEL_PMU_EVENT); }
+mem-loads { return str(yyscanner, PE_KERNEL_PMU_EVENT); }
+mem-stores { return str(yyscanner, PE_KERNEL_PMU_EVENT); }
+
L1-dcache|l1-d|l1d|L1-data |
L1-icache|l1-i|l1i|L1-instruction |
LLC|L2 |
@@ -199,7 +227,7 @@ r{num_raw_hex} { return raw(yyscanner); }
{num_hex} { return value(yyscanner, 16); }
{modifier_event} { return str(yyscanner, PE_MODIFIER_EVENT); }
-{name} { return str(yyscanner, PE_NAME); }
+{name} { return pmu_str_check(yyscanner); }
"/" { BEGIN(config); return '/'; }
- { return '-'; }
, { BEGIN(event); return ','; }
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 55fab6a..3f26c03 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -47,6 +47,7 @@ static inc_group_count(struct list_head *list,
%token PE_NAME_CACHE_TYPE PE_NAME_CACHE_OP_RESULT
%token PE_PREFIX_MEM PE_PREFIX_RAW PE_PREFIX_GROUP
%token PE_ERROR
+%token PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
%type <num> PE_VALUE
%type <num> PE_VALUE_SYM_HW
%type <num> PE_VALUE_SYM_SW
@@ -58,6 +59,7 @@ static inc_group_count(struct list_head *list,
%type <str> PE_MODIFIER_EVENT
%type <str> PE_MODIFIER_BP
%type <str> PE_EVENT_NAME
+%type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
%type <num> value_sym
%type <head> event_config
%type <term> event_term
@@ -220,6 +222,44 @@ PE_NAME '/' '/'
ABORT_ON(parse_events_add_pmu(list, &data->idx, $1, NULL));
$$ = list;
}
+|
+PE_KERNEL_PMU_EVENT
+{
+ struct parse_events_evlist *data = _data;
+ struct list_head *head;
+ struct parse_events_term *term;
+ struct list_head *list;
+
+ ALLOC_LIST(head);
+ ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
+ $1, 1));
+ list_add_tail(&term->list, head);
+
+ ALLOC_LIST(list);
+ ABORT_ON(parse_events_add_pmu(list, &data->idx, "cpu", head));
+ parse_events__free_terms(head);
+ $$ = list;
+}
+|
+PE_PMU_EVENT_PRE '-' PE_PMU_EVENT_SUF
+{
+ struct parse_events_evlist *data = _data;
+ struct list_head *head;
+ struct parse_events_term *term;
+ struct list_head *list;
+ char pmu_name[128];
+ snprintf(&pmu_name, 128, "%s-%s", $1, $3);
+
+ ALLOC_LIST(head);
+ ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
+ &pmu_name, 1));
+ list_add_tail(&term->list, head);
+
+ ALLOC_LIST(list);
+ ABORT_ON(parse_events_add_pmu(list, &data->idx, "cpu", head));
+ parse_events__free_terms(head);
+ $$ = list;
+}
value_sym:
PE_VALUE_SYM_HW
--
1.8.3.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V6 0/3] perf tools: pmu event new style format fix
2014-09-11 19:08 [PATCH V6 0/3] perf tools: pmu event new style format fix kan.liang
` (2 preceding siblings ...)
2014-09-11 19:08 ` [PATCH V6 3/3] perf tools: Add support to new style format of kernel PMU event kan.liang
@ 2014-09-14 13:23 ` Jiri Olsa
2014-10-02 17:33 ` Liang, Kan
3 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2014-09-14 13:23 UTC (permalink / raw)
To: kan.liang; +Cc: acme, linux-kernel, ak
On Thu, Sep 11, 2014 at 03:08:56PM -0400, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
>
> There are two types of pmu event stytle formats, "pmu_event_name"
> or "cpu/pmu_event_name/". However, there is a bug on supporting these
> two formats, especially when they mixed with other perf events.
> The patch set intends to fix this issue.
>
> The patch set has been tested on my haswell.
> Here is the test script I used for this issue.
> (Note: please make sure that your test system supports TSX and
> L1-dcache-loads events. Otherwise, you may want to change the events
> to other pmu events.)
any chance changing this script to work for everyone?
and make it part of theautomated tests suite?
also how about something like in the attached change
jirka
---
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 5941927a4b7f..2b7c48474761 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1554,6 +1554,15 @@ static int test_pmu_events(void)
e.check = test__checkevent_pmu_events;
ret = test_event(&e);
+ if (ret)
+ break;
+
+ snprintf(name, MAX_NAME, "event=%s", ent->d_name);
+
+ e.name = name;
+ e.check = test__checkevent_pmu_events;
+
+ ret = test_event(&e);
#undef MAX_NAME
}
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH V6 0/3] perf tools: pmu event new style format fix
2014-09-14 13:23 ` [PATCH V6 0/3] perf tools: pmu event new style format fix Jiri Olsa
@ 2014-10-02 17:33 ` Liang, Kan
2014-10-03 8:24 ` Jiri Olsa
0 siblings, 1 reply; 10+ messages in thread
From: Liang, Kan @ 2014-10-02 17:33 UTC (permalink / raw)
To: Jiri Olsa; +Cc: acme, linux-kernel, ak
>
> On Thu, Sep 11, 2014 at 03:08:56PM -0400, kan.liang@intel.com wrote:
> > From: Kan Liang <kan.liang@intel.com>
> >
> > There are two types of pmu event stytle formats, "pmu_event_name"
> > or "cpu/pmu_event_name/". However, there is a bug on supporting these
> > two formats, especially when they mixed with other perf events.
> > The patch set intends to fix this issue.
> >
> > The patch set has been tested on my haswell.
> > Here is the test script I used for this issue.
> > (Note: please make sure that your test system supports TSX and
> > L1-dcache-loads events. Otherwise, you may want to change the events
> > to other pmu events.)
>
> any chance changing this script to work for everyone?
> and make it part of theautomated tests suite?
>
> also how about something like in the attached change
It cannot trigger the issue, since current perf will automatically add cpu// for pmu event.
We have to test at least two events with different style.
How about the change as below?
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -457,6 +457,36 @@ static int test__checkevent_pmu_events(struct perf_evlist *evlist)
return 0;
}
+
+static int test__checkevent_pmu_events_mix(struct perf_evlist *evlist)
+{
+ struct perf_evsel *evsel = perf_evlist__first(evlist);
+
+ /* pmu-event:u */
+ TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->nr_entries);
+ TEST_ASSERT_VAL("wrong exclude_user",
+ !evsel->attr.exclude_user);
+ TEST_ASSERT_VAL("wrong exclude_kernel",
+ evsel->attr.exclude_kernel);
+ TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+ TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
+ TEST_ASSERT_VAL("wrong pinned", !evsel->attr.pinned);
+
+ /* cpu/pmu-event/u*/
+ evsel = perf_evsel__next(evsel);
+ TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->nr_entries);
+ TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->attr.type);
+ TEST_ASSERT_VAL("wrong exclude_user",
+ !evsel->attr.exclude_user);
+ TEST_ASSERT_VAL("wrong exclude_kernel",
+ evsel->attr.exclude_kernel);
+ TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+ TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
+ TEST_ASSERT_VAL("wrong pinned", !evsel->attr.pinned);
+
+ return 0;
+}
+
static int test__checkterms_simple(struct list_head *terms)
{
struct parse_events_term *term;
@@ -1554,6 +1584,12 @@ static int test_pmu_events(void)
e.check = test__checkevent_pmu_events;
ret = test_event(&e);
+ if (ret)
+ break;
+ snprintf(name, MAX_NAME, "%s:u,cpu/event=%s/u", ent->d_name, ent->d_name);
+ e.name = name;
+ e.check = test__checkevent_pmu_events_mix;
+ ret = test_event(&e);
#undef MAX_NAME
}
Thanks,
Kan
>
> jirka
>
>
> ---
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-
> events.c index 5941927a4b7f..2b7c48474761 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -1554,6 +1554,15 @@ static int test_pmu_events(void)
> e.check = test__checkevent_pmu_events;
>
> ret = test_event(&e);
> + if (ret)
> + break;
> +
> + snprintf(name, MAX_NAME, "event=%s", ent->d_name);
> +
> + e.name = name;
> + e.check = test__checkevent_pmu_events;
> +
> + ret = test_event(&e);
> #undef MAX_NAME
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V6 0/3] perf tools: pmu event new style format fix
2014-10-02 17:33 ` Liang, Kan
@ 2014-10-03 8:24 ` Jiri Olsa
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Olsa @ 2014-10-03 8:24 UTC (permalink / raw)
To: Liang, Kan; +Cc: acme, linux-kernel, ak
On Thu, Oct 02, 2014 at 05:33:28PM +0000, Liang, Kan wrote:
>
>
> >
> > On Thu, Sep 11, 2014 at 03:08:56PM -0400, kan.liang@intel.com wrote:
> > > From: Kan Liang <kan.liang@intel.com>
> > >
> > > There are two types of pmu event stytle formats, "pmu_event_name"
> > > or "cpu/pmu_event_name/". However, there is a bug on supporting these
> > > two formats, especially when they mixed with other perf events.
> > > The patch set intends to fix this issue.
> > >
> > > The patch set has been tested on my haswell.
> > > Here is the test script I used for this issue.
> > > (Note: please make sure that your test system supports TSX and
> > > L1-dcache-loads events. Otherwise, you may want to change the events
> > > to other pmu events.)
> >
> > any chance changing this script to work for everyone?
> > and make it part of theautomated tests suite?
> >
> > also how about something like in the attached change
>
> It cannot trigger the issue, since current perf will automatically add cpu// for pmu event.
> We have to test at least two events with different style.
> How about the change as below?
looks good,
jirka
>
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -457,6 +457,36 @@ static int test__checkevent_pmu_events(struct perf_evlist *evlist)
> return 0;
> }
>
> +
> +static int test__checkevent_pmu_events_mix(struct perf_evlist *evlist)
> +{
> + struct perf_evsel *evsel = perf_evlist__first(evlist);
> +
> + /* pmu-event:u */
> + TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->nr_entries);
> + TEST_ASSERT_VAL("wrong exclude_user",
> + !evsel->attr.exclude_user);
> + TEST_ASSERT_VAL("wrong exclude_kernel",
> + evsel->attr.exclude_kernel);
> + TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
> + TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
> + TEST_ASSERT_VAL("wrong pinned", !evsel->attr.pinned);
> +
> + /* cpu/pmu-event/u*/
> + evsel = perf_evsel__next(evsel);
> + TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->nr_entries);
> + TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->attr.type);
> + TEST_ASSERT_VAL("wrong exclude_user",
> + !evsel->attr.exclude_user);
> + TEST_ASSERT_VAL("wrong exclude_kernel",
> + evsel->attr.exclude_kernel);
> + TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
> + TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
> + TEST_ASSERT_VAL("wrong pinned", !evsel->attr.pinned);
> +
> + return 0;
> +}
> +
> static int test__checkterms_simple(struct list_head *terms)
> {
> struct parse_events_term *term;
> @@ -1554,6 +1584,12 @@ static int test_pmu_events(void)
> e.check = test__checkevent_pmu_events;
>
> ret = test_event(&e);
> + if (ret)
> + break;
> + snprintf(name, MAX_NAME, "%s:u,cpu/event=%s/u", ent->d_name, ent->d_name);
> + e.name = name;
> + e.check = test__checkevent_pmu_events_mix;
> + ret = test_event(&e);
> #undef MAX_NAME
> }
>
>
> Thanks,
> Kan
>
>
> >
> > jirka
> >
> >
> > ---
> > diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-
> > events.c index 5941927a4b7f..2b7c48474761 100644
> > --- a/tools/perf/tests/parse-events.c
> > +++ b/tools/perf/tests/parse-events.c
> > @@ -1554,6 +1554,15 @@ static int test_pmu_events(void)
> > e.check = test__checkevent_pmu_events;
> >
> > ret = test_event(&e);
> > + if (ret)
> > + break;
> > +
> > + snprintf(name, MAX_NAME, "event=%s", ent->d_name);
> > +
> > + e.name = name;
> > + e.check = test__checkevent_pmu_events;
> > +
> > + ret = test_event(&e);
> > #undef MAX_NAME
> > }
^ permalink raw reply [flat|nested] 10+ messages in thread