[RFC/PATCH] perf: Add sizeof operator support

[RFC/PATCH] perf: Add sizeof operator support

[Jeremy Linton]

There are a fair number of tracepoints in the kernel making
use of the sizeof operator. Allow perf to understand some of
those cases, and report a more informative error message for
the ones it cannot understand.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---

So this is as much a RFC as a patch because the use of sizeof
seems to extend to structures, pointers, etc that aren't easy
to deduce from userspace. I'm not sure what the correct solution
should be in those cases.

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

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index a8b6357..5813248 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -31,6 +31,8 @@
 #include <errno.h>
 #include <stdint.h>
 #include <limits.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
 
 #include <netinet/ip6.h>
 #include "event-parse.h"
@@ -2868,6 +2870,46 @@ process_str(struct event_format *event __maybe_unused, struct print_arg *arg,
 }
 
 static enum event_type
+process_sizeof(struct event_format *event __maybe_unused, struct print_arg *arg,
+	       char **tok)
+{
+	char *token;
+	char *atom;
+
+	if (process_paren(event, arg, &token) < 0)
+		goto out_free;
+
+	atom = arg->atom.atom;
+	if (arg->type != PRINT_ATOM) {
+		do_warning_event(event, "didn't understand %s for sizeof\n",
+				 token);
+		goto out_free_atom;
+	}
+
+	if (strcmp(token, "__u64") == 0) {
+		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u64)) < 0)
+			goto out_free_atom;
+	} else if (strcmp(token, "__u32") == 0) {
+		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u32)) < 0)
+			goto out_free_atom;
+	} else {
+		do_warning_event(event, "unknown sizeof %s\n", token);
+		goto out_free_atom;
+	}
+
+	free(atom);
+	*tok = token;
+	return EVENT_DELIM;
+
+ out_free_atom:
+	free_token(atom);
+ out_free:
+	free_token(token);
+	*tok = NULL;
+	return EVENT_ERROR;
+}
+
+static enum event_type
 process_bitmask(struct event_format *event __maybe_unused, struct print_arg *arg,
 	    char **tok)
 {
@@ -3026,6 +3068,10 @@ process_function(struct event_format *event, struct print_arg *arg,
 		free_token(token);
 		return process_dynamic_array_len(event, arg, tok);
 	}
+	if (strcmp(token, "sizeof") == 0) {
+		free_token(token);
+		return process_sizeof(event, arg, tok);
+	}
 
 	func = find_func_handler(event->pevent, token);
 	if (func) {
-- 
2.5.5

Re: [RFC/PATCH] perf: Add sizeof operator support

[Steven Rostedt]

On Tue, 14 Jun 2016 11:38:32 -0500
Jeremy Linton <jeremy.linton@arm.com> wrote:

> There are a fair number of tracepoints in the kernel making
> use of the sizeof operator. Allow perf to understand some of
> those cases, and report a more informative error message for
> the ones it cannot understand.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> 
> So this is as much a RFC as a patch because the use of sizeof
> seems to extend to structures, pointers, etc that aren't easy
> to deduce from userspace. I'm not sure what the correct solution
> should be in those cases.
> 
>  tools/lib/traceevent/event-parse.c | 46 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index a8b6357..5813248 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -31,6 +31,8 @@
>  #include <errno.h>
>  #include <stdint.h>
>  #include <limits.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
>  
>  #include <netinet/ip6.h>
>  #include "event-parse.h"
> @@ -2868,6 +2870,46 @@ process_str(struct event_format *event __maybe_unused, struct print_arg *arg,
>  }
>  
>  static enum event_type
> +process_sizeof(struct event_format *event __maybe_unused, struct print_arg *arg,
> +	       char **tok)
> +{
> +	char *token;
> +	char *atom;
> +
> +	if (process_paren(event, arg, &token) < 0)
> +		goto out_free;
> +
> +	atom = arg->atom.atom;
> +	if (arg->type != PRINT_ATOM) {
> +		do_warning_event(event, "didn't understand %s for sizeof\n",
> +				 token);
> +		goto out_free_atom;
> +	}
> +
> +	if (strcmp(token, "__u64") == 0) {
> +		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u64)) < 0)
> +			goto out_free_atom;
> +	} else if (strcmp(token, "__u32") == 0) {
> +		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u32)) < 0)
> +			goto out_free_atom;

What events are doing sizeof(__u64) and sizeof(__u32)?

