[PATCH v2 1/4] perf tools: Adds the config_term callback for different type events

[PATCH v2 1/4] perf tools: Adds the config_term callback for different type events

[He Kuang]

Currently, function config_term() is used for checking config terms of
all types of events, while unknown terms is not reported as an error
because pmu events have valid terms in sysfs. But this is wrong when
unknown terms are specificed to hw/sw events. This patch Adds the
config_term callback so we can use separate check routines for each
type of events.

Signed-off-by: He Kuang <hekuang@huawei.com>
---
 tools/perf/util/parse-events.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 61c2bc2..aa64cf3 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;
@@ -665,9 +663,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 +748,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 +809,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))
-- 
1.8.5.2

[PATCH v2 2/4] 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. This patch makes the original error prompt
function formats_error_string() more generic, which only outputs the
static config terms for hw/sw perf events, and prepends pmu formats
for pmu events.

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 | 26 ++++++++++++++++++++++++++
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/pmu.c          | 35 ++++++++++++-----------------------
 3 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index aa64cf3..f624b99 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -656,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;
 	}
 
@@ -1875,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 v2 3/4] perf tools: Adds the tracepoint name parsing support

[He Kuang]

Adds rules for parsing tracepoint names. Change rules of tracepoint
which derives of PE_NAMEs into tracepoint names directly, so adding
more rules based on tracepoint names will be easier.

Signed-off-by: He Kuang <hekuang@huawei.com>
---
 tools/perf/util/parse-events.y | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 8bcc458..279c01c 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 <tracepoint_name> __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 tracepoint_name{
+		char *sys;
+		char *event;
+	} tracepoint_name;
 }
 %%
 
@@ -368,36 +373,39 @@ 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 (parse_events_add_tracepoint(list, &data->idx, $1.sys, $1.event,
+					error)) {
 		if (error)
 			error->idx = @1.first_column;
 		return -1;
 	}
 	$$ = list;
 }
+
+__event_legacy_tracepoint:
+PE_NAME '-' PE_NAME ':' PE_NAME
+{
+	char sys_name[128];
+	struct tracepoint_name tracepoint;
+
+	snprintf(&sys_name, 128, "%s-%s", $1, $3);
+	tracepoint.sys = &sys_name;
+	tracepoint.event = $5;
+
+	$$ = tracepoint;
+}
 |
 PE_NAME ':' PE_NAME
 {
-	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;
-		return -1;
-	}
-	$$ = list;
+	struct tracepoint_name tracepoint = {$1, $3};
+	$$ = tracepoint;
 }
 
 event_legacy_numeric:
-- 
1.8.5.2

[PATCH v2 4/4] 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 | 26 ++++++++++--
 3 files changed, 93 insertions(+), 25 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 279c01c..07c478a 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -380,12 +380,30 @@ __event_legacy_tracepoint
 	struct list_head *list;
 
 	ALLOC_LIST(list);
+	if (error)
+		error->idx = @1.first_column;
+
 	if (parse_events_add_tracepoint(list, &data->idx, $1.sys, $1.event,
-					error)) {
-		if (error)
-			error->idx = @1.first_column;
+					error, NULL))
 		return -1;
-	}
+
+	$$ = list;
+}
+|
+__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 (error)
+		error->idx = @1.first_column;
+
+	if (parse_events_add_tracepoint(list, &data->idx, $1.sys, $1.event,
+					error, $3))
+		return -1;
+
 	$$ = list;
 }
 
-- 
1.8.5.2

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

[Jiri Olsa]

On Fri, Sep 25, 2015 at 11:11:49AM +0000, He Kuang wrote:
> Prompt proper error message when wrong config terms is specificed for
> hw/sw type perf events. This patch makes the original error prompt
> function formats_error_string() more generic, which only outputs the
> static config terms for hw/sw perf events, and prepends pmu formats
> for pmu events.
> 
> 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) ]

this output is not correct at this version,
patch 1/4 will make the above command fail

please change the example accordingly

jirka

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

[Jiri Olsa]

On Fri, Sep 25, 2015 at 11:11:49AM +0000, He Kuang wrote:

SNIP

> +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;
> +}

also please change the comment in the flex file

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 936d566f48d8..c29832bce496 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -174,7 +174,7 @@ modifier_bp	[rwx]{1,3}
 
 <config>{
 	/*
-	 * Please update formats_error_string any time
+	 * Please update parse_events_formats_error_string any time
 	 * new static term is added.
 	 */
 config			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_CONFIG); }

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

[Jiri Olsa]

On Fri, Sep 25, 2015 at 11:11:49AM +0000, He Kuang wrote:

SNIP

> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -656,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;
>  	}
>  
> @@ -1875,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);

not excited about freeing the additional_terms arg in here,
please state it in the comment above the function

thanks,
jirka

Re: [PATCH v2 3/4] perf tools: Adds the tracepoint name parsing support

[Jiri Olsa]

On Fri, Sep 25, 2015 at 11:11:50AM +0000, He Kuang wrote:
> Adds rules for parsing tracepoint names. Change rules of tracepoint
> which derives of PE_NAMEs into tracepoint names directly, so adding
> more rules based on tracepoint names will be easier.
> 
> Signed-off-by: He Kuang <hekuang@huawei.com>
> ---
>  tools/perf/util/parse-events.y | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 8bcc458..279c01c 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 <tracepoint_name> __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 tracepoint_name{
> +		char *sys;
> +		char *event;
> +	} tracepoint_name;
>  }
>  %%
>  
> @@ -368,36 +373,39 @@ 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 (parse_events_add_tracepoint(list, &data->idx, $1.sys, $1.event,
> +					error)) {
>  		if (error)
>  			error->idx = @1.first_column;
>  		return -1;
>  	}
>  	$$ = list;
>  }
> +
> +__event_legacy_tracepoint:

how about the tracepoint_name label change?

thanks,
jirka

Re: [PATCH v2 3/4] perf tools: Adds the tracepoint name parsing support

[Jiri Olsa]

On Fri, Sep 25, 2015 at 11:11:50AM +0000, He Kuang wrote:
> Adds rules for parsing tracepoint names. Change rules of tracepoint
> which derives of PE_NAMEs into tracepoint names directly, so adding
> more rules based on tracepoint names will be easier.
> 
> Signed-off-by: He Kuang <hekuang@huawei.com>
> ---
>  tools/perf/util/parse-events.y | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 8bcc458..279c01c 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 <tracepoint_name> __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 tracepoint_name{
                              ^^^ missing space

you could use scripts/checkpatch.pl and follow some of the recomendations


jirka

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

[Jiri Olsa]

On Fri, Sep 25, 2015 at 11:11:51AM +0000, He Kuang wrote:

SNIP

>  
> +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");

please keep that on one line, we dont follow 80 chars rule strictly
if it results in ugly formatting like above

thanks,
jirka