[PATCH 0/5] Squelch warnings emitted by 'perf test'

[PATCH 0/5] Squelch warnings emitted by 'perf test'

[Ramkumar Ramachandra]

Hi,

The motivation for the series is mainly [2/5] and [5/5] -- to squelch
the warnings emitted by 'perf test'. I made some effort to understand
what's going on, but I didn't get very far. [5/5] feels especially
like a band-aid fix: please let me know how to do it right.

Thanks.

Ramkumar Ramachandra (5):
  MAINTAINERS: add tools/lib/traceevent/ to perf subsystem
  tools lib traceevent: handle the '->' operator
  tools lib traceevent: use else-if cascade, not separate ifs
  tools lib traceevent: field_is_long() includes unsigned long
  perf test: squelch warnings about undefined sizeof

 MAINTAINERS                        |  1 +
 tools/lib/traceevent/event-parse.c | 23 +++++++++++++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

-- 
1.9.0.431.g014438b

[PATCH 1/5] MAINTAINERS: add tools/lib/traceevent/ to perf subsystem

[Ramkumar Ramachandra]

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b3fdb0f..788effd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6693,6 +6693,7 @@ F:	arch/*/kernel/*/*/perf_event*.c
 F:	arch/*/include/asm/perf_event.h
 F:	arch/*/kernel/perf_callchain.c
 F:	tools/perf/
+F:	tools/lib/traceevent/
 
 PERSONALITY HANDLING
 M:	Christoph Hellwig <hch@infradead.org>
-- 
1.9.0.431.g014438b

Re: [PATCH 1/5] MAINTAINERS: add tools/lib/traceevent/ to perf subsystem

[Steven Rostedt]

On Mon, 17 Mar 2014 18:26:34 -0400
Ramkumar Ramachandra <artagnon@gmail.com> wrote:

> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b3fdb0f..788effd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6693,6 +6693,7 @@ F:	arch/*/kernel/*/*/perf_event*.c
>  F:	arch/*/include/asm/perf_event.h
>  F:	arch/*/kernel/perf_callchain.c
>  F:	tools/perf/
> +F:	tools/lib/traceevent/

If this goes in, I probably should get my name added as well here.
Note, I'm currently working on some clean ups to the perf system on the
kernel side.

-- Steve

>  
>  PERSONALITY HANDLING
>  M:	Christoph Hellwig <hch@infradead.org>

[PATCH 2/5] tools lib traceevent: handle the '->' operator

[Ramkumar Ramachandra]

perf test emits the following warning:

  $ perf test
   5: parse events tests
    ...
    Warning: unknown op '->'
    ...

Add the operator to the checks.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/lib/traceevent/event-parse.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 1587ea39..42bc571 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -1759,6 +1759,8 @@ static int get_op_prio(char *op)
 			return 14;
 		} else if (strcmp(op, "||") == 0) {
 			return 15;
+		} else if (strcmp(op, "->") == 0) {
+			return 17;
 		} else {
 			do_warning("unknown op '%s'", op);
 			return -1;
@@ -1858,7 +1860,8 @@ process_op(struct event_format *event, struct print_arg *arg, char **tok)
 		   strcmp(token, "<=") == 0 ||
 		   strcmp(token, ">=") == 0 ||
 		   strcmp(token, "==") == 0 ||
-		   strcmp(token, "!=") == 0) {
+		   strcmp(token, "!=") == 0 ||
+		   strcmp(token, "->") == 0) {
 
 		left = alloc_arg();
 		if (!left)
-- 
1.9.0.431.g014438b

Re: [PATCH 2/5] tools lib traceevent: handle the '->' operator

[Ramkumar Ramachandra]

Hi Namhyung,

On Tue, Mar 18, 2014 at 3:58 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> Hmm.. AFAIK we don't support the '->' operator so I think it's an error
> in the callsite.  Where do you see the message though?  I couldn't find
> it on my setup.
>
> I'd also like to add following patch to see the location of parse
> failure easily.

I applied your patch and ran 'perf test'. I get:

  Warning: [i915:i915_gem_evict_vm] unknown op '->'

So, where should we dig to find this callsite?

Ram

Re: [PATCH 2/5] tools lib traceevent: handle the '->' operator

[Namhyung Kim]

Hi Ramkumar,

On Tue, Mar 18, 2014 at 11:12 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Hi Namhyung,
>
> On Tue, Mar 18, 2014 at 3:58 AM, Namhyung Kim <namhyung@kernel.org> wrote:
>> Hmm.. AFAIK we don't support the '->' operator so I think it's an error
>> in the callsite.  Where do you see the message though?  I couldn't find
>> it on my setup.
>>
>> I'd also like to add following patch to see the location of parse
>> failure easily.
>
> I applied your patch and ran 'perf test'. I get:
>
>   Warning: [i915:i915_gem_evict_vm] unknown op '->'
>
> So, where should we dig to find this callsite?

/sys/kernel/debug/tracing/events/i915/i915_gen_evict_vm/format :)