First, that's useless, as sizeof(__u64) will always be 8, and
sizeof(__u32) will always be 4.

What exactly is this fixing?

-- Steve


> +	} else {
> +		do_warning_event(event, "unknown sizeof %s\n", token);
> +		goto out_free_atom;
> +	}
> +
> +	free(atom);
> +	*tok = token;
> +	return EVENT_DELIM;
> +
> + out_free_atom:
> +	free_token(atom);
> + out_free:
> +	free_token(token);
> +	*tok = NULL;
> +	return EVENT_ERROR;
> +}
> +
> +static enum event_type
>  process_bitmask(struct event_format *event __maybe_unused, struct print_arg *arg,
>  	    char **tok)
>  {
> @@ -3026,6 +3068,10 @@ process_function(struct event_format *event, struct print_arg *arg,
>  		free_token(token);
>  		return process_dynamic_array_len(event, arg, tok);
>  	}
> +	if (strcmp(token, "sizeof") == 0) {
> +		free_token(token);
> +		return process_sizeof(event, arg, tok);
> +	}
>  
>  	func = find_func_handler(event->pevent, token);
>  	if (func) {

Re: [RFC/PATCH] perf: Add sizeof operator support

[Jeremy Linton]

Hi Steven,

On 06/17/2016 11:17 AM, Steven Rostedt wrote:
> On Tue, 14 Jun 2016 11:38:32 -0500
> Jeremy Linton <jeremy.linton@arm.com> wrote:
>
>> There are a fair number of tracepoints in the kernel making
>> use of the sizeof operator. Allow perf to understand some of
>> those cases, and report a more informative error message for
>> the ones it cannot understand.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>
>> So this is as much a RFC as a patch because the use of sizeof
>> seems to extend to structures, pointers, etc that aren't easy
>> to deduce from userspace. I'm not sure what the correct solution
>> should be in those cases.
>>
>>   tools/lib/traceevent/event-parse.c | 46 ++++++++++++++++++++++++++++++++++++++

(trimming)

>> +
>> +	if (strcmp(token, "__u64") == 0) {
>> +		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u64)) < 0)
>> +			goto out_free_atom;
>> +	} else if (strcmp(token, "__u32") == 0) {
>> +		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u32)) < 0)
>> +			goto out_free_atom;
>
> What events are doing sizeof(__u64) and sizeof(__u32)?
>
> First, that's useless, as sizeof(__u64) will always be 8, and
> sizeof(__u32) will always be 4.
>
> What exactly is this fixing?

It starts to fix things like:

kmem:mm_page_alloc
   Warning: [kmem:mm_page_alloc] function sizeof not defined

or:

# perf stat -e kvm:kvm_arm_set_regset -- true
   Warning: [kvm:kvm_arm_set_regset] function sizeof not defined
   Warning: Error: expected type 5 but read 0
*** Error in `perf': double free or corruption (fasttop): 
0x00000000303f5930 ***

There is a RH bug about it (and the "~" operator, which has been fixed) 
here: https://bugzilla.redhat.com/show_bug.cgi?id=1298229

Thanks for taking a look at this,

Re: [RFC/PATCH] perf: Add sizeof operator support

[Steven Rostedt]

On Fri, 17 Jun 2016 11:32:08 -0500
Jeremy Linton <jeremy.linton@arm.com> wrote:


> >> +
> >> +	if (strcmp(token, "__u64") == 0) {
> >> +		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u64)) < 0)
> >> +			goto out_free_atom;
> >> +	} else if (strcmp(token, "__u32") == 0) {
> >> +		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u32)) < 0)
> >> +			goto out_free_atom;  
> >
> > What events are doing sizeof(__u64) and sizeof(__u32)?
> >
> > First, that's useless, as sizeof(__u64) will always be 8, and
> > sizeof(__u32) will always be 4.
> >
> > What exactly is this fixing?  
> 
> It starts to fix things like:
> 
> kmem:mm_page_alloc
>    Warning: [kmem:mm_page_alloc] function sizeof not defined

I don't see any sizeof() calls in my format files. And still, its
useless to add sizeof() for __u64 and __u32 unless perhaps a type is a
macro defined to that.

Ah, this is arm64 (as I don't see it in x86).

No the real fix is to nuke the sizeof(__u64) in the TP_printk(), it's
useless because it will always be 8.

-- Steve


> 
> or:
> 
> # perf stat -e kvm:kvm_arm_set_regset -- true
>    Warning: [kvm:kvm_arm_set_regset] function sizeof not defined
>    Warning: Error: expected type 5 but read 0
> *** Error in `perf': double free or corruption (fasttop): 
> 0x00000000303f5930 ***
> 
> There is a RH bug about it (and the "~" operator, which has been fixed) 
> here: https://bugzilla.redhat.com/show_bug.cgi?id=1298229
> 
> Thanks for taking a look at this,

