Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1422071 > unrolled thread

[RFC/PATCH] perf: Add sizeof operator support

Back to article view | Back to linux.kernel

Contents

  [RFC/PATCH] perf: Add sizeof operator support Jeremy Linton <jeremy.linton@arm.com> - 2016-06-14 18:40 +0200
    Re: [RFC/PATCH] perf: Add sizeof operator support Steven Rostedt <rostedt@goodmis.org> - 2016-06-17 18:20 +0200
      Re: [RFC/PATCH] perf: Add sizeof operator support Jeremy Linton <jeremy.linton@arm.com> - 2016-06-17 18:40 +0200
        Re: [RFC/PATCH] perf: Add sizeof operator support Steven Rostedt <rostedt@goodmis.org> - 2016-06-17 19:00 +0200
          Re: [RFC/PATCH] perf: Add sizeof operator support Jeremy Linton <jeremy.linton@arm.com> - 2016-06-17 21:00 +0200
            Re: [RFC/PATCH] perf: Add sizeof operator support Steven Rostedt <rostedt@goodmis.org> - 2016-06-17 21:10 +0200
              Re: [RFC/PATCH] perf: Add sizeof operator support Namhyung Kim <namhyung@kernel.org> - 2016-06-20 02:50 +0200

#1422071 — [RFC/PATCH] perf: Add sizeof operator support

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

[toc] | [next] | [standalone]

#1425300

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) {

[toc] | [prev] | [next] | [standalone]

#1425312

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,

[toc] | [prev] | [next] | [standalone]

#1425325

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,

[toc] | [prev] | [next] | [standalone]

#1425384

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

[toc] | [prev] | [next] | [standalone]

#1425388

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
> 

[toc] | [prev] | [next] | [standalone]

#1426091

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web