[PATCH v2 0/3] perf: convert to ctf: support converting syscalls:* tracepoints.

[PATCH v2 0/3] perf: convert to ctf: support converting syscalls:* tracepoints.

[Wang Nan]

Babeltrace now support bt_ctf_validate_identifier() to allow us detect
keyword confliction before real add it. I update patch 2/3 to use that
new interface. Also, I group all 3 patches I made during converting
syscalls:* tracepoints samples together for review.

Wang Nan (3):
  tools lib traceevent: add priv field to truct format_field.
  perf: convert: fix duplicate field names and avoid reserved keywords.
  perf: convert: fix signess of value.

 tools/lib/traceevent/event-parse.c |   2 +
 tools/lib/traceevent/event-parse.h |   2 +
 tools/perf/util/data-convert-bt.c  | 161 +++++++++++++++++++++++++++++++++----
 3 files changed, 149 insertions(+), 16 deletions(-)

-- 
1.8.4

[PATCH v2 1/3] tools lib traceevent: add priv field to struct format_field.

[Wang Nan]

Introduce a priv field to 'struct format_field' for futher expansion.

(In https://lkml.org/lkml/2015/1/21/383 , Jiri Olsa gives a suggestion
 about changing lib traceevent to solve a bug of perf-convert-to-ctf,
 which is related to duplicated field names. I think his suggestion
 should be something like this patch. )

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/lib/traceevent/event-parse.c | 2 ++
 tools/lib/traceevent/event-parse.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index cf3a44b..5f76003 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -5909,6 +5909,8 @@ static void free_format_fields(struct format_field *field)
 		free(field->type);
 		free(field->name);
 		free(field);
