[PATCH][RFC] tracing: Use class->reg() for all registering of events

[PATCH][RFC] tracing: Use class->reg() for all registering of events

[Steven Rostedt]

I'm pushing this as an RFC first. This probably should be something that
makes it into 2.6.35.

Acks and perhaps a little testing from the perf and kprobe angle?

-- Steve


Steven Rostedt (1):
      tracing: Use class->reg() for all registering of events

----
 include/linux/ftrace_event.h    |    3 ++
 include/trace/ftrace.h          |    2 +
 kernel/trace/trace_event_perf.c |   17 ++----------
 kernel/trace/trace_events.c     |   55 +++++++++++++++++++++++++-------------
 4 files changed, 44 insertions(+), 33 deletions(-)
---------------------------
commit 0ab320dbe5894d7b4eec7d7da3a270abc68e4992
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Tue Jun 8 11:22:06 2010 -0400

    tracing: Use class->reg() for all registering of events
    
    Because kprobes and syscalls need special processing to register
    events, the class->reg() method was created to handle the differences.
    
    But instead of creating a default ->reg for perf and ftrace events,
    the code was scattered with:
    
    	if (class->reg)
    		class->reg();
    	else
    		default_reg();
    
    This is messy and can also lead to bugs.
    
    This patch cleans up this code and creates a default reg() entry for
    the events allowing for the code to directly call the class->reg()
    without the condition.
    
    Reported-by: Peter Zijlstra <peterz@infradead.org>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 3167f2d..a69bd32 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -146,6 +146,9 @@ struct ftrace_event_class {
 	int			(*raw_init)(struct ftrace_event_call *);
 };
 
+extern int ftrace_event_reg(struct ftrace_event_call *event,
+			    enum trace_reg type);
+
 enum {
 	TRACE_EVENT_FL_ENABLED_BIT,
 	TRACE_EVENT_FL_FILTERED_BIT,
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 5a64905..8815e27 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -439,6 +439,7 @@ static inline notrace int ftrace_get_offsets_##call(			\
  *	.fields			= LIST_HEAD_INIT(event_class_##call.fields),
  *	.raw_init		= trace_event_raw_init,
  *	.probe			= ftrace_raw_event_##call,
+ *	.reg			= ftrace_event_reg,
  * };
  *
  * static struct ftrace_event_call __used
@@ -567,6 +568,7 @@ static struct ftrace_event_class __used event_class_##call = {		\
 	.fields			= LIST_HEAD_INIT(event_class_##call.fields),\
 	.raw_init		= trace_event_raw_init,			\
 	.probe			= ftrace_raw_event_##call,		\
+	.reg			= ftrace_event_reg,			\
 	_TRACE_PERF_INIT(call)						\
 };
 
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index e6f6588..de2f170 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -56,13 +56,7 @@ static int perf_trace_event_init(struct ftrace_event_call *tp_event,
 		}
 	}
 
-	if (tp_event->class->reg)
-		ret = tp_event->class->reg(tp_event, TRACE_REG_PERF_REGISTER);
-	else
-		ret = tracepoint_probe_register(tp_event->name,
-						tp_event->class->perf_probe,
-						tp_event);
-
+	ret = tp_event->class->reg(tp_event, TRACE_REG_PERF_REGISTER);
 	if (ret)
 		goto fail;
 
