perf trace: Handle legacy syscalls

tools/perf/builtin-trace.c +13 -3

@@ -1442,14 +1442,24 @@ static int syscall__set_arg_fmts(struct syscall *sc)
 	struct format_field *field;
 	int idx = 0;
 
-	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields - 1, sizeof(void *));
+	field = sc->tp_format->format.fields;
+	/* drop nr field - not relevant here; does not exist on older kernels */
+	if (field && strcmp(field->name, "nr") == 0) {
+		struct format_field *next = field->next;
+
+		free_format_field(field);
+		sc->tp_format->format.fields = next;
+		sc->tp_format->format.nr_fields--;
+	}
+
+	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields, sizeof(void *));
 	if (sc->arg_scnprintf == NULL)
 		return -1;
 
 	if (sc->fmt)
 		sc->arg_parm = sc->fmt->arg_parm;
 
-	for (field = sc->tp_format->format.fields->next; field; field = field->next) {
+	for (field = sc->tp_format->format.fields; field; field = field->next) {
 		if (sc->fmt && sc->fmt->arg_scnprintf[idx])
 			sc->arg_scnprintf[idx] = sc->fmt->arg_scnprintf[idx];
 		else if (field->flags & FIELD_IS_POINTER)

@@ -1547,7 +1557,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
 			.thread = thread,
 		};
 
-		for (field = sc->tp_format->format.fields->next; field;
+		for (field = sc->tp_format->format.fields; field;
 		     field = field->next, ++arg.idx, bit <<= 1) {
 			if (arg.mask & bit)
 				continue;

Signature · 1 line

2.2.1

Em Thu, Mar 19, 2015 at 12:36:21PM -0600, David Ahern escreveu:

Quoted 54 lines

Currently the code skips the first field with the expectation that it is 'nr'. But older kernels do not have the 'nr' field:

    field:int nr;   offset:8;   size:4; signed:1;

Change perf-trace to drop the field if it exists after parsing the format file.

This fixes the off-by-one problem with older kernels (e.g., RHEL6). e.g, perf-trace shows this for write:

    1.515 ( 0.006 ms): dd/4245 write(buf: 2</dev/pts/0>, count: 140733837536224                      ) = 26

where 2 is really the fd, the huge number is really the buf address, etc. With this patch you get the more appropriate:

    1.813 ( 0.003 ms): dd/6330 write(fd: 2</dev/pts/0>, buf: 0x7fff22fc81f0, count: 25               ) = 25

Signed-off-by David Ahern <[email protected]>

---
 tools/lib/traceevent/event-parse.c | 11 ++++++++---
 tools/lib/traceevent/event-parse.h |  1 +
 tools/perf/builtin-trace.c         | 16 +++++++++++++---
 3 files changed, 22 insertions(+), 6 deletions(-)

tools/lib/traceevent/event-parse.c +8 -3

index afe20ed9fac8..e8a29e730dfb 100644
@@ -6228,15 +6228,20 @@ void pevent_ref(struct pevent *pevent)
 	pevent->ref_count++;
 }
 
+void free_format_field(struct format_field *field)
+{
+	free(field->type);
+	free(field->name);
+	free(field);
+}
+
 static void free_format_fields(struct format_field *field)
 {
 	struct format_field *next;
 
 	while (field) {
 		next = field->next;
-		free(field->type);
-		free(field->name);
-		free(field);
+		free_format_field(field);
 		field = next;
 	}
 }

tools/lib/traceevent/event-parse.h +0 -0

tools/lib/traceevent/event-parse.h enum pevent_errno pevent_parse_format(struct pevent *pevent, · line 622

Quoted 8 lines

index 5b4efc062320..a548fac646f6 100644

tools/lib/traceevent/event-parse.h +1 -0

@@ -619,6 +619,7 @@ enum pevent_errno pevent_parse_format(struct pevent *pevent,
 				      const char *buf,
 				      unsigned long size, const char *sys);
 void pevent_free_format(struct event_format *event);
+void free_format_field(struct format_field *field);

tools/perf/builtin-trace.c

Quoted 5 lines

tools/lib/traceevent/event-parse.h +0 -0

 void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
 			   const char *name, struct pevent_record *record,

tools/perf/builtin-trace.c +0 -0

index dcd950ef2fd7..5caeefeda48a 100644

Up to here we should have a separate patch, one that I would like to have an Acked-by: Rostedt, ok?

But then I don't think we need to do that, i.e. we can just have a boolean we set at some point to tell that we need to skip the first entry.

I'll try to cook up a patch for that.

Quoted 27 lines · On Fri, 20 Mar 2015 12:32:26 -0300 Arnaldo Carvalho de Melo <[email protected]>…

Em Thu, Mar 19, 2015 at 12:36:21PM -0600, David Ahern escreveu:

Quoted 26 lines

+++ b/tools/lib/traceevent/event-parse.c

tools/lib/traceevent/event-parse.c +8 -3

@@ -6228,15 +6228,20 @@ void pevent_ref(struct pevent *pevent)
 	pevent->ref_count++;
 }
 
+void free_format_field(struct format_field *field)
+{
+	free(field->type);
+	free(field->name);
+	free(field);
+}
+
 static void free_format_fields(struct format_field *field)
 {
 	struct format_field *next;
 
 	while (field) {
 		next = field->next;
-		free(field->type);
-		free(field->name);
-		free(field);
+		free_format_field(field);
 		field = next;
 	}
 }

tools/lib/traceevent/event-parse.h +0 -0

Quoted 8 lines

tools/lib/traceevent/event-parse.h enum pevent_errno pevent_parse_format(struct pevent *pevent, · line 622

Quoted 8 lines

index 5b4efc062320..a548fac646f6 100644

tools/lib/traceevent/event-parse.h +1 -0

@@ -619,6 +619,7 @@ enum pevent_errno pevent_parse_format(struct pevent *pevent,
 				      const char *buf,
 				      unsigned long size, const char *sys);
 void pevent_free_format(struct event_format *event);
+void free_format_field(struct format_field *field);

Can we make it "pevent_free_format_field" to keep consistency.

Quoted 9 lines

tools/perf/builtin-trace.c

Quoted 5 lines

tools/lib/traceevent/event-parse.h +0 -0

 void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
 			   const char *name, struct pevent_record *record,

tools/perf/builtin-trace.c +0 -0

index dcd950ef2fd7..5caeefeda48a 100644

Up to here we should have a separate patch, one that I would like to have an Acked-by: Rostedt, ok?

I'm fine with the change if the name of the function is updated.

Ok, so I'll put it in a separate patch, authored by David, with your Acked-by, it may be useful at some point besides being used in the libtraceevent internal free_format_fields() static function.

But then I don't think we need to do that, i.e. we can just have a boolean we set at some point to tell that we need to skip the first entry.

I'll try to cook up a patch for that.

Quoted 5 lines

But then I don't think we need to do that, i.e. we can just have a boolean we set at some point to tell that we need to skip the first entry.

I'll try to cook up a patch for that.

why have a boolean that is checked every time through the loop when its value will always be the same for a give run? why not just remove the entry as I suggested?

Em Fri, Mar 20, 2015 at 09:48:10AM -0600, David Ahern escreveu:

Quoted 10 lines · Arnaldo Carvalho de Melo

Quoted 5 lines

But then I don't think we need to do that, i.e. we can just have a boolean we set at some point to tell that we need to skip the first entry.

I'll try to cook up a patch for that.

why have a boolean that is checked every time through the loop when its value will always be the same for a give run? why not just remove the entry as I suggested?

First gut reaction was: hey, we got that from a library, libtraceevent, that could have other users, etc, better not to change it behind its back.

I.e. something like this, based on your approach, but not modifying the data structures received from libtraceevent:

tools/perf/builtin-trace.c +13 -3

index 6af6bcec930e..001c6ae9a1b1 100644
@@ -1135,6 +1135,8 @@ static struct syscall_fmt *syscall_fmt__find(const char *name)
 
  struct syscall {
  	struct event_format *tp_format;
+	int		    nr_args;
+	struct format_field *args;
  	const char	    *name;
  	bool		    filtered;
  	bool		    is_exit;

@@ -1442,14 +1444,14 @@ static int syscall__set_arg_fmts(struct syscall *sc)
  	struct format_field *field;
  	int idx = 0;
 
-	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields - 1, sizeof(void *));
+	sc->arg_scnprintf = calloc(sc->nr_args, sizeof(void *));
  	if (sc->arg_scnprintf == NULL)
  		return -1;
 
  	if (sc->fmt)
  		sc->arg_parm = sc->fmt->arg_parm;
 
-	for (field = sc->tp_format->format.fields->next; field; field = field->next) {
+	for (field = sc->args; field; field = field->next) {
  		if (sc->fmt && sc->fmt->arg_scnprintf[idx])
  			sc->arg_scnprintf[idx] = sc->fmt->arg_scnprintf[idx];
  		else if (field->flags & FIELD_IS_POINTER)

@@ -1515,6 +1517,14 @@ static int trace__read_syscall_info(struct trace *trace, int id)
  	if (sc->tp_format == NULL)
  		return -1;
 
+	sc->args = sc->tp_format->format.fields;
+	sc->nr_args = sc->tp_format->format.nr_fields;
+	/* drop nr field - not relevant here; does not exist on older kernels */
+	if (sc->args && strcmp(sc->args->name, "nr") == 0) {
+		sc->args = sc->args->next;
+		--sc->nr_args;
+	}
+
  	sc->is_exit = !strcmp(name, "exit_group") || !strcmp(name, "exit");
 
  	return syscall__set_arg_fmts(sc);

@@ -1537,7 +1547,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
  	unsigned char *p;
  	unsigned long val;
 
-	if (sc->tp_format != NULL) {
+	if (sc->args != NULL) {
  		struct format_field *field;
  		u8 bit = 1;
  		struct syscall_arg arg = {

tools/perf/builtin-trace.c static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size, · line 1563

Quoted 6 lines

tools/perf/builtin-trace.c +1 -1

-		for (field = sc->tp_format->format.fields->next; field;
+		for (field = sc->args; field;
 		     field = field->next, ++arg.idx, bit <<= 1) {
 			if (arg.mask & bit)
 				continue;

Seems reasonable.

Em Fri, Mar 20, 2015 at 10:12:33AM -0600, David Ahern escreveu:

Quoted 9 lines · Arnaldo Carvalho de Melo

tools/perf/builtin-trace.c static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size, · line 1563

Quoted 6 lines

tools/perf/builtin-trace.c +1 -1

-		for (field = sc->tp_format->format.fields->next; field;
+		for (field = sc->args; field;
  		     field = field->next, ++arg.idx, bit <<= 1) {
  			if (arg.mask & bit)
  				continue;

Seems reasonable.

Ok, so I'll add this one, with a Based-on-a-patch-by: you, with your Acked-by, ok?