+		if (field->destroy_priv)
+			field->destroy_priv(field);
 		field = next;
 	}
 }
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 7a3873f..928d801 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -190,6 +190,8 @@ struct format_field {
 	unsigned int		arraylen;
 	unsigned int		elementsize;
 	unsigned long		flags;
+	void			*priv;
+	void 			(*destroy_priv)(struct format_field *);
 };
 
 struct format {
-- 
1.8.4

[PATCH v2 2/3] perf: convert: fix duplicate field names and avoid reserved keywords.

[Wang Nan]

(If Steven Rostedt accept the previous patch which introduce a priv
 field to 'struct format_field', we can use a relative simple method
 for name conversion. If not , perf must track name conversion by
 itself.)

Some parameters of syscall tracepoints named as 'nr', 'event', etc.
When dealing with them, perf convert to ctf meets some problem:

 1. If a parameter with name 'nr', it will duplicate syscall's
    common field 'nr'. One such syscall is io_submit().

 2. If a parameter with name 'event', it is denied to be inserted
    because 'event' is a babeltrace keywork. One such syscall is
    epoll_ctl.

This patch appends '_dupl_X' suffix to avoid problem 1, prepend a '_'
prefix to avoid problem 2.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/util/data-convert-bt.c | 98 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 94 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
index ddecce8..934bd9b 100644
--- a/tools/perf/util/data-convert-bt.c
+++ b/tools/perf/util/data-convert-bt.c
@@ -14,6 +14,7 @@
 #include <babeltrace/ctf-writer/event.h>
 #include <babeltrace/ctf-writer/event-types.h>
 #include <babeltrace/ctf-writer/event-fields.h>
+#include <babeltrace/ctf-ir/utils.h>
 #include <babeltrace/ctf/events.h>
 #include <traceevent/event-parse.h>
 #include "asm/bug.h"
@@ -184,6 +185,7 @@ static int add_tracepoint_field_value(struct ctf_writer *cw,
 	unsigned int len;
 	int ret;
 
+	name = fmtf->priv ? (const char *)fmtf->priv : fmtf->name;
 	offset = fmtf->offset;
 	len = fmtf->size;
 	if (flags & FIELD_IS_STRING)
@@ -567,6 +569,94 @@ static int process_sample_event(struct perf_tool *tool,
 	return cs ? 0 : -1;
 }
 
+/* If dup < 0, add a prefix. Else, add _dupl_X suffix. */
+static char *change_name(char *name, char *orig_name, int dup)
+{
+	char *new_name = NULL;
+	size_t len;
+
+	if (!name)
+		name = orig_name;
+
+	if (dup >= 10)
+		goto out;
+	/*
+	 * Add '_' prefix to potential keywork.  According to
+	 * Mathieu Desnoyers (https://lkml.org/lkml/2015/1/23/652),
+	 * futher CTF spec updating may require us to use '$'.
+	 */
+	if (dup < 0)
+		len = strlen(name) + sizeof("_");
+	else
+		len = strlen(orig_name) + sizeof("_dupl_X");
+
+	new_name = malloc(len);
+	if (!new_name)
+		goto out;
+
+	if (dup < 0)
+		snprintf(new_name, len, "_%s", name);
+	else
+		snprintf(new_name, len, "%s_dupl_%d", orig_name, dup);
+
+out:
+	if (name != orig_name)
+		free(name);
+	return new_name;
+}
+
+static void destroy_field_priv(struct format_field *field)
+{
+	if (!field->priv)
+		return;
+
+	if (field->priv != field->name)
+		free(field->priv);
+
+	field->priv = NULL;
+	field->destroy_priv = NULL;
+}
+
+static int event_class_add_field(struct bt_ctf_event_class *event_class,
+		struct bt_ctf_field_type *type,
+		struct format_field *field)
+{
+	struct bt_ctf_field_type *t = NULL;
+	char *name;
+	int dup = 1;
+	int ret;
+
+	if (field->priv)
+		return bt_ctf_event_class_add_field(event_class, type,
+				(char *)field->priv);
+
+	name = field->name;
+
+	/* If 'name' is a keywork, add prefix. */
+	if (bt_ctf_validate_identifier(name))
+		name = change_name(name, field->name, -1);
+
+	if (!name) {
+		pr_err("Failed to fix invalid identifier.");
+		return -1;
+	}
+	while ((t = bt_ctf_event_class_get_field_by_name(event_class, name))) {
+		bt_ctf_field_type_put(t);
+		name = change_name(name, field->name, dup++);
+		if (!name) {
+			pr_err("Failed to create dup name for '%s'\n", field->name);
+			return -1;
+		}
+	}
+
+	ret = bt_ctf_event_class_add_field(event_class, type, name);
+
+	field->priv = name;
+	field->destroy_priv = destroy_field_priv;
+
+	return ret;
+}
+
 static int add_tracepoint_fields_types(struct ctf_writer *cw,
 				       struct format_field *fields,
 				       struct bt_ctf_event_class *event_class)
@@ -595,14 +685,14 @@ static int add_tracepoint_fields_types(struct ctf_writer *cw,
 		if (flags & FIELD_IS_ARRAY)
 			type = bt_ctf_field_type_array_create(type, field->arraylen);
 
-		ret = bt_ctf_event_class_add_field(event_class, type,
-				field->name);
+		ret = event_class_add_field(event_class, type, field);
 
 		if (flags & FIELD_IS_ARRAY)
 			bt_ctf_field_type_put(type);
 
 		if (ret) {
-			pr_err("Failed to add field '%s\n", field->name);
+			pr_err("Failed to add field '%s': %d\n",
+					field->name, ret);
 			return -1;
 		}
 	}
@@ -646,7 +736,7 @@ static int add_generic_types(struct ctf_writer *cw, struct perf_evsel *evsel,
 	do {								\
 		pr2("  field '%s'\n", n);				\
 		if (bt_ctf_event_class_add_field(cl, t, n)) {		\
-			pr_err("Failed to add field '%s;\n", n);	\
+			pr_err("Failed to add field '%s';\n", n);	\
 			return -1;					\
 		}							\
 	} while (0)
-- 
1.8.4

[PATCH v2 3/3] perf: convert: fix signess of value.

[Wang Nan]

When converting int values, perf first extractes it to a ulonglong, then
feeds it to babeltrace as a signed value. For negative 32 bit values
(for example, return values of failed syscalls), the extracted data
should be something like 0xfffffffe (-2). It becomes a large int64
value. Babeltrace denies to insert it with
bt_ctf_field_signed_integer_set_value() because it is larger than
0x7fffffff, the largest positive value a signed 32 bit int can be.

This patch introduces adjust_signess(), which fills high bits of
ulonglong with 1 if the value is negative.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/util/data-convert-bt.c | 63 +++++++++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
index 934bd9b..a89f879 100644
--- a/tools/perf/util/data-convert-bt.c
+++ b/tools/perf/util/data-convert-bt.c
@@ -166,6 +166,43 @@ get_tracepoint_field_type(struct ctf_writer *cw, struct format_field *field)
 		return cw->data.u32;
 }
 
+static unsigned long long adjust_signess(unsigned long long value_int, int size)
+{
+	unsigned long long value_mask;
+
+	/*
+	 * value_mask = (1 << (size * 8 - 1)) - 1.
+	 * Directly set value_mask for code readers.
+	 */
+	switch (size) {
+	case 1:
+		value_mask = 0x7fULL;
+		break;
+	case 2:
+		value_mask = 0x7fffULL;
+		break;
+	case 4:
+		value_mask = 0x7fffffffULL;
+		break;
+	case 8:
+		/*
+		 * For 64 bit value, return it self. There is no need
+		 * to fill high bit.
+		 */
+		/* Fall through */
+	default:
+		/* BUG! */
+		return value_int;
+	}
+
+	/* If it is a positive value, don't adjust. */
+	if ((value_int & (~0ULL - value_mask)) == 0)
+		return value_int;
+
+	/* Fill upper part of value_int with 1 to make it a negative long long. */
+	return (value_int & value_mask) | ~value_mask;
+}
+
 static int add_tracepoint_field_value(struct ctf_writer *cw,
 				      struct bt_ctf_event_class *event_class,
 				      struct bt_ctf_event *event,
@@ -177,7 +214,6 @@ static int add_tracepoint_field_value(struct ctf_writer *cw,
 	struct bt_ctf_field *field;
 	const char *name = fmtf->name;
 	void *data = sample->raw_data;
-	unsigned long long value_int;
 	unsigned long flags = fmtf->flags;
 	unsigned int n_items;
 	unsigned int i;
@@ -222,11 +258,6 @@ static int add_tracepoint_field_value(struct ctf_writer *cw,
 	type = get_tracepoint_field_type(cw, fmtf);
 
 	for (i = 0; i < n_items; i++) {
-		if (!(flags & FIELD_IS_STRING))
-			value_int = pevent_read_number(
-					fmtf->event->pevent,
-					data + offset + i * len, len);
-
 		if (flags & FIELD_IS_ARRAY)
 			field = bt_ctf_field_array_get_field(array_field, i);
 		else
@@ -240,12 +271,20 @@ static int add_tracepoint_field_value(struct ctf_writer *cw,
 		if (flags & FIELD_IS_STRING)
 			ret = bt_ctf_field_string_set_value(field,
 					data + offset + i * len);
-		else if (!(flags & FIELD_IS_SIGNED))
-			ret = bt_ctf_field_unsigned_integer_set_value(
-					field, value_int);
-		else
-			ret = bt_ctf_field_signed_integer_set_value(
-					field, value_int);
+		else {
+			unsigned long long value_int;
+			value_int = pevent_read_number(
+					fmtf->event->pevent,
+					data + offset + i * len, len);
+
+			if (!(flags & FIELD_IS_SIGNED))
+				ret = bt_ctf_field_unsigned_integer_set_value(
+						field, value_int);
+			else
+				ret = bt_ctf_field_signed_integer_set_value(
+						field, adjust_signess(value_int, len));
+		}
+
 		if (ret) {
 			pr_err("failed to set file value %s\n", name);
 			goto err_put_field;
-- 
1.8.4

Re: [PATCH v2 0/3] perf: convert to ctf: support converting syscalls:* tracepoints.

[Jiri Olsa]

On Mon, Jan 26, 2015 at 06:38:22PM +0800, Wang Nan wrote:
> Babeltrace now support bt_ctf_validate_identifier() to allow us detect
> keyword confliction before real add it. I update patch 2/3 to use that
> new interface. Also, I group all 3 patches I made during converting
> syscalls:* tracepoints samples together for review.
> 
> Wang Nan (3):
>   tools lib traceevent: add priv field to truct format_field.
>   perf: convert: fix duplicate field names and avoid reserved keywords.
>   perf: convert: fix signess of value.

heya,
looks ok to me.. I applied/pushed this on the top fo my 'perf/core_ctf_convert'
branch and will repost together with CTF v4 post, once we figure out the:
  http://marc.info/?l=linux-kernel&m=142219345232713&w=2

and get feedback from Steven about the libtrace change ;-)

thanks,
jirka

Re: [PATCH v2 1/3] tools lib traceevent: add priv field to struct format_field.

[Jiri Olsa]

On Mon, Jan 26, 2015 at 06:38:23PM +0800, Wang Nan wrote:
> Introduce a priv field to 'struct format_field' for futher expansion.
> 
> (In https://lkml.org/lkml/2015/1/21/383 , Jiri Olsa gives a suggestion
>  about changing lib traceevent to solve a bug of perf-convert-to-ctf,
>  which is related to duplicated field names. I think his suggestion
>  should be something like this patch. )

Hi Steven,
any feedback on this one?

thanks,
jirka

> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/lib/traceevent/event-parse.c | 2 ++
>  tools/lib/traceevent/event-parse.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index cf3a44b..5f76003 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -5909,6 +5909,8 @@ static void free_format_fields(struct format_field *field)
>  		free(field->type);
>  		free(field->name);
>  		free(field);
> +		if (field->destroy_priv)
> +			field->destroy_priv(field);
>  		field = next;
>  	}
>  }
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 7a3873f..928d801 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -190,6 +190,8 @@ struct format_field {
>  	unsigned int		arraylen;
>  	unsigned int		elementsize;
>  	unsigned long		flags;
> +	void			*priv;
> +	void 			(*destroy_priv)(struct format_field *);
>  };
>  
>  struct format {
> -- 
> 1.8.4
> 

Re: [PATCH v2 1/3] tools lib traceevent: add priv field to struct format_field.

[Steven Rostedt]

On Mon, 26 Jan 2015 18:38:23 +0800
Wang Nan <wangnan0@huawei.com> wrote:


> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index cf3a44b..5f76003 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -5909,6 +5909,8 @@ static void free_format_fields(struct format_field *field)
>  		free(field->type);
>  		free(field->name);
>  		free(field);
> +		if (field->destroy_priv)
> +			field->destroy_priv(field);

I think you want to call field->destroy_priv() *before* you free field.

-- Steve

>  		field = next;
>  	}
>  }

Re: [PATCH v2 1/3] tools lib traceevent: add priv field to struct format_field.

[Jiri Olsa]

On Fri, Jan 30, 2015 at 09:17:19AM -0500, Steven Rostedt wrote:
> On Mon, 26 Jan 2015 18:38:23 +0800
> Wang Nan <wangnan0@huawei.com> wrote:
> 
> 
> > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > index cf3a44b..5f76003 100644
> > --- a/tools/lib/traceevent/event-parse.c
> > +++ b/tools/lib/traceevent/event-parse.c
> > @@ -5909,6 +5909,8 @@ static void free_format_fields(struct format_field *field)
> >  		free(field->type);
> >  		free(field->name);
> >  		free(field);
> > +		if (field->destroy_priv)
> > +			field->destroy_priv(field);
> 
> I think you want to call field->destroy_priv() *before* you free field.

argh.. missed that :-\ will fix

thanks,
jirka

> 
> -- Steve
> 
> >  		field = next;
> >  	}
> >  }

[PATCHv3] tools lib traceevent: Add priv field to struct format_field

[Jiri Olsa]

On Fri, Jan 30, 2015 at 03:24:47PM +0100, Jiri Olsa wrote:
> On Fri, Jan 30, 2015 at 09:17:19AM -0500, Steven Rostedt wrote:
> > On Mon, 26 Jan 2015 18:38:23 +0800
> > Wang Nan <wangnan0@huawei.com> wrote:
> > 
> > 
> > > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > > index cf3a44b..5f76003 100644
> > > --- a/tools/lib/traceevent/event-parse.c
> > > +++ b/tools/lib/traceevent/event-parse.c
> > > @@ -5909,6 +5909,8 @@ static void free_format_fields(struct format_field *field)
> > >  		free(field->type);
> > >  		free(field->name);
> > >  		free(field);
> > > +		if (field->destroy_priv)
> > > +			field->destroy_priv(field);
> > 
> > I think you want to call field->destroy_priv() *before* you free field.
> 
> argh.. missed that :-\ will fix
> 

fixed version

jirka


---
From: Wang Nan <wangnan0@huawei.com>

Introduce a priv field to 'struct format_field' for futher expansion.

(In https://lkml.org/lkml/2015/1/21/383 , Jiri Olsa gives a suggestion
 about changing lib traceevent to solve a bug of perf-convert-to-ctf,
 which is related to duplicated field names. I think his suggestion
 should be something like this patch. )

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Tom Zanussi <tzanussi@gmail.com>
[ moved field release after destroy callback call ]
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/traceevent/event-parse.c | 2 ++
 tools/lib/traceevent/event-parse.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index afe20ed9fac8..64d40b7e0582 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -6236,6 +6236,8 @@ static void free_format_fields(struct format_field *field)
 		next = field->next;
 		free(field->type);
 		free(field->name);
+		if (field->destroy_priv)
+			field->destroy_priv(field);
 		free(field);
 		field = next;
 	}
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 7a3873ff9a4f..928d801444ab 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -190,6 +190,8 @@ struct format_field {
 	unsigned int		arraylen;
 	unsigned int		elementsize;
 	unsigned long		flags;
+	void			*priv;
+	void 			(*destroy_priv)(struct format_field *);
 };
 
 struct format {
-- 
1.9.3

Re: [PATCH v2 2/3] perf: convert: fix duplicate field names and avoid reserved keywords.

[Steven Rostedt]

On Mon, Jan 26, 2015 at 06:38:24PM +0800, Wang Nan wrote:
> (If Steven Rostedt accept the previous patch which introduce a priv
>  field to 'struct format_field', we can use a relative simple method
>  for name conversion. If not , perf must track name conversion by
>  itself.)

Sorry for coming in so late here.

> 
> Some parameters of syscall tracepoints named as 'nr', 'event', etc.
> When dealing with them, perf convert to ctf meets some problem:
> 
>  1. If a parameter with name 'nr', it will duplicate syscall's
>     common field 'nr'. One such syscall is io_submit().
> 
>  2. If a parameter with name 'event', it is denied to be inserted
>     because 'event' is a babeltrace keywork. One such syscall is
>     epoll_ctl.
> 
> This patch appends '_dupl_X' suffix to avoid problem 1, prepend a '_'
> prefix to avoid problem 2.

Actually, I don't like this approach. That is, to have this private
data structure. Why not just add an "alias" to format_field. In other
words, instead of hiding this interaction behind a void pointer and 
needing to create a function pointer to free it, just add another
field to format field and be done with it. I think it would make
the code a hell of a lot simpler and easier to understand.

 struct format_field {
 	struct format_field	*next;
	struct event_format	*event;
	char			*type;
	char			*name;
+	char			*alias;
	int			offset;
	int			size;
	unsigned int		arraylen;
	unsigned int		elementsize;
	unsigned long		flags;
};

And put the logic in event parse to free alias if need be.
Heck, you could even add a pevent_*() func to assign the alias
using strdup, or what not.

-- Steve

Re: [PATCH v2 2/3] perf: convert: fix duplicate field names and avoid reserved keywords.

[Jiri Olsa]

On Fri, Jan 30, 2015 at 10:25:37AM -0500, Steven Rostedt wrote:
> On Mon, Jan 26, 2015 at 06:38:24PM +0800, Wang Nan wrote:
> > (If Steven Rostedt accept the previous patch which introduce a priv
> >  field to 'struct format_field', we can use a relative simple method
> >  for name conversion. If not , perf must track name conversion by
> >  itself.)
> 
> Sorry for coming in so late here.
> 
> > 
> > Some parameters of syscall tracepoints named as 'nr', 'event', etc.
> > When dealing with them, perf convert to ctf meets some problem:
> > 
> >  1. If a parameter with name 'nr', it will duplicate syscall's
> >     common field 'nr'. One such syscall is io_submit().
> > 
> >  2. If a parameter with name 'event', it is denied to be inserted
> >     because 'event' is a babeltrace keywork. One such syscall is
> >     epoll_ctl.
> > 
> > This patch appends '_dupl_X' suffix to avoid problem 1, prepend a '_'
> > prefix to avoid problem 2.
> 
> Actually, I don't like this approach. That is, to have this private
> data structure. Why not just add an "alias" to format_field. In other
> words, instead of hiding this interaction behind a void pointer and 
> needing to create a function pointer to free it, just add another
> field to format field and be done with it. I think it would make
> the code a hell of a lot simpler and easier to understand.
> 
>  struct format_field {
>  	struct format_field	*next;
> 	struct event_format	*event;
> 	char			*type;
> 	char			*name;
> +	char			*alias;
> 	int			offset;
> 	int			size;
> 	unsigned int		arraylen;
> 	unsigned int		elementsize;
> 	unsigned long		flags;
> };
> 
> And put the logic in event parse to free alias if need be.
> Heck, you could even add a pevent_*() func to assign the alias
> using strdup, or what not.

ok, sounds good

thanks,
jirka