@@ -96,7 +90,7 @@ int perf_trace_init(struct perf_event *p_event)
 	mutex_lock(&event_mutex);
 	list_for_each_entry(tp_event, &ftrace_events, list) {
 		if (tp_event->event.type == event_id &&
-		    tp_event->class && tp_event->class->perf_probe &&
+		    tp_event->class && tp_event->class->reg &&
 		    try_module_get(tp_event->mod)) {
 			ret = perf_trace_event_init(tp_event, p_event);
 			break;
@@ -136,12 +130,7 @@ void perf_trace_destroy(struct perf_event *p_event)
 	if (--tp_event->perf_refcount > 0)
 		goto out;
 
-	if (tp_event->class->reg)
-		tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER);
-	else
-		tracepoint_probe_unregister(tp_event->name,
-					    tp_event->class->perf_probe,
-					    tp_event);
+	tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER);
 
 	/*
 	 * Ensure our callback won't be called anymore. See
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 45a8968..b353639 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -132,6 +132,35 @@ int trace_event_raw_init(struct ftrace_event_call *call)
 }
 EXPORT_SYMBOL_GPL(trace_event_raw_init);
 
+int ftrace_event_reg(struct ftrace_event_call *call, enum trace_reg type)
+{
+	switch (type) {
+	case TRACE_REG_REGISTER:
+		return tracepoint_probe_register(call->name,
+						 call->class->probe,
+						 call);
+	case TRACE_REG_UNREGISTER:
+		tracepoint_probe_unregister(call->name,
+					    call->class->probe,
+					    call);
+		return 0;
+
+#ifdef CONFIG_PERF_EVENTS
+	case TRACE_REG_PERF_REGISTER:
+		return tracepoint_probe_register(call->name,
+						 call->class->perf_probe,
+						 call);
+	case TRACE_REG_PERF_UNREGISTER:
+		tracepoint_probe_unregister(call->name,
+					    call->class->perf_probe,
+					    call);
+		return 0;
+#endif
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ftrace_event_reg);
+
 static int ftrace_event_enable_disable(struct ftrace_event_call *call,
 					int enable)
 {
@@ -142,23 +171,13 @@ static int ftrace_event_enable_disable(struct ftrace_event_call *call,
 		if (call->flags & TRACE_EVENT_FL_ENABLED) {
 			call->flags &= ~TRACE_EVENT_FL_ENABLED;
 			tracing_stop_cmdline_record();
-			if (call->class->reg)
-				call->class->reg(call, TRACE_REG_UNREGISTER);
-			else
-				tracepoint_probe_unregister(call->name,
-							    call->class->probe,
-							    call);
+			call->class->reg(call, TRACE_REG_UNREGISTER);
 		}
 		break;
 	case 1:
 		if (!(call->flags & TRACE_EVENT_FL_ENABLED)) {
 			tracing_start_cmdline_record();
-			if (call->class->reg)
-				ret = call->class->reg(call, TRACE_REG_REGISTER);
-			else
-				ret = tracepoint_probe_register(call->name,
-								call->class->probe,
-								call);
+			ret = call->class->reg(call, TRACE_REG_REGISTER);
 			if (ret) {
 				tracing_stop_cmdline_record();
 				pr_info("event trace: Could not enable event "
@@ -196,8 +215,7 @@ static int __ftrace_set_clr_event(const char *match, const char *sub,
 	mutex_lock(&event_mutex);
 	list_for_each_entry(call, &ftrace_events, list) {
 
-		if (!call->name || !call->class ||
-		    (!call->class->probe && !call->class->reg))
+		if (!call->name || !call->class || !call->class->reg)
 			continue;
 
 		if (match &&
@@ -323,7 +341,7 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
 		 * The ftrace subsystem is for showing formats only.
 		 * They can not be enabled or disabled via the event files.
 		 */
-		if (call->class && (call->class->probe || call->class->reg))
+		if (call->class && call->class->reg)
 			return call;
 	}
 
@@ -476,8 +494,7 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 
 	mutex_lock(&event_mutex);
 	list_for_each_entry(call, &ftrace_events, list) {
-		if (!call->name || !call->class ||
-		    (!call->class->probe && !call->class->reg))
+		if (!call->name || !call->class || !call->class->reg)
 			continue;
 
 		if (system && strcmp(call->class->system, system) != 0)
@@ -1037,12 +1054,12 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 		return -1;
 	}
 
-	if (call->class->probe || call->class->reg)
+	if (call->class->reg)
 		trace_create_file("enable", 0644, call->dir, call,
 				  enable);
 
 #ifdef CONFIG_PERF_EVENTS
-	if (call->event.type && (call->class->perf_probe || call->class->reg))
+	if (call->event.type && call->class->reg)
 		trace_create_file("id", 0444, call->dir, call,
 		 		  id);
 #endif

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Peter Zijlstra]