Re: [RFC/PATCH] perf: Add sizeof operator support

[Jeremy Linton]

On 06/17/2016 11:50 AM, Steven Rostedt wrote:
> On Fri, 17 Jun 2016 11:32:08 -0500
> Jeremy Linton <jeremy.linton@arm.com> wrote:
>
>
>>>> +
>>>> +	if (strcmp(token, "__u64") == 0) {
>>>> +		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u64)) < 0)
>>>> +			goto out_free_atom;
>>>> +	} else if (strcmp(token, "__u32") == 0) {
>>>> +		if (asprintf(&arg->atom.atom, "%zd", sizeof(__u32)) < 0)
>>>> +			goto out_free_atom;
>>>
>>> What events are doing sizeof(__u64) and sizeof(__u32)?
>>>
>>> First, that's useless, as sizeof(__u64) will always be 8, and
>>> sizeof(__u32) will always be 4.
>>>
>>> What exactly is this fixing?
>>
>> It starts to fix things like:
>>
>> kmem:mm_page_alloc
>>     Warning: [kmem:mm_page_alloc] function sizeof not defined
>
> I don't see any sizeof() calls in my format files. And still, its
> useless to add sizeof() for __u64 and __u32 unless perhaps a type is a
> macro defined to that.
>
> Ah, this is arm64 (as I don't see it in x86).
>
> No the real fix is to nuke the sizeof(__u64) in the TP_printk(), it's
> useless because it will always be 8.

That is the simple case, initially I was going to just hand code some of 
the sizeofs in the kernel, but then I started noticing more complex 
cases, and why I RFCed this patch.

For example, on x64/xen there are fair number with sizeof(pXXval_t), 
IIRC I've also seen a fair number of sizeof(struct page *). Some, but I 
dont think all of these case be determined from the field sizes like 
this one:

[root@X tracing]# cat events/xen/xen_mmu_set_pte/format
name: xen_mmu_set_pte
ID: 45
format:
         field:unsigned short common_type;       offset:0;       size:2; 
signed:0;
         field:unsigned char common_flags;       offset:2;       size:1; 
signed:0;
         field:unsigned char common_preempt_count;       offset:3; 
  size:1; signed:0;
         field:int common_pid;   offset:4;       size:4; signed:1;

         field:pte_t * ptep;     offset:8;       size:8; signed:0;
         field:pteval_t pteval;  offset:16;      size:8; signed:0;

print fmt: "ptep %p pteval %0*llx (raw %0*llx)", REC->ptep, 
(int)sizeof(pteval_t) * 2, (unsigned long 
long)pte_val(native_make_pte(REC->pteval)), (int)sizeof(pteval_t) * 2, 
(unsigned long long)REC->pteval

Re: [RFC/PATCH] perf: Add sizeof operator support

[Steven Rostedt]

On Fri, 17 Jun 2016 13:57:44 -0500
Jeremy Linton <jeremy.linton@arm.com> wrote:


> That is the simple case, initially I was going to just hand code some of 
> the sizeofs in the kernel, but then I started noticing more complex 
> cases, and why I RFCed this patch.
> 
> For example, on x64/xen there are fair number with sizeof(pXXval_t), 
> IIRC I've also seen a fair number of sizeof(struct page *). Some, but I 
> dont think all of these case be determined from the field sizes like 
> this one:

Right, but there's no easy fix for that. Your patch wont fix these,
because they can change over time.

Now, what we can do is add a sizeof() helper that is like the
TRACE_DEFINE_ENUM() macro. We could add a TRACE_DEFINE_SIZEOF(), that
basically does the same, and in update_event_printk() we could
substitute the sizeof() with the actual number value.

You want to take a crack at that?

Take a look at commit 0c564a538aa93.

-- Steve


> 
> [root@X tracing]# cat events/xen/xen_mmu_set_pte/format
> name: xen_mmu_set_pte
> ID: 45
> format:
>          field:unsigned short common_type;       offset:0;       size:2; 
> signed:0;
>          field:unsigned char common_flags;       offset:2;       size:1; 
> signed:0;
>          field:unsigned char common_preempt_count;       offset:3; 
>   size:1; signed:0;
>          field:int common_pid;   offset:4;       size:4; signed:1;
> 
>          field:pte_t * ptep;     offset:8;       size:8; signed:0;
>          field:pteval_t pteval;  offset:16;      size:8; signed:0;
> 
> print fmt: "ptep %p pteval %0*llx (raw %0*llx)", REC->ptep, 
> (int)sizeof(pteval_t) * 2, (unsigned long 
> long)pte_val(native_make_pte(REC->pteval)), (int)sizeof(pteval_t) * 2, 
> (unsigned long long)REC->pteval
> 

Re: [RFC/PATCH] perf: Add sizeof operator support

[Namhyung Kim]

On Fri, Jun 17, 2016 at 03:08:27PM -0400, Steven Rostedt wrote:
> On Fri, 17 Jun 2016 13:57:44 -0500
> Jeremy Linton <jeremy.linton@arm.com> wrote:
> 
> 
> > That is the simple case, initially I was going to just hand code some of 
> > the sizeofs in the kernel, but then I started noticing more complex 
> > cases, and why I RFCed this patch.
> > 
> > For example, on x64/xen there are fair number with sizeof(pXXval_t), 
> > IIRC I've also seen a fair number of sizeof(struct page *). Some, but I 
> > dont think all of these case be determined from the field sizes like 
> > this one:
> 
> Right, but there's no easy fix for that. Your patch wont fix these,
> because they can change over time.
> 
> Now, what we can do is add a sizeof() helper that is like the
> TRACE_DEFINE_ENUM() macro. We could add a TRACE_DEFINE_SIZEOF(), that
> basically does the same, and in update_event_printk() we could
> substitute the sizeof() with the actual number value.
> 
> You want to take a crack at that?
> 
> Take a look at commit 0c564a538aa93.

Or, maybe we can limit the use of sizeof() to the format fields and
obvious simple types only which we already know the size.  Just an
idea..

Thanks,
Namhyung


> 
> -- Steve
> 
> 
> > 
> > [root@X tracing]# cat events/xen/xen_mmu_set_pte/format
> > name: xen_mmu_set_pte
> > ID: 45
> > format:
> >          field:unsigned short common_type;       offset:0;       size:2; 
> > signed:0;
> >          field:unsigned char common_flags;       offset:2;       size:1; 
> > signed:0;
> >          field:unsigned char common_preempt_count;       offset:3; 
> >   size:1; signed:0;
> >          field:int common_pid;   offset:4;       size:4; signed:1;
> > 
> >          field:pte_t * ptep;     offset:8;       size:8; signed:0;
> >          field:pteval_t pteval;  offset:16;      size:8; signed:0;
> > 
> > print fmt: "ptep %p pteval %0*llx (raw %0*llx)", REC->ptep, 
> > (int)sizeof(pteval_t) * 2, (unsigned long 
> > long)pte_val(native_make_pte(REC->pteval)), (int)sizeof(pteval_t) * 2, 
> > (unsigned long long)REC->pteval
> > 
> 

Re: [RFC/PATCH] perf: Add sizeof operator support

[Jon Masters]

Hi Jeremy,

On 06/14/2016 12:38 PM, Jeremy Linton wrote:

> There are a fair number of tracepoints in the kernel making
> use of the sizeof operator. Allow perf to understand some of
> those cases, and report a more informative error message for
> the ones it cannot understand.

What's the status of this patch series? Are you planning to update?

Jon.

Re: [RFC/PATCH] perf: Add sizeof operator support

[Jeremy Linton]

On 04/30/2017 05:06 PM, Jon Masters wrote:
> Hi Jeremy,
>
> On 06/14/2016 12:38 PM, Jeremy Linton wrote:
>
>> There are a fair number of tracepoints in the kernel making
>> use of the sizeof operator. Allow perf to understand some of
>> those cases, and report a more informative error message for
>> the ones it cannot understand.
>
> What's the status of this patch series? Are you planning to update?

Hi,

Well, this series is dead. The question remains how to solve the 
problem. As the maintainers (at least in the case of arm/kvm) aren't 
interested in fixing the individual trace-points, that leaves just 
Steves's suggestion. Which hasn't yet been done. So, I guess that leaves me.