[PATCH 1/2] perf tools: Prompt error message for wrong terms of hw/sw events

[PATCH 1/2] perf tools: Prompt error message for wrong terms of hw/sw events

[He Kuang]

Prompt proper error message when wrong config terms is specificed for
hw/sw type perf events. Currently, unknown terms is not reported as an
error because pmu events have valid terms in sysfs. But for hw/sw
events, valid config terms are fixed and it should be reported if
wrong terms are given.

Before this patch:

  $ perf record -e 'cpu-clock/freq=200/' -a sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.028 MB perf.data (202 samples) ]

  $ perf record -e 'cpu-clock/freqx=200/' -a sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.042 MB perf.data (506 samples) ]

After this patch:

  $ perf record -e 'cpu-clock/freqx=200/' -a sleep 1
  event syntax error: 'cpu-clock/freqx=200/'
                                 \___ unknown term

  valid terms: config,config1,config2,name,period,freq,branch_type,time,call-graph,stack-size

  Run 'perf list' for a list of valid events

   usage: perf record [<options>] [<command>]
      or: perf record [<options>] -- <command> [<options>]

      -e, --event <event>   event selector. use 'perf list' to list available events

Signed-off-by: He Kuang <hekuang@huawei.com>
---
 tools/perf/util/parse-events.c | 60 +++++++++++++++++++++++++++++++++++-------
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/pmu.c          | 35 +++++++++---------------
 3 files changed, 63 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 61c2bc2..f624b99 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -599,7 +599,11 @@ static int check_type_val(struct parse_events_term *term,
 	return -EINVAL;
 }
 