On Tue, 2010-06-08 at 11:33 -0400, Steven Rostedt wrote:
> I'm pushing this as an RFC first. This probably should be something that
> makes it into 2.6.35.
> 
> Acks and perhaps a little testing from the perf and kprobe angle?

I'll have a look soon, but lets add Srikar to CC, he actually reported
the problem :-)

> Steven Rostedt (1):
>       tracing: Use class->reg() for all registering of events
> 
> ----
>  include/linux/ftrace_event.h    |    3 ++
>  include/trace/ftrace.h          |    2 +
>  kernel/trace/trace_event_perf.c |   17 ++----------
>  kernel/trace/trace_events.c     |   55 +++++++++++++++++++++++++-------------
>  4 files changed, 44 insertions(+), 33 deletions(-)
> ---------------------------
> commit 0ab320dbe5894d7b4eec7d7da3a270abc68e4992
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Tue Jun 8 11:22:06 2010 -0400
> 
>     tracing: Use class->reg() for all registering of events
>     
>     Because kprobes and syscalls need special processing to register
>     events, the class->reg() method was created to handle the differences.
>     
>     But instead of creating a default ->reg for perf and ftrace events,
>     the code was scattered with:
>     
>     	if (class->reg)
>     		class->reg();
>     	else
>     		default_reg();
>     
>     This is messy and can also lead to bugs.
>     
>     This patch cleans up this code and creates a default reg() entry for
>     the events allowing for the code to directly call the class->reg()
>     without the condition.
>     
>     Reported-by: Peter Zijlstra <peterz@infradead.org>
>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 3167f2d..a69bd32 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -146,6 +146,9 @@ struct ftrace_event_class {
>  	int			(*raw_init)(struct ftrace_event_call *);
>  };
>  
> +extern int ftrace_event_reg(struct ftrace_event_call *event,
> +			    enum trace_reg type);
> +
>  enum {
>  	TRACE_EVENT_FL_ENABLED_BIT,
>  	TRACE_EVENT_FL_FILTERED_BIT,
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 5a64905..8815e27 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -439,6 +439,7 @@ static inline notrace int ftrace_get_offsets_##call(			\
>   *	.fields			= LIST_HEAD_INIT(event_class_##call.fields),
>   *	.raw_init		= trace_event_raw_init,
>   *	.probe			= ftrace_raw_event_##call,
> + *	.reg			= ftrace_event_reg,
>   * };
>   *
>   * static struct ftrace_event_call __used
> @@ -567,6 +568,7 @@ static struct ftrace_event_class __used event_class_##call = {		\
>  	.fields			= LIST_HEAD_INIT(event_class_##call.fields),\
>  	.raw_init		= trace_event_raw_init,			\
>  	.probe			= ftrace_raw_event_##call,		\
> +	.reg			= ftrace_event_reg,			\
>  	_TRACE_PERF_INIT(call)						\
>  };
>  
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index e6f6588..de2f170 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -56,13 +56,7 @@ static int perf_trace_event_init(struct ftrace_event_call *tp_event,
>  		}
>  	}
>  
> -	if (tp_event->class->reg)
> -		ret = tp_event->class->reg(tp_event, TRACE_REG_PERF_REGISTER);
> -	else
> -		ret = tracepoint_probe_register(tp_event->name,
> -						tp_event->class->perf_probe,
> -						tp_event);
> -
> +	ret = tp_event->class->reg(tp_event, TRACE_REG_PERF_REGISTER);
>  	if (ret)
>  		goto fail;
>  
> @@ -96,7 +90,7 @@ int perf_trace_init(struct perf_event *p_event)
>  	mutex_lock(&event_mutex);
>  	list_for_each_entry(tp_event, &ftrace_events, list) {
>  		if (tp_event->event.type == event_id &&
> -		    tp_event->class && tp_event->class->perf_probe &&
> +		    tp_event->class && tp_event->class->reg &&
>  		    try_module_get(tp_event->mod)) {
>  			ret = perf_trace_event_init(tp_event, p_event);
>  			break;
> @@ -136,12 +130,7 @@ void perf_trace_destroy(struct perf_event *p_event)
>  	if (--tp_event->perf_refcount > 0)
>  		goto out;
>  
> -	if (tp_event->class->reg)
> -		tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER);
> -	else
> -		tracepoint_probe_unregister(tp_event->name,
> -					    tp_event->class->perf_probe,
> -					    tp_event);
> +	tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER);
>  
>  	/*
>  	 * Ensure our callback won't be called anymore. See
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 45a8968..b353639 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -132,6 +132,35 @@ int trace_event_raw_init(struct ftrace_event_call *call)
>  }
>  EXPORT_SYMBOL_GPL(trace_event_raw_init);
>  
> +int ftrace_event_reg(struct ftrace_event_call *call, enum trace_reg type)
> +{
> +	switch (type) {
> +	case TRACE_REG_REGISTER:
> +		return tracepoint_probe_register(call->name,
> +						 call->class->probe,
> +						 call);
> +	case TRACE_REG_UNREGISTER:
> +		tracepoint_probe_unregister(call->name,
> +					    call->class->probe,
> +					    call);
> +		return 0;
> +
> +#ifdef CONFIG_PERF_EVENTS
> +	case TRACE_REG_PERF_REGISTER:
> +		return tracepoint_probe_register(call->name,
> +						 call->class->perf_probe,
> +						 call);
> +	case TRACE_REG_PERF_UNREGISTER:
> +		tracepoint_probe_unregister(call->name,
> +					    call->class->perf_probe,
> +					    call);
> +		return 0;
> +#endif
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ftrace_event_reg);
> +
>  static int ftrace_event_enable_disable(struct ftrace_event_call *call,
>  					int enable)
>  {
> @@ -142,23 +171,13 @@ static int ftrace_event_enable_disable(struct ftrace_event_call *call,
>  		if (call->flags & TRACE_EVENT_FL_ENABLED) {
>  			call->flags &= ~TRACE_EVENT_FL_ENABLED;
>  			tracing_stop_cmdline_record();
> -			if (call->class->reg)
> -				call->class->reg(call, TRACE_REG_UNREGISTER);
> -			else
> -				tracepoint_probe_unregister(call->name,
> -							    call->class->probe,
> -							    call);
> +			call->class->reg(call, TRACE_REG_UNREGISTER);
>  		}
>  		break;
>  	case 1:
>  		if (!(call->flags & TRACE_EVENT_FL_ENABLED)) {
>  			tracing_start_cmdline_record();
> -			if (call->class->reg)
> -				ret = call->class->reg(call, TRACE_REG_REGISTER);
> -			else
> -				ret = tracepoint_probe_register(call->name,
> -								call->class->probe,
> -								call);
> +			ret = call->class->reg(call, TRACE_REG_REGISTER);
>  			if (ret) {
>  				tracing_stop_cmdline_record();
>  				pr_info("event trace: Could not enable event "
> @@ -196,8 +215,7 @@ static int __ftrace_set_clr_event(const char *match, const char *sub,
>  	mutex_lock(&event_mutex);
>  	list_for_each_entry(call, &ftrace_events, list) {
>  
> -		if (!call->name || !call->class ||
> -		    (!call->class->probe && !call->class->reg))
> +		if (!call->name || !call->class || !call->class->reg)
>  			continue;
>  
>  		if (match &&
> @@ -323,7 +341,7 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
>  		 * The ftrace subsystem is for showing formats only.
>  		 * They can not be enabled or disabled via the event files.
>  		 */
> -		if (call->class && (call->class->probe || call->class->reg))
> +		if (call->class && call->class->reg)
>  			return call;
>  	}
>  
> @@ -476,8 +494,7 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  
>  	mutex_lock(&event_mutex);
>  	list_for_each_entry(call, &ftrace_events, list) {
> -		if (!call->name || !call->class ||
> -		    (!call->class->probe && !call->class->reg))
> +		if (!call->name || !call->class || !call->class->reg)
>  			continue;
>  
>  		if (system && strcmp(call->class->system, system) != 0)
> @@ -1037,12 +1054,12 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
>  		return -1;
>  	}
>  
> -	if (call->class->probe || call->class->reg)
> +	if (call->class->reg)
>  		trace_create_file("enable", 0644, call->dir, call,
>  				  enable);
>  
>  #ifdef CONFIG_PERF_EVENTS
> -	if (call->event.type && (call->class->perf_probe || call->class->reg))
> +	if (call->event.type && call->class->reg)
>  		trace_create_file("id", 0444, call->dir, call,
>  		 		  id);
>  #endif
> 
> 

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Srikar Dronamraju]