Thanks,
Namhyung

Re: [PATCH 2/5] tools lib traceevent: handle the '->' operator

[Steven Rostedt]

On Tue, 18 Mar 2014 23:18:24 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Ramkumar,
> 
> On Tue, Mar 18, 2014 at 11:12 PM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
> > Hi Namhyung,
> >
> > On Tue, Mar 18, 2014 at 3:58 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> >> Hmm.. AFAIK we don't support the '->' operator so I think it's an error
> >> in the callsite.  Where do you see the message though?  I couldn't find
> >> it on my setup.
> >>
> >> I'd also like to add following patch to see the location of parse
> >> failure easily.
> >
> > I applied your patch and ran 'perf test'. I get:
> >
> >   Warning: [i915:i915_gem_evict_vm] unknown op '->'
> >
> > So, where should we dig to find this callsite?
> 
> /sys/kernel/debug/tracing/events/i915/i915_gen_evict_vm/format :)
> 

Actually, that's a bug in the kernel. You should *NEVER* redirect from
the ring buffer. There's no guarantee that the data still exists and
this can cause a kernel oops.

-- Steve

[PATCH 3/5] tools lib traceevent: use else-if cascade, not separate ifs

[Ramkumar Ramachandra]

When token cannot be more than one value, it seems wasteful to go
through all the strcmp() calls. Use an else-if cascade instead.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/lib/traceevent/event-parse.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 42bc571..7a8d9ae 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -2770,21 +2770,17 @@ process_function(struct event_format *event, struct print_arg *arg,
 		free_token(token);
 		is_flag_field = 1;
 		return process_flags(event, arg, tok);
-	}
-	if (strcmp(token, "__print_symbolic") == 0) {
+	} else if (strcmp(token, "__print_symbolic") == 0) {
 		free_token(token);
 		is_symbolic_field = 1;
 		return process_symbols(event, arg, tok);
-	}
-	if (strcmp(token, "__print_hex") == 0) {
+	} else if (strcmp(token, "__print_hex") == 0) {
 		free_token(token);
 		return process_hex(event, arg, tok);
-	}
-	if (strcmp(token, "__get_str") == 0) {
+	} else if (strcmp(token, "__get_str") == 0) {
 		free_token(token);
 		return process_str(event, arg, tok);
-	}
-	if (strcmp(token, "__get_dynamic_array") == 0) {
+	} else if (strcmp(token, "__get_dynamic_array") == 0) {
 		free_token(token);
 		return process_dynamic_array(event, arg, tok);
 	}
-- 
1.9.0.431.g014438b

Re: [PATCH 3/5] tools lib traceevent: use else-if cascade, not separate ifs

[Arnaldo Carvalho de Melo]

Em Tue, Mar 18, 2014 at 05:02:09PM +0900, Namhyung Kim escreveu:
> On Mon, 17 Mar 2014 18:26:36 -0400, Ramkumar Ramachandra wrote:
> > When token cannot be more than one value, it seems wasteful to go
> > through all the strcmp() calls. Use an else-if cascade instead.
> 
> I think the end result will be same since it returns from inside the
> block, right?

Exactly :-)

We could have it changed to something like:

	if (strcmp()) {
		ret = bla();
		goto out_free_token;
	}
.
.
.
out_free_token:
	free_token(token);
	return ret;

 
> Thanks,
> Namhyung
> 
> >
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> > ---
> >  tools/lib/traceevent/event-parse.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > index 42bc571..7a8d9ae 100644
> > --- a/tools/lib/traceevent/event-parse.c
> > +++ b/tools/lib/traceevent/event-parse.c
> > @@ -2770,21 +2770,17 @@ process_function(struct event_format *event, struct print_arg *arg,
> >  		free_token(token);
> >  		is_flag_field = 1;
> >  		return process_flags(event, arg, tok);
> > -	}
> > -	if (strcmp(token, "__print_symbolic") == 0) {
> > +	} else if (strcmp(token, "__print_symbolic") == 0) {
> >  		free_token(token);
> >  		is_symbolic_field = 1;
> >  		return process_symbols(event, arg, tok);
> > -	}
> > -	if (strcmp(token, "__print_hex") == 0) {
> > +	} else if (strcmp(token, "__print_hex") == 0) {
> >  		free_token(token);
> >  		return process_hex(event, arg, tok);
> > -	}
> > -	if (strcmp(token, "__get_str") == 0) {
> > +	} else if (strcmp(token, "__get_str") == 0) {
> >  		free_token(token);
> >  		return process_str(event, arg, tok);
> > -	}
> > -	if (strcmp(token, "__get_dynamic_array") == 0) {
> > +	} else if (strcmp(token, "__get_dynamic_array") == 0) {
> >  		free_token(token);
> >  		return process_dynamic_array(event, arg, tok);
> >  	}