-static int config_term(struct perf_event_attr *attr,
+typedef int config_term_func_t(struct perf_event_attr *attr,
+			       struct parse_events_term *term,
+			       struct parse_events_error *err);
+
+static int config_term_common(struct perf_event_attr *attr,
 		       struct parse_events_term *term,
 		       struct parse_events_error *err)
 {
@@ -610,12 +614,6 @@ do {									   \
 } while (0)
 
 	switch (term->type_term) {
-	case PARSE_EVENTS__TERM_TYPE_USER:
-		/*
-		 * Always succeed for sysfs terms, as we dont know
-		 * at this point what type they need to have.
-		 */
-		return 0;
 	case PARSE_EVENTS__TERM_TYPE_CONFIG:
 		CHECK_TYPE_VAL(NUM);
 		attr->config = term->val.num;
@@ -658,6 +656,9 @@ do {									   \
 		CHECK_TYPE_VAL(STR);
 		break;
 	default:
+		err->str = strdup("unknown term");
+		err->idx = term->err_term;
+		err->help = parse_events_formats_error_string(NULL);
 		return -EINVAL;
 	}
 
@@ -665,9 +666,24 @@ do {									   \
 #undef CHECK_TYPE_VAL
 }
 
+static int config_term_pmu(struct perf_event_attr *attr,
+		       struct parse_events_term *term,
+		       struct parse_events_error *err)
+{
+	if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER)
+		/*
+		 * Always succeed for sysfs terms, as we dont know
+		 * at this point what type they need to have.
+		 */
+		return 0;
+	else
+		return config_term_common(attr, term, err);
+}
+
 static int config_attr(struct perf_event_attr *attr,
 		       struct list_head *head,
-		       struct parse_events_error *err)
+		       struct parse_events_error *err,
+		       config_term_func_t config_term)
 {
 	struct parse_events_term *term;
 
@@ -735,7 +751,8 @@ int parse_events_add_numeric(struct parse_events_evlist *data,
 	attr.config = config;
 
 	if (head_config) {
-		if (config_attr(&attr, head_config, data->error))
+		if (config_attr(&attr, head_config, data->error,
+				config_term_common))
 			return -EINVAL;
 
 		if (get_config_terms(head_config, &config_terms))
@@ -795,7 +812,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 	 * Configure hardcoded terms first, no need to check
 	 * return value when called with fail == 0 ;)
 	 */
-	if (config_attr(&attr, head_config, data->error))
+	if (config_attr(&attr, head_config, data->error, config_term_pmu))
 		return -EINVAL;
 
 	if (get_config_terms(head_config, &config_terms))
@@ -1861,3 +1878,26 @@ void parse_events_evlist_error(struct parse_events_evlist *data,
 	err->str = strdup(str);
 	WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
 }
+
+char *parse_events_formats_error_string(char *additional_terms)
+{
+	char *str;
+	static const char *static_terms = "config,config1,config2,name,"
+					  "period,freq,branch_type,time,"
+					  "call-graph,stack-size\n";
+
+	/* valid terms */
+	if (additional_terms) {
+		if (!asprintf(&str, "valid terms: %s,%s",
+			      additional_terms, static_terms))
+			goto fail;
+		free(additional_terms);
+	} else {
+		if (!asprintf(&str, "valid terms: %s", static_terms))
+			goto fail;
+	}
+	return str;
+
+fail:
+	return NULL;
+}
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index ffee7ec..c7b904a 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -156,5 +156,6 @@ int print_hwcache_events(const char *event_glob, bool name_only);
 extern int is_valid_tracepoint(const char *event_string);
 
 int valid_event_mount(const char *eventfs);
+char *parse_events_formats_error_string(char *additional_terms);
 
 #endif /* __PERF_PARSE_EVENTS_H */
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 89c91a1..ea3dd04 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -626,38 +626,26 @@ static int pmu_resolve_param_term(struct parse_events_term *term,
 	return -1;
 }
 
-static char *formats_error_string(struct list_head *formats)
+static char *pmu_formats_string(struct list_head *formats)
 {
 	struct perf_pmu_format *format;
-	char *err, *str;
-	static const char *static_terms = "config,config1,config2,name,"
-					  "period,freq,branch_type,time,"
-					  "call-graph,stack-size\n";
+	char *str;
+	struct strbuf buf;
 	unsigned i = 0;
 
-	if (!asprintf(&str, "valid terms:"))
+	if (!formats)
 		return NULL;
 
+	strbuf_init(&buf, 0);
 	/* sysfs exported terms */
-	list_for_each_entry(format, formats, list) {
-		char c = i++ ? ',' : ' ';
-
-		err = str;
-		if (!asprintf(&str, "%s%c%s", err, c, format->name))
-			goto fail;
-		free(err);
-	}
+	list_for_each_entry(format, formats, list)
+		strbuf_addf(&buf, i++ ? ",%s" : "%s",
+			    format->name);
 
-	/* static terms */
-	err = str;
-	if (!asprintf(&str, "%s,%s", err, static_terms))
-		goto fail;
+	str = strbuf_detach(&buf, NULL);
+	strbuf_release(&buf);
 
-	free(err);
 	return str;
-fail:
-	free(err);
-	return NULL;
 }
 
 /*
@@ -695,7 +683,8 @@ static int pmu_config_term(struct list_head *formats,
 		if (err) {
 			err->idx  = term->err_term;
 			err->str  = strdup("unknown term");
-			err->help = formats_error_string(formats);
+			err->help = parse_events_formats_error_string(
+				pmu_formats_string(formats));
 		}
 		return -EINVAL;
 	}
-- 
1.8.5.2

[PATCH 2/2] perf tools: Enable event_config terms to tracepoint events

[He Kuang]

This patch enable config terms for tracepoint perf events. Valid terms
for tracepoint events are call-graph and stack-size, so we can use
different callgraph settings for each event and eliminate unnecessary
overhead.

Here is an example for using different call-graph config for each
tracepoint.

  $ perf record -e syscalls:sys_enter_write/call-graph=fp/
                -e syscalls:sys_exit_write/call-graph=no/
                dd if=/dev/zero of=test bs=4k count=10

  $ perf report --stdio

  #
  # Total Lost Samples: 0
  #
  # Samples: 13  of event 'syscalls:sys_enter_write'
  # Event count (approx.): 13
  #
  # Children      Self  Command  Shared Object       Symbol
  # ........  ........  .......  ..................  ......................
  #
      76.92%    76.92%  dd       libpthread-2.20.so  [.] __write_nocancel
                   |
                   ---__write_nocancel

      23.08%    23.08%  dd       libc-2.20.so        [.] write
                   |
                   ---write
                      |
                      |--33.33%-- 0x2031342820736574
                      |
                      |--33.33%-- 0xa6e69207364726f
                      |
                       --33.33%-- 0x34202c7320393039
  ...

  # Samples: 13  of event 'syscalls:sys_exit_write'
  # Event count (approx.): 13
  #
  # Children      Self  Command  Shared Object       Symbol
  # ........  ........  .......  ..................  ......................
  #
      76.92%    76.92%  dd       libpthread-2.20.so  [.] __write_nocancel
      23.08%    23.08%  dd       libc-2.20.so        [.] write
       7.69%     0.00%  dd       [unknown]           [.] 0x0a6e69207364726f
       7.69%     0.00%  dd       [unknown]           [.] 0x2031342820736574
       7.69%     0.00%  dd       [unknown]           [.] 0x34202c7320393039

Signed-off-by: He Kuang <hekuang@huawei.com>
---
 tools/perf/util/parse-events.c | 89 ++++++++++++++++++++++++++++++++----------
 tools/perf/util/parse-events.h |  3 +-
 tools/perf/util/parse-events.y | 46 +++++++++++++++++-----
 3 files changed, 107 insertions(+), 31 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f624b99..e5cc112 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -27,6 +27,8 @@
 extern int parse_events_debug;
 #endif
 int parse_events_parse(void *data, void *scanner);
+static int get_config_terms(struct list_head *head_config,
+			    struct list_head *head_terms __maybe_unused);
 
 static struct perf_pmu_event_symbol *perf_pmu_events_list;
 /*
@@ -416,7 +418,8 @@ static void tracepoint_error(struct parse_events_error *error, int err,
 
 static int add_tracepoint(struct list_head *list, int *idx,
 			  char *sys_name, char *evt_name,
-			  struct parse_events_error *error __maybe_unused)
+			  struct parse_events_error *error __maybe_unused,
+			  struct list_head *head_config)
 {
 	struct perf_evsel *evsel;
 
@@ -426,13 +429,22 @@ static int add_tracepoint(struct list_head *list, int *idx,
 		return PTR_ERR(evsel);
 	}
 
+	if (head_config) {
+		LIST_HEAD(config_terms);
+
+		if (get_config_terms(head_config, &config_terms))
+			return -ENOMEM;
+		list_splice(&config_terms, &evsel->config_terms);
+	}
+
 	list_add_tail(&evsel->node, list);
 	return 0;
 }
 
 static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 				      char *sys_name, char *evt_name,
-				      struct parse_events_error *error)
+				      struct parse_events_error *error,
+				      struct list_head *head_config)
 {
 	char evt_path[MAXPATHLEN];
 	struct dirent *evt_ent;
@@ -456,7 +468,8 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 		if (!strglobmatch(evt_ent->d_name, evt_name))
 			continue;
 
-		ret = add_tracepoint(list, idx, sys_name, evt_ent->d_name, error);
+		ret = add_tracepoint(list, idx, sys_name, evt_ent->d_name,
+				     error, head_config);
 	}
 
 	closedir(evt_dir);
@@ -465,16 +478,20 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 
 static int add_tracepoint_event(struct list_head *list, int *idx,
 				char *sys_name, char *evt_name,
-				struct parse_events_error *error)
+				struct parse_events_error *error,
+				struct list_head *head_config)
 {
 	return strpbrk(evt_name, "*?") ?
-	       add_tracepoint_multi_event(list, idx, sys_name, evt_name, error) :
-	       add_tracepoint(list, idx, sys_name, evt_name, error);
+	       add_tracepoint_multi_event(list, idx, sys_name, evt_name,
+					  error, head_config) :
+	       add_tracepoint(list, idx, sys_name, evt_name,
+			      error, head_config);
 }
 
 static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 				    char *sys_name, char *evt_name,
-				    struct parse_events_error *error)
+				    struct parse_events_error *error,
+				    struct list_head *head_config)
 {
 	struct dirent *events_ent;
 	DIR *events_dir;
@@ -498,23 +515,13 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 			continue;
 
 		ret = add_tracepoint_event(list, idx, events_ent->d_name,
-					   evt_name, error);
+					   evt_name, error, head_config);
 	}
 
 	closedir(events_dir);
 	return ret;
 }
 
-int parse_events_add_tracepoint(struct list_head *list, int *idx,
-				char *sys, char *event,
-				struct parse_events_error *error)
-{
-	if (strpbrk(sys, "*?"))
-		return add_tracepoint_multi_sys(list, idx, sys, event, error);
-	else
-		return add_tracepoint_event(list, idx, sys, event, error);
-}
-
 static int
 parse_breakpoint_type(const char *type, struct perf_event_attr *attr)
 {
@@ -604,8 +611,8 @@ typedef int config_term_func_t(struct perf_event_attr *attr,
 			       struct parse_events_error *err);
 
 static int config_term_common(struct perf_event_attr *attr,
-		       struct parse_events_term *term,
-		       struct parse_events_error *err)
+			      struct parse_events_term *term,
+			      struct parse_events_error *err)
 {
 #define CHECK_TYPE_VAL(type)						   \
 do {									   \
@@ -680,6 +687,27 @@ static int config_term_pmu(struct perf_event_attr *attr,
 		return config_term_common(attr, term, err);
 }
 
+static int config_term_tracepoint(struct perf_event_attr *attr,
+				  struct parse_events_term *term,
+				  struct parse_events_error *err)
+{
+	switch (term->type_term) {
+	case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
+	case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
+		return config_term_common(attr, term, err);
+	default:
+		if (err) {
+			err->idx = term->err_term;
+			err->str = strdup("unknown term");
+			err->help = strdup(
+				"valid terms: call-graph,stack-size\n");
+		}
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int config_attr(struct perf_event_attr *attr,
 		       struct list_head *head,
 		       struct parse_events_error *err,
@@ -738,6 +766,27 @@ do {								\
 	return 0;
 }
 
+int parse_events_add_tracepoint(struct list_head *list, int *idx,
+				char *sys, char *event,
+				struct parse_events_error *error,
+				struct list_head *head_config)
+{
+	if (head_config) {
+		struct perf_event_attr attr;
+
+		if (config_attr(&attr, head_config, error,
+				config_term_tracepoint))
+			return -EINVAL;
+	}
+
+	if (strpbrk(sys, "*?"))
+		return add_tracepoint_multi_sys(list, idx, sys, event,
+						error, head_config);
+	else
+		return add_tracepoint_event(list, idx, sys, event,
+					    error, head_config);
+}
+
 int parse_events_add_numeric(struct parse_events_evlist *data,
 			     struct list_head *list,
 			     u32 type, u64 config,
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index c7b904a..f13d3cc 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -119,7 +119,8 @@ int parse_events__modifier_group(struct list_head *list, char *event_mod);
 int parse_events_name(struct list_head *list, char *name);
 int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				char *sys, char *event,
-				struct parse_events_error *error);
+				struct parse_events_error *error,
+				struct list_head *head_config);
 int parse_events_add_numeric(struct parse_events_evlist *data,
 			     struct list_head *list,
 			     u32 type, u64 config,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 8bcc458..6b15a80 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -67,6 +67,7 @@ static inc_group_count(struct list_head *list,
 %type <head> event_legacy_cache
 %type <head> event_legacy_mem
 %type <head> event_legacy_tracepoint
+%type <str_group> __event_legacy_tracepoint
 %type <head> event_legacy_numeric
 %type <head> event_legacy_raw
 %type <head> event_def
@@ -84,6 +85,10 @@ static inc_group_count(struct list_head *list,
 	u64 num;
 	struct list_head *head;
 	struct parse_events_term *term;
+	struct str_group{
+		char *name1;
+		char *name2;
+	} str_group;
 }
 %%
 
@@ -368,38 +373,59 @@ PE_PREFIX_MEM PE_VALUE sep_dc
 }
 
 event_legacy_tracepoint:
-PE_NAME '-' PE_NAME ':' PE_NAME
+__event_legacy_tracepoint
 {
 	struct parse_events_evlist *data = _data;
 	struct parse_events_error *error = data->error;
 	struct list_head *list;
-	char sys_name[128];
-	snprintf(&sys_name, 128, "%s-%s", $1, $3);
 
 	ALLOC_LIST(list);
-	if (parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, error)) {
-		if (error)
-			error->idx = @1.first_column;
+	if (error)
+		error->idx = @1.first_column;
+
+	if (parse_events_add_tracepoint(list, &data->idx, $1.name1, $1.name2,
+					error, NULL)) {
 		return -1;
 	}
 	$$ = list;
 }
 |
-PE_NAME ':' PE_NAME
+__event_legacy_tracepoint '/' event_config '/'
 {
 	struct parse_events_evlist *data = _data;
 	struct parse_events_error *error = data->error;
 	struct list_head *list;
 
 	ALLOC_LIST(list);
-	if (parse_events_add_tracepoint(list, &data->idx, $1, $3, error)) {
-		if (error)
-			error->idx = @1.first_column;
+	if (error)
+		error->idx = @1.first_column;
+
+	if (parse_events_add_tracepoint(list, &data->idx, $1.name1, $1.name2,
+					error, $3)) {
 		return -1;
 	}
 	$$ = list;
 }
 
+__event_legacy_tracepoint:
+PE_NAME '-' PE_NAME ':' PE_NAME
+{
+	char sys_name[128];
+	struct str_group group;
+
+	snprintf(&sys_name, 128, "%s-%s", $1, $3);
+	group.name1 = &sys_name;
+	group.name2 = $5;
+
+	$$ = group;
+}
+|
+PE_NAME ':' PE_NAME
+{
+	struct str_group group = {$1, $3};
+	$$ = group;
+}
+
 event_legacy_numeric:
 PE_VALUE ':' PE_VALUE
 {
-- 
1.8.5.2

Re: [PATCH 2/2] perf tools: Enable event_config terms to tracepoint events

[Jiri Olsa]

On Fri, Sep 25, 2015 at 03:21:19AM +0000, He Kuang wrote:

SNIP

> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index c7b904a..f13d3cc 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -119,7 +119,8 @@ int parse_events__modifier_group(struct list_head *list, char *event_mod);
>  int parse_events_name(struct list_head *list, char *name);
>  int parse_events_add_tracepoint(struct list_head *list, int *idx,
>  				char *sys, char *event,
> -				struct parse_events_error *error);
> +				struct parse_events_error *error,
> +				struct list_head *head_config);
>  int parse_events_add_numeric(struct parse_events_evlist *data,
>  			     struct list_head *list,
>  			     u32 type, u64 config,
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 8bcc458..6b15a80 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -67,6 +67,7 @@ static inc_group_count(struct list_head *list,
>  %type <head> event_legacy_cache
>  %type <head> event_legacy_mem
>  %type <head> event_legacy_tracepoint
> +%type <str_group> __event_legacy_tracepoint
>  %type <head> event_legacy_numeric
>  %type <head> event_legacy_raw
>  %type <head> event_def
> @@ -84,6 +85,10 @@ static inc_group_count(struct list_head *list,
>  	u64 num;
>  	struct list_head *head;
>  	struct parse_events_term *term;
> +	struct str_group{
> +		char *name1;
> +		char *name2;
> +	} str_group;


perhaps tracepoint_name would be more suitable?
with 'struct tracepoint_name' instead of str_group

thanks,
jirka

Re: [PATCH 2/2] perf tools: Enable event_config terms to tracepoint events

[Jiri Olsa]

On Fri, Sep 25, 2015 at 03:21:19AM +0000, He Kuang wrote:

SNIP

>  }
>  
> +__event_legacy_tracepoint:
> +PE_NAME '-' PE_NAME ':' PE_NAME
> +{
> +	char sys_name[128];
> +	struct str_group group;
> +
> +	snprintf(&sys_name, 128, "%s-%s", $1, $3);
> +	group.name1 = &sys_name;
> +	group.name2 = $5;
> +
> +	$$ = group;
> +}
> +|
> +PE_NAME ':' PE_NAME
> +{
> +	struct str_group group = {$1, $3};
> +	$$ = group;
> +}
> +
>  event_legacy_numeric:
>  PE_VALUE ':' PE_VALUE
>  {
> -- 
> 1.8.5.2
> 

please split this into adding the tracepoint terms parsing support
and the new tracepoint terms config logic

thanks,
jirka

Re: [PATCH 1/2] perf tools: Prompt error message for wrong terms of hw/sw events

[Jiri Olsa]

On Fri, Sep 25, 2015 at 03:21:18AM +0000, He Kuang wrote:

SNIP

>  static int config_attr(struct perf_event_attr *attr,
>  		       struct list_head *head,
> -		       struct parse_events_error *err)
> +		       struct parse_events_error *err,
> +		       config_term_func_t config_term)
>  {
>  	struct parse_events_term *term;
>  
> @@ -735,7 +751,8 @@ int parse_events_add_numeric(struct parse_events_evlist *data,
>  	attr.config = config;
>  
>  	if (head_config) {
> -		if (config_attr(&attr, head_config, data->error))
> +		if (config_attr(&attr, head_config, data->error,
> +				config_term_common))
>  			return -EINVAL;
>  
>  		if (get_config_terms(head_config, &config_terms))
> @@ -795,7 +812,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
>  	 * Configure hardcoded terms first, no need to check
>  	 * return value when called with fail == 0 ;)
>  	 */
> -	if (config_attr(&attr, head_config, data->error))
> +	if (config_attr(&attr, head_config, data->error, config_term_pmu))
>  		return -EINVAL;
>  
>  	if (get_config_terms(head_config, &config_terms))
> @@ -1861,3 +1878,26 @@ void parse_events_evlist_error(struct parse_events_evlist *data,
>  	err->str = strdup(str);
>  	WARN_ONCE(!err->str, "WARNING: failed to allocate error string");

could you please plit this into the part that adds the
config_term callback and the error message formating change?

thanks,
jirka