> On Tue, 2010-06-08 at 11:33 -0400, Steven Rostedt wrote:
> > I'm pushing this as an RFC first. This probably should be something that
> > makes it into 2.6.35.
> > 
> > Acks and perhaps a little testing from the perf and kprobe angle?
> 
> I'll have a look soon, but lets add Srikar to CC, he actually reported
> the problem :-)
> 
> > Steven Rostedt (1):
> >       tracing: Use class->reg() for all registering of evints
> > 

I tested the patch and it fixed the regression where
perf record -e probe:do_fork -aR sleep 1 would fail. 

Now with this patch, it records the events. However perf record
encounters a floating point exception .. (Peter said he was aware and had a
fix for the floating point exception problem)


However I still see another minor regression (atleast on the tip tree)
This regression was present even before this patch.
The first time I run a perf probe command, it fails, subsequent runs
pass. 

i.e 

411 [srikar@llm69 ]$  sudo perf probe do_fork 
kprobe_events file does not exist - please rebuild kernel with CONFIG_KPROBE_EVENT.
  Error: Failed to add events. (-1)
411 [srikar@llm69 ]$  sudo perf probe do_fork 
Add new event:
  probe:do_fork        (on do_fork)

You can now use it on all perf tools, such as:

        perf record -e probe:do_fork -aR sleep 1

