Re: [PATCH] perf trace: Handle legacy syscalls

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 20 Mar 2015 12:32:26 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Thu, Mar 19, 2015 at 12:36:21PM -0600, David Ahern escreveu:
> > 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 <dsahern@gmail.com>
> > ---
> >  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(-)
> > 
> > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > index afe20ed9fac8..e8a29e730dfb 100644
> > --- a/tools/lib/traceevent/event-parse.c
> > +++ b/tools/lib/traceevent/event-parse.c
> > @@ -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;
> >  	}
> >  }
> > diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> > index 5b4efc062320..a548fac646f6 100644
> > --- a/tools/lib/traceevent/event-parse.h
> > +++ b/tools/lib/traceevent/event-parse.h
> > @@ -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.

> >  
> >  void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
> >  			   const char *name, struct pevent_record *record,
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > 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.

-- Steve

> 
> 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.