linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC/PATCH] perf: Add sizeof operator support
@ 2016-06-14 16:38 Jeremy Linton
  2016-06-17 16:17 ` Steven Rostedt
  2017-04-30 22:06 ` Jon Masters
  0 siblings, 2 replies; 9+ messages in thread
From: Jeremy Linton @ 2016-06-14 16:38 UTC (permalink / raw)
  To: linux-kernel
  Cc: acme, rostedt, namhyung, kapileshwar.singh, scottwood, hekuang

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC/PATCH] perf: Add sizeof operator support
  2016-06-14 16:38 [RFC/PATCH] perf: Add sizeof operator support Jeremy Linton
@ 2016-06-17 16:17 ` Steven Rostedt
  2016-06-17 16:32   ` Jeremy Linton
  2017-04-30 22:06 ` Jon Masters
  1 sibling, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2016-06-17 16:17 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: linux-kernel, acme, namhyung, kapileshwar.singh, scottwood, hekuang

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC/PATCH] perf: Add sizeof operator support
  2016-06-17 16:17 ` Steven Rostedt
@ 2016-06-17 16:32   ` Jeremy Linton
  2016-06-17 16:50     ` Steven Rostedt
  0 siblings, 1 reply; 9+ messages in thread
From: Jeremy Linton @ 2016-06-17 16:32 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, acme, namhyung, kapileshwar.singh, scottwood, hekuang

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,

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC/PATCH] perf: Add sizeof operator support
  2016-06-17 16:32   ` Jeremy Linton
@ 2016-06-17 16:50     ` Steven Rostedt
  2016-06-17 18:57       ` Jeremy Linton
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2016-06-17 16:50 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: linux-kernel, acme, namhyung, kapileshwar.singh, scottwood, hekuang

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,

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC/PATCH] perf: Add sizeof operator support
  2016-06-17 16:50     ` Steven Rostedt
@ 2016-06-17 18:57       ` Jeremy Linton
  2016-06-17 19:08         ` Steven Rostedt
  0 siblings, 1 reply; 9+ messages in thread
From: Jeremy Linton @ 2016-06-17 18:57 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, acme, namhyung, kapileshwar.singh, scottwood, hekuang

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC/PATCH] perf: Add sizeof operator support
  2016-06-17 18:57       ` Jeremy Linton
@ 2016-06-17 19:08         ` Steven Rostedt
  2016-06-20  0:29           ` Namhyung Kim
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2016-06-17 19:08 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: linux-kernel, acme, namhyung, kapileshwar.singh, scottwood, hekuang

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
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC/PATCH] perf: Add sizeof operator support
  2016-06-17 19:08         ` Steven Rostedt
@ 2016-06-20  0:29           ` Namhyung Kim
  0 siblings, 0 replies; 9+ messages in thread
From: Namhyung Kim @ 2016-06-20  0:29 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Jeremy Linton, linux-kernel, acme, kapileshwar.singh, scottwood, hekuang

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC/PATCH] perf: Add sizeof operator support
  2016-06-14 16:38 [RFC/PATCH] perf: Add sizeof operator support Jeremy Linton
  2016-06-17 16:17 ` Steven Rostedt
@ 2017-04-30 22:06 ` Jon Masters
  2017-05-02 11:47   ` Jeremy Linton
  1 sibling, 1 reply; 9+ messages in thread
From: Jon Masters @ 2017-04-30 22:06 UTC (permalink / raw)
  To: Jeremy Linton, linux-kernel
  Cc: acme, rostedt, namhyung, kapileshwar.singh, scottwood, hekuang

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC/PATCH] perf: Add sizeof operator support
  2017-04-30 22:06 ` Jon Masters
@ 2017-05-02 11:47   ` Jeremy Linton
  0 siblings, 0 replies; 9+ messages in thread
From: Jeremy Linton @ 2017-05-02 11:47 UTC (permalink / raw)
  To: Jon Masters, linux-kernel
  Cc: acme, rostedt, namhyung, kapileshwar.singh, scottwood, hekuang

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-05-02 11:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-14 16:38 [RFC/PATCH] perf: Add sizeof operator support Jeremy Linton
2016-06-17 16:17 ` Steven Rostedt
2016-06-17 16:32   ` Jeremy Linton
2016-06-17 16:50     ` Steven Rostedt
2016-06-17 18:57       ` Jeremy Linton
2016-06-17 19:08         ` Steven Rostedt
2016-06-20  0:29           ` Namhyung Kim
2017-04-30 22:06 ` Jon Masters
2017-05-02 11:47   ` Jeremy Linton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).