411 [srikar@llm69 ]$ 


--
Thanks and Regards
Srikar

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Peter Zijlstra]

On Tue, 2010-06-08 at 11:33 -0400, Steven Rostedt wrote:
> I'm pushing this as an RFC first. This probably should be something that
> makes it into 2.6.35.
> 
> Acks and perhaps a little testing from the perf and kprobe angle?


Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Steven Rostedt]

On Tue, 2010-06-08 at 23:05 +0530, Srikar Dronamraju wrote:
> > On Tue, 2010-06-08 at 11:33 -0400, Steven Rostedt wrote:

> I tested the patch and it fixed the regression where
> perf record -e probe:do_fork -aR sleep 1 would fail. 
> 

Thanks! Unfortunately the patch does two things, one is to fix this
regression, the other is a clean up. Linus is currently only wanting
fixes now and may not accept the clean up part of this patch. Can you
test the patch below. It only addresses the regression.

Thanks,

-- Steve

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index e6f6588..8a2b73f 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -96,7 +96,9 @@ int perf_trace_init(struct perf_event *p_event)
 	mutex_lock(&event_mutex);
 	list_for_each_entry(tp_event, &ftrace_events, list) {
 		if (tp_event->event.type == event_id &&
-		    tp_event->class && tp_event->class->perf_probe &&
+		    tp_event->class &&
+		    (tp_event->class->perf_probe ||
+		     tp_event->class->reg) &&
 		    try_module_get(tp_event->mod)) {
 			ret = perf_trace_event_init(tp_event, p_event);
 			break;

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Steven Rostedt]

On Tue, 2010-06-08 at 15:10 -0400, Steven Rostedt wrote:
> On Tue, 2010-06-08 at 23:05 +0530, Srikar Dronamraju wrote:
> > > On Tue, 2010-06-08 at 11:33 -0400, Steven Rostedt wrote:
> 
> > I tested the patch and it fixed the regression where
> > perf record -e probe:do_fork -aR sleep 1 would fail. 
> > 
> 
> Thanks! Unfortunately the patch does two things, one is to fix this
> regression, the other is a clean up. Linus is currently only wanting
> fixes now and may not accept the clean up part of this patch. Can you
> test the patch below. It only addresses the regression.
> 
> Thanks,


Oh, and this is against latest Linus's branch.

-- Steve

> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index e6f6588..8a2b73f 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -96,7 +96,9 @@ int perf_trace_init(struct perf_event *p_event)
>  	mutex_lock(&event_mutex);
>  	list_for_each_entry(tp_event, &ftrace_events, list) {
>  		if (tp_event->event.type == event_id &&
> -		    tp_event->class && tp_event->class->perf_probe &&
> +		    tp_event->class &&
> +		    (tp_event->class->perf_probe ||
> +		     tp_event->class->reg) &&
>  		    try_module_get(tp_event->mod)) {
>  			ret = perf_trace_event_init(tp_event, p_event);
>  			break;
> 

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Srikar Dronamraju]

> 
> > I tested the patch and it fixed the regression where
> > perf record -e probe:do_fork -aR sleep 1 would fail. 
> > 
> 
> Thanks! Unfortunately the patch does two things, one is to fix this
> regression, the other is a clean up. Linus is currently only wanting
> fixes now and may not accept the clean up part of this patch. Can you
> test the patch below. It only addresses the regression.


I tested this patch on the mainline(latest git), and perf record is able
to record events. However the other regressions still remain. I am
seeing these on mainline too.

Peter, can you please point me to the fix for the floating point
exception patch? I am not sure if its because of the floating point
exception, but 'perf report' is showing blank output.

--
Thanks and Regards
Srikar

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Peter Zijlstra]

On Wed, 2010-06-09 at 10:35 +0530, Srikar Dronamraju wrote:

> Peter, can you please point me to the fix for the floating point
> exception patch?

Ingo just commited the fix, see tip commit f6ab91add6 (perf: Fix signed
comparison in perf_adjust_period()).

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2010-06-09 08:32:30]:

> On Wed, 2010-06-09 at 10:35 +0530, Srikar Dronamraju wrote:
> 
> > Peter, can you please point me to the fix for the floating point
> > exception patch?
> 
> Ingo just commited the fix, see tip commit f6ab91add6 (perf: Fix signed
> comparison in perf_adjust_period()).

Actually this fix was already checked-in but I still see the problem.

When ran gdb on the core, it showed me that it was dumping in
/usr/lib64/libnewt.so.0.52. in newtScaleSet function.
My newt lib was of version : newt-0.52.2-15

I am now able to workaround the problem by uninstalling newt-devel.

--
Thanks and Regards
Srikar

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Peter Zijlstra]

On Wed, 2010-06-09 at 17:17 +0530, Srikar Dronamraju wrote:
> * Peter Zijlstra <peterz@infradead.org> [2010-06-09 08:32:30]:
> 
> > On Wed, 2010-06-09 at 10:35 +0530, Srikar Dronamraju wrote:
> > 
> > > Peter, can you please point me to the fix for the floating point
> > > exception patch?
> > 
> > Ingo just commited the fix, see tip commit f6ab91add6 (perf: Fix signed
> > comparison in perf_adjust_period()).
> 
> Actually this fix was already checked-in but I still see the problem.
> 
> When ran gdb on the core, it showed me that it was dumping in
> /usr/lib64/libnewt.so.0.52. in newtScaleSet function.
> My newt lib was of version : newt-0.52.2-15
> 
> I am now able to workaround the problem by uninstalling newt-devel.

Ah, ok, then we're talking about two different issues, the patch I
referred to solves a possible /0 in the kernel which triggers easily
with software events because of that unsigned comparison.

I'll have to defer to Arnaldo on the newt issue.

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Arnaldo Carvalho de Melo]

Em Wed, Jun 09, 2010 at 01:53:34PM +0200, Peter Zijlstra escreveu:
> On Wed, 2010-06-09 at 17:17 +0530, Srikar Dronamraju wrote:
> > * Peter Zijlstra <peterz@infradead.org> [2010-06-09 08:32:30]:
> > 
> > > On Wed, 2010-06-09 at 10:35 +0530, Srikar Dronamraju wrote:
> > > 
> > > > Peter, can you please point me to the fix for the floating point
> > > > exception patch?
> > > 
> > > Ingo just commited the fix, see tip commit f6ab91add6 (perf: Fix signed
> > > comparison in perf_adjust_period()).
> > 
> > Actually this fix was already checked-in but I still see the problem.
> > 
> > When ran gdb on the core, it showed me that it was dumping in
> > /usr/lib64/libnewt.so.0.52. in newtScaleSet function.
> > My newt lib was of version : newt-0.52.2-15
> > 
> > I am now able to workaround the problem by uninstalling newt-devel.
> 
> Ah, ok, then we're talking about two different issues, the patch I
> referred to solves a possible /0 in the kernel which triggers easily
> with software events because of that unsigned comparison.
> 
> I'll have to defer to Arnaldo on the newt issue.