Re: [PATCH 3/5] tools lib traceevent: use else-if cascade, not separate ifs

[Steven Rostedt]

On Tue, 18 Mar 2014 10:10:48 -0300
Arnaldo Carvalho de Melo <acme@redhat.com> wrote:

> Em Tue, Mar 18, 2014 at 05:02:09PM +0900, Namhyung Kim escreveu:
> > On Mon, 17 Mar 2014 18:26:36 -0400, Ramkumar Ramachandra wrote:
> > > When token cannot be more than one value, it seems wasteful to go
> > > through all the strcmp() calls. Use an else-if cascade instead.
> > 
> > I think the end result will be same since it returns from inside the
> > block, right?
> 
> Exactly :-)
> 
> We could have it changed to something like:
> 
> 	if (strcmp()) {
> 		ret = bla();
> 		goto out_free_token;
> 	}
> .
> .
> .
> out_free_token:
> 	free_token(token);
> 	return ret;
> 
>  

Honestly, I prefer the original, thus I would add a NAK to this patch.

-- Steve

Re: [PATCH 3/5] tools lib traceevent: use else-if cascade, not separate ifs

[Arnaldo Carvalho de Melo]

Em Tue, Mar 18, 2014 at 09:30:06AM -0400, Steven Rostedt escreveu:
> On Tue, 18 Mar 2014 10:10:48 -0300 Arnaldo Carvalho de Melo <acme@redhat.com> wrote:
> > Em Tue, Mar 18, 2014 at 05:02:09PM +0900, Namhyung Kim escreveu:
> > > On Mon, 17 Mar 2014 18:26:36 -0400, Ramkumar Ramachandra wrote:
> > > > When token cannot be more than one value, it seems wasteful to go
> > > > through all the strcmp() calls. Use an else-if cascade instead.

> > > I think the end result will be same since it returns from inside the
> > > block, right?
> > 
> > Exactly :-)
> > 
> > We could have it changed to something like:
> > 
> > 	if (strcmp()) {
> > 		ret = bla();
> > 		goto out_free_token;
> > 	}
> > .
> > .
> > .
> > out_free_token:
> > 	free_token(token);
> > 	return ret;

> Honestly, I prefer the original, thus I would add a NAK to this patch.

No problem with that, if I had written that code, I'd prefer the gotos,
but that is kinda personal preference.

Yeah, I'd nack that patch as well, changes nothing.

- Arnaldo

[PATCH 4/5] tools lib traceevent: field_is_long() includes unsigned long

[Ramkumar Ramachandra]