I'm trying to reproduce this now.

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Arnaldo Carvalho de Melo]

Em Wed, Jun 09, 2010 at 09:02:24AM -0300, Arnaldo Carvalho de Melo escreveu:
> > On Wed, 2010-06-09 at 17:17 +0530, Srikar Dronamraju wrote:
> > > When ran gdb on the core, it showed me that it was dumping in
> > > /usr/lib64/libnewt.so.0.52. in newtScaleSet function.
> > > My newt lib was of version : newt-0.52.2-15

Can you please provide a backtrace? I think I know what the problem is,
but I'm not managing to reproduce it here.

> > > I am now able to workaround the problem by uninstalling newt-devel.

- Arnaldo

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Arnaldo Carvalho de Melo]

Em Wed, Jun 09, 2010 at 09:23:38AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Jun 09, 2010 at 09:02:24AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > On Wed, 2010-06-09 at 17:17 +0530, Srikar Dronamraju wrote:
> > > > When ran gdb on the core, it showed me that it was dumping in
> > > > /usr/lib64/libnewt.so.0.52. in newtScaleSet function.
> > > > My newt lib was of version : newt-0.52.2-15
> 
> Can you please provide a backtrace? I think I know what the problem is,
> but I'm not managing to reproduce it here.
> 
> > > > I am now able to workaround the problem by uninstalling newt-devel.

If you can reinstall it and then try with the following patch, that is
the minimal for .35, I'd appreciate,

Thanks,

- Arnaldo

diff --git a/tools/perf/util/newt.c b/tools/perf/util/newt.c
index cf182ca..059b772 100644
--- a/tools/perf/util/newt.c
+++ b/tools/perf/util/newt.c
@@ -43,6 +43,12 @@ struct ui_progress *ui_progress__new(const char *title, u64 total)
 
 	if (self != NULL) {
 		int cols;
+		/*
+		 * FIXME: We should have a per UI backend way of showing
+		 * progress, stdio will just show a percentage as NN%, etc.
+		 */
+		if (use_browser <= 0)	
+			return self;
 		newtGetScreenSize(&cols, NULL);
 		cols -= 4;
 		newtCenteredWindow(cols, 1, title);
@@ -67,14 +73,18 @@ out_free_self:
 
 void ui_progress__update(struct ui_progress *self, u64 curr)
 {
+	if (use_browser <= 0)
+		return;
 	newtScaleSet(self->scale, curr);
 	newtRefresh();
 }
 
 void ui_progress__delete(struct ui_progress *self)
 {
-	newtFormDestroy(self->form);
-	newtPopWindow();
+	if (use_browser > 0) {
+		newtFormDestroy(self->form);
+		newtPopWindow();
+	}
 	free(self);
 }
 

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Srikar Dronamraju]

> > 
> > Can you please provide a backtrace? I think I know what the problem is,
> > but I'm not managing to reproduce it here.
> > 
> > > > > I am now able to workaround the problem by uninstalling newt-devel.
> 
> If you can reinstall it and then try with the following patch, that is
> the minimal for .35, I'd appreciate,

Yes, this patch fixes the problem where perf record would fail with
floating point exception when newt-devel was installed.

Thanks Arnaldo.

--
Regards
Srikar

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Arnaldo Carvalho de Melo]

Em Thu, Jun 10, 2010 at 01:57:45PM +0530, Srikar Dronamraju escreveu:
> > > 
> > > Can you please provide a backtrace? I think I know what the problem is,
> > > but I'm not managing to reproduce it here.
> > > 
> > > > > > I am now able to workaround the problem by uninstalling newt-devel.
> > 
> > If you can reinstall it and then try with the following patch, that is
> > the minimal for .35, I'd appreciate,
> 
> Yes, this patch fixes the problem where perf record would fail with
> floating point exception when newt-devel was installed.
> 
> Thanks Arnaldo.

Thanks for testing, I didn't manage to reproduce it here, perhaps the
newt release I have does some checks and notices that newtInit wasn't
called, anyway, I'll push this patch to Ingo right now.

- Arnaldo

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Steven Rostedt]

On Wed, 2010-06-09 at 10:35 +0530, Srikar Dronamraju wrote:
> > 
> > > I tested the patch and it fixed the regression where
> > > perf record -e probe:do_fork -aR sleep 1 would fail. 
> > > 
> > 
> > Thanks! Unfortunately the patch does two things, one is to fix this
> > regression, the other is a clean up. Linus is currently only wanting
> > fixes now and may not accept the clean up part of this patch. Can you
> > test the patch below. It only addresses the regression.
> 
> 
> I tested this patch on the mainline(latest git), and perf record is able
> to record events. However the other regressions still remain. I am
> seeing these on mainline too.

But this means that the problem that I'm responsible for is fixed with
this patch, correct?

If so, I'll package it up and add your "Tested-by:" tag. OK?

Thanks,

-- Steve

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Srikar Dronamraju]

> > 
> > 
> > I tested this patch on the mainline(latest git), and perf record is able
> > to record events. However the other regressions still remain. I am
> > seeing these on mainline too.
> 
> But this means that the problem that I'm responsible for is fixed with
> this patch, correct?

Yes, 

> 
> If so, I'll package it up and add your "Tested-by:" tag. OK?

Okay.


--
Thanks and Regards
Srikar

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Steven Rostedt]

On Tue, 2010-06-08 at 15:10 -0400, Steven Rostedt wrote:
> On Tue, 2010-06-08 at 23:05 +0530, Srikar Dronamraju wrote:
> > > On Tue, 2010-06-08 at 11:33 -0400, Steven Rostedt wrote:
> 
> > I tested the patch and it fixed the regression where
> > perf record -e probe:do_fork -aR sleep 1 would fail. 
> > 
> 
> Thanks! Unfortunately the patch does two things, one is to fix this
> regression, the other is a clean up. Linus is currently only wanting
> fixes now and may not accept the clean up part of this patch. Can you
> test the patch below. It only addresses the regression.
> 


Peter,

Can I get an Acked-by from you on this patch? I know you acked the clean
up version. But this is a minimal fix for .35. We'll add the clean up
version to .36.

Thanks,

-- Steve


> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index e6f6588..8a2b73f 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -96,7 +96,9 @@ int perf_trace_init(struct perf_event *p_event)
>  	mutex_lock(&event_mutex);
>  	list_for_each_entry(tp_event, &ftrace_events, list) {
>  		if (tp_event->event.type == event_id &&
> -		    tp_event->class && tp_event->class->perf_probe &&
> +		    tp_event->class &&
> +		    (tp_event->class->perf_probe ||
> +		     tp_event->class->reg) &&
>  		    try_module_get(tp_event->mod)) {
>  			ret = perf_trace_event_init(tp_event, p_event);
>  			break;
> 

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Peter Zijlstra]

On Thu, 2010-06-10 at 14:41 -0400, Steven Rostedt wrote:
> Can I get an Acked-by from you on this patch? I know you acked the clean
> up version. But this is a minimal fix for .35. We'll add the clean up
> version to .36.

Sure,

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Re: [PATCH][RFC] tracing: Use class->reg() for all registering of events

[Steven Rostedt]

On Thu, 2010-06-10 at 22:36 +0200, Peter Zijlstra wrote:
> On Thu, 2010-06-10 at 14:41 -0400, Steven Rostedt wrote:
> > Can I get an Acked-by from you on this patch? I know you acked the clean
> > up version. But this is a minimal fix for .35. We'll add the clean up
> > version to .36.
> 
> Sure,
> 
> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Thanks!

-- Steve