Note in the comment that field_is_long() also checks for "unsigned
long" and "unsigned long long" in addition to "long long".

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/lib/traceevent/event-parse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 7a8d9ae..567d9ba 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -1233,7 +1233,7 @@ static int field_is_dynamic(struct format_field *field)
 
 static int field_is_long(struct format_field *field)
 {
-	/* includes long long */
+	/* includes long long, unsigned long, and unsigned long long */
 	if (strstr(field->type, "long"))
 		return 1;
 
-- 
1.9.0.431.g014438b

[PATCH 5/5] perf test: squelch warnings about undefined sizeof

[Ramkumar Ramachandra]

perf test emits the following warnings on the parse events test:

  $ perf test
   5: parse events tests
    Warning: function sizeof not defined
    Warning: function sizeof not defined
    Warning: function sizeof not defined
    Warning: function sizeof not defined
    Warning: function sizeof not defined
    Warning: function sizeof not defined
    Warning: function sizeof not defined
    Warning: function sizeof not defined
    ...

Squelch the warnings by explicitly ignoring the sizeof function.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Probably incorrect.

 tools/lib/traceevent/event-parse.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 567d9ba..06c269a 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -2783,6 +2783,10 @@ process_function(struct event_format *event, struct print_arg *arg,
 	} else if (strcmp(token, "__get_dynamic_array") == 0) {
 		free_token(token);
 		return process_dynamic_array(event, arg, tok);
+	} else if (strcmp(token, "sizeof") == 0) {
+		/* ignore sizeof function */
+		free_token(token);
+		return 0;
 	}
 
 	func = find_func_handler(event->pevent, token);
-- 
1.9.0.431.g014438b

Re: [PATCH 5/5] perf test: squelch warnings about undefined sizeof

[Jiri Olsa]

On Tue, Mar 18, 2014 at 05:09:33PM +0900, Namhyung Kim wrote:
> On Mon, 17 Mar 2014 18:26:38 -0400, Ramkumar Ramachandra wrote:
> > perf test emits the following warnings on the parse events test:
> >
> >   $ perf test
> >    5: parse events tests
> >     Warning: function sizeof not defined
> >     Warning: function sizeof not defined
> >     Warning: function sizeof not defined
> >     Warning: function sizeof not defined
> >     Warning: function sizeof not defined
> >     Warning: function sizeof not defined
> >     Warning: function sizeof not defined
> >     Warning: function sizeof not defined
> >     ...
> >
> > Squelch the warnings by explicitly ignoring the sizeof function.
> 
> It just hides the warning leaving the real problem untouched.  If you
> really don't want to see those, I guess installing proper plugin for the
> failing events will help you (in case you didn't).

it wont' help, sizeof is special.. AFAIK we did not figure
out a way to handle that so far

but I think it was used only in one trace subsystem.. maybe we
could replace it and forbid to use it in future ;-)

jirka

Re: [PATCH 5/5] perf test: squelch warnings about undefined sizeof

[Namhyung Kim]

Hi Jiri,

On Tue, Mar 18, 2014 at 10:23 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Tue, Mar 18, 2014 at 05:09:33PM +0900, Namhyung Kim wrote:
>> On Mon, 17 Mar 2014 18:26:38 -0400, Ramkumar Ramachandra wrote:
>> > perf test emits the following warnings on the parse events test:
>> >
>> >   $ perf test
>> >    5: parse events tests
>> >     Warning: function sizeof not defined
>> >     Warning: function sizeof not defined
>> >     Warning: function sizeof not defined
>> >     Warning: function sizeof not defined
>> >     Warning: function sizeof not defined
>> >     Warning: function sizeof not defined
>> >     Warning: function sizeof not defined
>> >     Warning: function sizeof not defined
>> >     ...
>> >
>> > Squelch the warnings by explicitly ignoring the sizeof function.
>>
>> It just hides the warning leaving the real problem untouched.  If you
>> really don't want to see those, I guess installing proper plugin for the
>> failing events will help you (in case you didn't).
>
> it wont' help, sizeof is special.. AFAIK we did not figure
> out a way to handle that so far
>
> but I think it was used only in one trace subsystem.. maybe we
> could replace it and forbid to use it in future ;-)

Yep, I know.  But I meant that if we have a handler for the failing
event, it might print the event without even using the sizeof somehow.
;-)

Thanks,
Namhyung

Re: [PATCH 5/5] perf test: squelch warnings about undefined sizeof

[Ramkumar Ramachandra]

Jiri Olsa wrote:
>> > Squelch the warnings by explicitly ignoring the sizeof function.
>>
>> It just hides the warning leaving the real problem untouched.  If you
>> really don't want to see those, I guess installing proper plugin for the
>> failing events will help you (in case you didn't).
>
> it wont' help, sizeof is special.. AFAIK we did not figure
> out a way to handle that so far
>
> but I think it was used only in one trace subsystem.. maybe we
> could replace it and forbid to use it in future ;-)

It only seems to be used by xen:

  Warning: [xen:xen_mmu_set_pte] function sizeof not defined
  Warning: [xen:xen_mmu_set_pte_atomic] function sizeof not defined
  Warning: [xen:xen_mmu_set_domain_pte] function sizeof not defined
  Warning: [xen:xen_mmu_set_pte_at] function sizeof not defined
  Warning: [xen:xen_mmu_set_pmd] function sizeof not defined
  Warning: [xen:xen_mmu_set_pud] function sizeof not defined
  Warning: [xen:xen_mmu_set_pgd] function sizeof not defined
  Warning: [xen:xen_mmu_ptep_modify_prot_start] function sizeof not defined
  Warning: [xen:xen_mmu_ptep_modify_prot_commit] function sizeof not defined

These events are defined in include/trace/events/xen.h. Any hints on
how to proceed?