[PATCH 1/5] tracing/events: make the filter files writable

[PATCH 1/5] tracing/events: make the filter files writable

[Frederic Weisbecker]

We need the filter files to be writable, the current
filter file permissions make are only set readable.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/trace_events.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 594d78a..19f61dd 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -706,7 +706,7 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
 
 	system->preds = NULL;
 
-	entry = debugfs_create_file("filter", 0444, system->entry, system,
+	entry = debugfs_create_file("filter", 0644, system->entry, system,
 				    &ftrace_subsystem_filter_fops);
 	if (!entry)
 		pr_warning("Could not create debugfs "
@@ -769,7 +769,7 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
 		}
 	}
 
-	entry = debugfs_create_file("filter", 0444, call->dir, call,
+	entry = debugfs_create_file("filter", 0644, call->dir, call,
 				    &ftrace_event_filter_fops);
 	if (!entry)
 		pr_warning("Could not create debugfs "
-- 
1.6.1

[PATCH 2/5] debugfs: function to know if debugfs is initialized

[Frederic Weisbecker]

With ftrace, some tracers are registered in early initcalls
and attempt to create files on the debugfs filesystem.
Depending on when they are activated, they can try to create their
file at any time. Some checks can be done on the tracing area
but providing a helper to know if debugfs is registered make it
really more easy.

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 fs/debugfs/inode.c      |   16 ++++++++++++++++
 include/linux/debugfs.h |    8 ++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 81ae9ea..0662ba6 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -30,6 +30,7 @@
 
 static struct vfsmount *debugfs_mount;
 static int debugfs_mount_count;
+static bool debugfs_registered;
 
 static struct inode *debugfs_get_inode(struct super_block *sb, int mode, dev_t dev)
 {
@@ -496,6 +497,16 @@ exit:
 }
 EXPORT_SYMBOL_GPL(debugfs_rename);
 
+/**
+ * debugfs_initialized - Tells whether debugfs has been registered
+ */
+bool debugfs_initialized(void)
+{
+	return debugfs_registered;
+}
+EXPORT_SYMBOL_GPL(debugfs_initialized);
+
+
 static struct kobject *debug_kobj;
 
 static int __init debugfs_init(void)
@@ -509,11 +520,16 @@ static int __init debugfs_init(void)
 	retval = register_filesystem(&debug_fs_type);
 	if (retval)
 		kobject_put(debug_kobj);
+	else
+		debugfs_registered = true;
+
 	return retval;
 }
 
 static void __exit debugfs_exit(void)
 {
+	debugfs_registered = false;
+
 	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 	unregister_filesystem(&debug_fs_type);
 	kobject_put(debug_kobj);
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index af0e01d..eb5c2ba 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -71,6 +71,9 @@ struct dentry *debugfs_create_bool(const char *name, mode_t mode,
 struct dentry *debugfs_create_blob(const char *name, mode_t mode,
 				  struct dentry *parent,
 				  struct debugfs_blob_wrapper *blob);
+
+bool debugfs_initialized(void);
+
 #else
 
 #include <linux/err.h>
@@ -183,6 +186,11 @@ static inline struct dentry *debugfs_create_blob(const char *name, mode_t mode,
 	return ERR_PTR(-ENODEV);
 }
 
+static inline bool debugfs_initialized(void)
+{
+	return false;
+}
+
 #endif
 
 #endif
-- 
1.6.1

Re: [PATCH 2/5] debugfs: function to know if debugfs is initialized

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> With ftrace, some tracers are registered in early initcalls
> and attempt to create files on the debugfs filesystem.
> Depending on when they are activated, they can try to create their
> file at any time. Some checks can be done on the tracing area
> but providing a helper to know if debugfs is registered make it
> really more easy.
> 
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  fs/debugfs/inode.c      |   16 ++++++++++++++++
>  include/linux/debugfs.h |    8 ++++++++
>  2 files changed, 24 insertions(+), 0 deletions(-)

Greg, is this patch is fine with you?

If yes, then would you mind if we picked this up into the tracing 
tree - as a subsequent fix-patch relies on it. There's no debugfs 
patches pending that i can see that would conflict with this.

	Ingo

Re: [PATCH 2/5] debugfs: function to know if debugfs is initialized

[Greg KH]

On Mon, Mar 23, 2009 at 09:18:05AM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > With ftrace, some tracers are registered in early initcalls
> > and attempt to create files on the debugfs filesystem.
> > Depending on when they are activated, they can try to create their
> > file at any time. Some checks can be done on the tracing area
> > but providing a helper to know if debugfs is registered make it
> > really more easy.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> >  fs/debugfs/inode.c      |   16 ++++++++++++++++
> >  include/linux/debugfs.h |    8 ++++++++
> >  2 files changed, 24 insertions(+), 0 deletions(-)
> 
> Greg, is this patch is fine with you?

This patch is fine with me, feel free to add:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
to it.

> If yes, then would you mind if we picked this up into the tracing 
> tree - as a subsequent fix-patch relies on it. There's no debugfs 
> patches pending that i can see that would conflict with this.

Yes, that is fine, please take it through the tracing tree.

greg k-h

Re: [PATCH 2/5] debugfs: function to know if debugfs is initialized

[Ingo Molnar]

* Greg KH <gregkh@suse.de> wrote:

> On Mon, Mar 23, 2009 at 09:18:05AM +0100, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > With ftrace, some tracers are registered in early initcalls
> > > and attempt to create files on the debugfs filesystem.
> > > Depending on when they are activated, they can try to create their
> > > file at any time. Some checks can be done on the tracing area
> > > but providing a helper to know if debugfs is registered make it
> > > really more easy.
> > > 
> > > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > > ---
> > >  fs/debugfs/inode.c      |   16 ++++++++++++++++
> > >  include/linux/debugfs.h |    8 ++++++++
> > >  2 files changed, 24 insertions(+), 0 deletions(-)
> > 
> > Greg, is this patch is fine with you?
> 
> This patch is fine with me, feel free to add:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> to it.
> 
> > If yes, then would you mind if we picked this up into the tracing 
> > tree - as a subsequent fix-patch relies on it. There's no debugfs 
> > patches pending that i can see that would conflict with this.
> 
> Yes, that is fine, please take it through the tracing tree.

Thanks Greg!

	Ingo

[tip:tracing/ftrace] debugfs: function to know if debugfs is initialized

[Frederic Weisbecker]

Commit-ID:  c0f92ba99bdeaf35f9c580291b4e1a657c67fbd4
Gitweb:     http://git.kernel.org/tip/c0f92ba99bdeaf35f9c580291b4e1a657c67fbd4
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sun, 22 Mar 2009 23:10:44 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 23 Mar 2009 16:25:46 +0100

debugfs: function to know if debugfs is initialized

Impact: add new debugfs API

With ftrace, some tracers are registered in early initcalls
and attempt to create files on the debugfs filesystem.
Depending on when they are activated, they can try to create their
file at any time. Some checks can be done on the tracing area
but providing a helper to know if debugfs is registered make it
really more easy.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
LKML-Reference: <1237759847-21025-2-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 fs/debugfs/inode.c      |   16 ++++++++++++++++
 include/linux/debugfs.h |    8 ++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 81ae9ea..0662ba6 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -30,6 +30,7 @@
 
 static struct vfsmount *debugfs_mount;
 static int debugfs_mount_count;
+static bool debugfs_registered;
 
 static struct inode *debugfs_get_inode(struct super_block *sb, int mode, dev_t dev)
 {
@@ -496,6 +497,16 @@ exit:
 }
 EXPORT_SYMBOL_GPL(debugfs_rename);
 
+/**
+ * debugfs_initialized - Tells whether debugfs has been registered
+ */
+bool debugfs_initialized(void)
+{
+	return debugfs_registered;
+}
+EXPORT_SYMBOL_GPL(debugfs_initialized);
+
+
 static struct kobject *debug_kobj;
 
 static int __init debugfs_init(void)
@@ -509,11 +520,16 @@ static int __init debugfs_init(void)
 	retval = register_filesystem(&debug_fs_type);
 	if (retval)
 		kobject_put(debug_kobj);
+	else
+		debugfs_registered = true;
+
 	return retval;
 }
 
 static void __exit debugfs_exit(void)
 {
+	debugfs_registered = false;
+
 	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 	unregister_filesystem(&debug_fs_type);
 	kobject_put(debug_kobj);
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index af0e01d..eb5c2ba 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -71,6 +71,9 @@ struct dentry *debugfs_create_bool(const char *name, mode_t mode,
 struct dentry *debugfs_create_blob(const char *name, mode_t mode,
 				  struct dentry *parent,
 				  struct debugfs_blob_wrapper *blob);
+
+bool debugfs_initialized(void);
+
 #else
 
 #include <linux/err.h>
@@ -183,6 +186,11 @@ static inline struct dentry *debugfs_create_blob(const char *name, mode_t mode,
 	return ERR_PTR(-ENODEV);
 }
 
+static inline bool debugfs_initialized(void)
+{
+	return false;
+}
+
 #endif
 
 #endif

[PATCH 3/5] tracing/ftrace: check if debugfs is registered before creating files

[Frederic Weisbecker]

Impact: fix a crash with ftrace={nop,boot} parameter

If the nop or initcall tracers are launched as boot tracers,
they will attempt to create their option directory and files.
But these tracers are registered very early and then assigned
as "boot tracers" very early if asked to.

Since they do this before debugfs has been registered (core initcall),
a crash is triggered.

Another early tracers could also come later. So we fix it by
checking if debugfs is initialized before creating the root
tracing directory.

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/trace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index ace685c..f0e1337 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3513,6 +3513,9 @@ struct dentry *tracing_init_dentry(void)
 	if (d_tracer)
 		return d_tracer;
 
+	if (!debugfs_initialized())
+		return NULL;
+
 	d_tracer = debugfs_create_dir("tracing", NULL);
 
 	if (!d_tracer && !once) {
-- 
1.6.1

Re: [PATCH 3/5] tracing/ftrace: check if debugfs is registered before creating files

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Impact: fix a crash with ftrace={nop,boot} parameter
> 
> If the nop or initcall tracers are launched as boot tracers,
> they will attempt to create their option directory and files.
> But these tracers are registered very early and then assigned
> as "boot tracers" very early if asked to.
> 
> Since they do this before debugfs has been registered (core initcall),
> a crash is triggered.
> 
> Another early tracers could also come later. So we fix it by
> checking if debugfs is initialized before creating the root
> tracing directory.
> 
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  kernel/trace/trace.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

Note - i'll delay this patch pending Greg's Ack.

	Ingo

[tip:tracing/ftrace] tracing/ftrace: check if debugfs is registered before creating files

[Frederic Weisbecker]

Commit-ID:  3e1f60b80cafcb5d7e8d3665b35962fbb8fb9efa
Gitweb:     http://git.kernel.org/tip/3e1f60b80cafcb5d7e8d3665b35962fbb8fb9efa
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sun, 22 Mar 2009 23:10:45 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 23 Mar 2009 16:25:47 +0100

tracing/ftrace: check if debugfs is registered before creating files

Impact: fix a crash with ftrace={nop,boot} parameter

If the nop or initcall tracers are launched as boot tracers,
they will attempt to create their option directory and files.
But these tracers are registered very early and then assigned
as "boot tracers" very early if asked to.

Since they do this before debugfs has been registered (core initcall),
a crash is triggered.

Another early tracers could also come later. So we fix it by
checking if debugfs is initialized before creating the root
tracing directory.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
LKML-Reference: <1237759847-21025-3-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index ace685c..f0e1337 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3513,6 +3513,9 @@ struct dentry *tracing_init_dentry(void)
 	if (d_tracer)
 		return d_tracer;
 
+	if (!debugfs_initialized())
+		return NULL;
+
 	d_tracer = debugfs_create_dir("tracing", NULL);
 
 	if (!d_tracer && !once) {

Re: [PATCH 3/5] tracing/ftrace: check if debugfs is registered before creating files

[Steven Rostedt]

On Sun, 22 Mar 2009, Frederic Weisbecker wrote:

> Impact: fix a crash with ftrace={nop,boot} parameter
> 
> If the nop or initcall tracers are launched as boot tracers,
> they will attempt to create their option directory and files.
> But these tracers are registered very early and then assigned
> as "boot tracers" very early if asked to.
> 
> Since they do this before debugfs has been registered (core initcall),
> a crash is triggered.
> 
> Another early tracers could also come later. So we fix it by
> checking if debugfs is initialized before creating the root
> tracing directory.
> 
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  kernel/trace/trace.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index ace685c..f0e1337 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3513,6 +3513,9 @@ struct dentry *tracing_init_dentry(void)
>  	if (d_tracer)
>  		return d_tracer;
>  
> +	if (!debugfs_initialized())
> +		return NULL;
> +
>  	d_tracer = debugfs_create_dir("tracing", NULL);
>  

Hmm, those tracers should separate out the debugfs code, and put it into 
a fs_initcall(), where debugfs should already initialized.

-- Steve

Re: [PATCH 3/5] tracing/ftrace: check if debugfs is registered before creating files

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Sun, 22 Mar 2009, Frederic Weisbecker wrote:
> 
> > Impact: fix a crash with ftrace={nop,boot} parameter
> > 
> > If the nop or initcall tracers are launched as boot tracers,
> > they will attempt to create their option directory and files.
> > But these tracers are registered very early and then assigned
> > as "boot tracers" very early if asked to.
> > 
> > Since they do this before debugfs has been registered (core initcall),
> > a crash is triggered.
> > 
> > Another early tracers could also come later. So we fix it by
> > checking if debugfs is initialized before creating the root
> > tracing directory.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> >  kernel/trace/trace.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index ace685c..f0e1337 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -3513,6 +3513,9 @@ struct dentry *tracing_init_dentry(void)
> >  	if (d_tracer)
> >  		return d_tracer;
> >  
> > +	if (!debugfs_initialized())
> > +		return NULL;
> > +
> >  	d_tracer = debugfs_create_dir("tracing", NULL);
> >  
> 
> Hmm, those tracers should separate out the debugfs code, and put 
> it into a fs_initcall(), where debugfs should already initialized.

indeed, this seems much cleaner and we'll have the debugfs 
structures initialized as well.

	Ingo

Re: [PATCH 3/5] tracing/ftrace: check if debugfs is registered before creating files

[Frederic Weisbecker]

On Mon, Mar 23, 2009 at 03:18:49PM -0400, Steven Rostedt wrote:
> 
> On Sun, 22 Mar 2009, Frederic Weisbecker wrote:
> 
> > Impact: fix a crash with ftrace={nop,boot} parameter
> > 
> > If the nop or initcall tracers are launched as boot tracers,
> > they will attempt to create their option directory and files.
> > But these tracers are registered very early and then assigned
> > as "boot tracers" very early if asked to.
> > 
> > Since they do this before debugfs has been registered (core initcall),
> > a crash is triggered.
> > 
> > Another early tracers could also come later. So we fix it by
> > checking if debugfs is initialized before creating the root
> > tracing directory.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> >  kernel/trace/trace.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index ace685c..f0e1337 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -3513,6 +3513,9 @@ struct dentry *tracing_init_dentry(void)
> >  	if (d_tracer)
> >  		return d_tracer;
> >  
> > +	if (!debugfs_initialized())
> > +		return NULL;
> > +
> >  	d_tracer = debugfs_create_dir("tracing", NULL);
> >  
> 
> Hmm, those tracers should separate out the debugfs code, and put it into 
> a fs_initcall(), where debugfs should already initialized.
> 
> -- Steve
> 

Indeed, I lacked a bit of imagination here :-)

Thanks!

[PATCH 4/5] tracing/events: don't use wake up for events

[Frederic Weisbecker]

Impact: fix hard lockup with sched switch events

Some ftrace events, such as sched wakeup, can be traced
while the runqueue lock is hold. Since they are using
trace_current_buffer_unlock_commit(), they call wake_up()
which can try to grab the runqueue lock too, resulting in
a deadlock.

Now for all event, we call a new helper:
trace_nowake_buffer_unlock_commit() which do pretty the same than
trace_current_buffer_unlock_commit() except than it doesn't call
trace_wake_up().

Reported-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/trace.c                |   26 +++++++++++++++++++++-----
 kernel/trace/trace.h                |    2 ++
 kernel/trace/trace_events_stage_3.h |    2 +-
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index f0e1337..8dddb42 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -860,15 +860,25 @@ static void ftrace_trace_stack(struct trace_array *tr,
 static void ftrace_trace_userstack(struct trace_array *tr,
 				   unsigned long flags, int pc);
 
-void trace_buffer_unlock_commit(struct trace_array *tr,
-				struct ring_buffer_event *event,
-				unsigned long flags, int pc)
+static inline void __trace_buffer_unlock_commit(struct trace_array *tr,
+					struct ring_buffer_event *event,
+					unsigned long flags, int pc,
+					int wake)
 {
 	ring_buffer_unlock_commit(tr->buffer, event);
 
 	ftrace_trace_stack(tr, flags, 6, pc);
 	ftrace_trace_userstack(tr, flags, pc);
-	trace_wake_up();
+
+	if (wake)
+		trace_wake_up();
+}
+
+void trace_buffer_unlock_commit(struct trace_array *tr,
+					struct ring_buffer_event *event,
+					unsigned long flags, int pc)
+{
+	__trace_buffer_unlock_commit(tr, event, flags, pc, 1);
 }
 
 struct ring_buffer_event *
@@ -882,7 +892,13 @@ trace_current_buffer_lock_reserve(unsigned char type, unsigned long len,
 void trace_current_buffer_unlock_commit(struct ring_buffer_event *event,
 					unsigned long flags, int pc)
 {
-	return trace_buffer_unlock_commit(&global_trace, event, flags, pc);
+	return __trace_buffer_unlock_commit(&global_trace, event, flags, pc, 1);
+}
+
+void trace_nowake_buffer_unlock_commit(struct ring_buffer_event *event,
+					unsigned long flags, int pc)
+{
+	return __trace_buffer_unlock_commit(&global_trace, event, flags, pc, 0);
 }
 
 void
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index f267723..54fd9bc 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -483,6 +483,8 @@ trace_current_buffer_lock_reserve(unsigned char type, unsigned long len,
 				  unsigned long flags, int pc);
 void trace_current_buffer_unlock_commit(struct ring_buffer_event *event,
 					unsigned long flags, int pc);
+void trace_nowake_buffer_unlock_commit(struct ring_buffer_event *event,
+					unsigned long flags, int pc);
 
 struct trace_entry *tracing_get_trace_entry(struct trace_array *tr,
 						struct trace_array_cpu *data);
diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
index ebf215e..9a3bd49 100644
--- a/kernel/trace/trace_events_stage_3.h
+++ b/kernel/trace/trace_events_stage_3.h
@@ -222,7 +222,7 @@ static void ftrace_raw_event_##call(proto)				\
 									\
 	assign;								\
 									\
-	trace_current_buffer_unlock_commit(event, irq_flags, pc);	\
+	trace_nowake_buffer_unlock_commit(event, irq_flags, pc);	\
 									\
 	if (call->preds && !filter_match_preds(call, entry))		\
 		ring_buffer_event_discard(event);			\
-- 
1.6.1

[tip:tracing/filters] tracing/events: don't use wake up for events

[Frederic Weisbecker]

Commit-ID:  07edf7121374609709ef1b0889f6e7b8d6a62ec1
Gitweb:     http://git.kernel.org/tip/07edf7121374609709ef1b0889f6e7b8d6a62ec1
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sun, 22 Mar 2009 23:10:46 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 23 Mar 2009 09:22:14 +0100

tracing/events: don't use wake up for events

Impact: fix hard-lockup with sched switch events

Some ftrace events, such as sched wakeup, can be traced
while the runqueue lock is hold. Since they are using
trace_current_buffer_unlock_commit(), they call wake_up()
which can try to grab the runqueue lock too, resulting in
a deadlock.

Now for all event, we call a new helper:
trace_nowake_buffer_unlock_commit() which do pretty the same than
trace_current_buffer_unlock_commit() except than it doesn't call
trace_wake_up().

Reported-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
LKML-Reference: <1237759847-21025-4-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace.c                |   26 +++++++++++++++++++++-----
 kernel/trace/trace.h                |    2 ++
 kernel/trace/trace_events_stage_3.h |    2 +-
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e6fac0f..6bad128 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -860,15 +860,25 @@ static void ftrace_trace_stack(struct trace_array *tr,
 static void ftrace_trace_userstack(struct trace_array *tr,
 				   unsigned long flags, int pc);
 
-void trace_buffer_unlock_commit(struct trace_array *tr,
-				struct ring_buffer_event *event,
-				unsigned long flags, int pc)
+static inline void __trace_buffer_unlock_commit(struct trace_array *tr,
+					struct ring_buffer_event *event,
+					unsigned long flags, int pc,
+					int wake)
 {
 	ring_buffer_unlock_commit(tr->buffer, event);
 
 	ftrace_trace_stack(tr, flags, 6, pc);
 	ftrace_trace_userstack(tr, flags, pc);
-	trace_wake_up();
+
+	if (wake)
+		trace_wake_up();
+}
+
+void trace_buffer_unlock_commit(struct trace_array *tr,
+					struct ring_buffer_event *event,
+					unsigned long flags, int pc)
+{
+	__trace_buffer_unlock_commit(tr, event, flags, pc, 1);
 }
 
 struct ring_buffer_event *
@@ -882,7 +892,13 @@ trace_current_buffer_lock_reserve(unsigned char type, unsigned long len,
 void trace_current_buffer_unlock_commit(struct ring_buffer_event *event,
 					unsigned long flags, int pc)
 {
-	return trace_buffer_unlock_commit(&global_trace, event, flags, pc);
+	return __trace_buffer_unlock_commit(&global_trace, event, flags, pc, 1);
+}
+
+void trace_nowake_buffer_unlock_commit(struct ring_buffer_event *event,
+					unsigned long flags, int pc)
+{
+	return __trace_buffer_unlock_commit(&global_trace, event, flags, pc, 0);
 }
 
 void
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index f267723..54fd9bc 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -483,6 +483,8 @@ trace_current_buffer_lock_reserve(unsigned char type, unsigned long len,
 				  unsigned long flags, int pc);
 void trace_current_buffer_unlock_commit(struct ring_buffer_event *event,
 					unsigned long flags, int pc);
+void trace_nowake_buffer_unlock_commit(struct ring_buffer_event *event,
+					unsigned long flags, int pc);
 
 struct trace_entry *tracing_get_trace_entry(struct trace_array *tr,
 						struct trace_array_cpu *data);
diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
index ebf215e..9a3bd49 100644
--- a/kernel/trace/trace_events_stage_3.h
+++ b/kernel/trace/trace_events_stage_3.h
@@ -222,7 +222,7 @@ static void ftrace_raw_event_##call(proto)				\
 									\
 	assign;								\
 									\
-	trace_current_buffer_unlock_commit(event, irq_flags, pc);	\
+	trace_nowake_buffer_unlock_commit(event, irq_flags, pc);	\
 									\
 	if (call->preds && !filter_match_preds(call, entry))		\
 		ring_buffer_event_discard(event);			\

Re: [tip:tracing/filters] tracing/events: don't use wake up for events

[Steven Rostedt]

On Mon, 23 Mar 2009, Frederic Weisbecker wrote:
> tracing/events: don't use wake up for events
> 
> Impact: fix hard-lockup with sched switch events
> 
> Some ftrace events, such as sched wakeup, can be traced
> while the runqueue lock is hold. Since they are using
> trace_current_buffer_unlock_commit(), they call wake_up()
> which can try to grab the runqueue lock too, resulting in
> a deadlock.
> 
> Now for all event, we call a new helper:
> trace_nowake_buffer_unlock_commit() which do pretty the same than

Ug, that's an ugly name. It should at least be:

  trace_buffer_unlock_commit_nowake

to be more in line with kernel conventions. That is where the derivitive
has the same name as the original, like preempt_enable_noresched.

-- Steve

> trace_current_buffer_unlock_commit() except than it doesn't call
> trace_wake_up().
> 
> Reported-by: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> LKML-Reference: <1237759847-21025-4-git-send-email-fweisbec@gmail.com>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 

Re: [tip:tracing/filters] tracing/events: don't use wake up for events

[Frederic Weisbecker]

On Mon, Mar 23, 2009 at 03:21:55PM -0400, Steven Rostedt wrote:
> 
> On Mon, 23 Mar 2009, Frederic Weisbecker wrote:
> > tracing/events: don't use wake up for events
> > 
> > Impact: fix hard-lockup with sched switch events
> > 
> > Some ftrace events, such as sched wakeup, can be traced
> > while the runqueue lock is hold. Since they are using
> > trace_current_buffer_unlock_commit(), they call wake_up()
> > which can try to grab the runqueue lock too, resulting in
> > a deadlock.
> > 
> > Now for all event, we call a new helper:
> > trace_nowake_buffer_unlock_commit() which do pretty the same than
> 
> Ug, that's an ugly name. It should at least be:
> 
>   trace_buffer_unlock_commit_nowake
> 
> to be more in line with kernel conventions. That is where the derivitive
> has the same name as the original, like preempt_enable_noresched.
> 
> -- Steve


Indeed.

I scratched my head the evening long to find a name that could mean:
trace_current_buffer_unlock_commit_nowake()  ;-)

I think my headache brought me to this silly trace_nowake....

I will fix the name.
Thanks.

> 
> > trace_current_buffer_unlock_commit() except than it doesn't call
> > trace_wake_up().
> > 
> > Reported-by: Ingo Molnar <mingo@elte.hu>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > LKML-Reference: <1237759847-21025-4-git-send-email-fweisbec@gmail.com>
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > 
> > 

[PATCH 5/5] tracing/ftrace: make nop using polling wait for events on pipe

[Frederic Weisbecker]

Impact: display events when they arrive

Now that the events don't use wake_up() anymore, we need the nop
tracer to poll waiting for events on the pipe. Especially because
nop is useful to look at orphans traces types (traces types that
don't rely on specific tracers) because it doesn't produce traces
itself.

And unlike other tracers that trigger specific traces periodically,
nop triggers no traces by itself that can wake him.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/trace_nop.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace_nop.c b/kernel/trace/trace_nop.c
index 9aa84bd..394f944 100644
--- a/kernel/trace/trace_nop.c
+++ b/kernel/trace/trace_nop.c
@@ -91,6 +91,7 @@ struct tracer nop_trace __read_mostly =
 	.name		= "nop",
 	.init		= nop_trace_init,
 	.reset		= nop_trace_reset,
+	.wait_pipe	= poll_wait_pipe,
 #ifdef CONFIG_FTRACE_SELFTEST
 	.selftest	= trace_selftest_startup_nop,
 #endif
-- 
1.6.1

[tip:tracing/filters] tracing/ftrace: make nop-tracer use polling wait for events on pipe

[Frederic Weisbecker]

Commit-ID:  7e6ea92df3fd7cbe74e7985c6f3e40255c44b201
Gitweb:     http://git.kernel.org/tip/7e6ea92df3fd7cbe74e7985c6f3e40255c44b201
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sun, 22 Mar 2009 23:10:47 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 23 Mar 2009 09:22:15 +0100

tracing/ftrace: make nop-tracer use polling wait for events on pipe

Impact: display events when they arrive

Now that the events don't use wake_up() anymore, we need the nop
tracer to poll waiting for events on the pipe. Especially because
nop is useful to look at orphan traces types (traces types that
don't rely on specific tracers) because it doesn't produce traces
itself.

And unlike other tracers that trigger specific traces periodically,
nop triggers no traces by itself that can wake him.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
LKML-Reference: <1237759847-21025-5-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace_nop.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace_nop.c b/kernel/trace/trace_nop.c
index 9aa84bd..394f944 100644
--- a/kernel/trace/trace_nop.c
+++ b/kernel/trace/trace_nop.c
@@ -91,6 +91,7 @@ struct tracer nop_trace __read_mostly =
 	.name		= "nop",
 	.init		= nop_trace_init,
 	.reset		= nop_trace_reset,
+	.wait_pipe	= poll_wait_pipe,
 #ifdef CONFIG_FTRACE_SELFTEST
 	.selftest	= trace_selftest_startup_nop,
 #endif

Re: [PATCH 1/5] tracing/events: make the filter files writable

[Frederic Weisbecker]

On Sun, Mar 22, 2009 at 11:10:43PM +0100, Frederic Weisbecker wrote:
> We need the filter files to be writable, the current
> filter file permissions make are only set readable.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  kernel/trace/trace_events.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 594d78a..19f61dd 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -706,7 +706,7 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
>  
>  	system->preds = NULL;
>  
> -	entry = debugfs_create_file("filter", 0444, system->entry, system,
> +	entry = debugfs_create_file("filter", 0644, system->entry, system,
>  				    &ftrace_subsystem_filter_fops);
>  	if (!entry)
>  		pr_warning("Could not create debugfs "
> @@ -769,7 +769,7 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
>  		}
>  	}
>  
> -	entry = debugfs_create_file("filter", 0444, call->dir, call,
> +	entry = debugfs_create_file("filter", 0644, call->dir, call,
>  				    &ftrace_event_filter_fops);
>  	if (!entry)
>  		pr_warning("Could not create debugfs "
> -- 
> 1.6.1
> 


Adding Tom Zanussi in Cc.

Re: [PATCH 1/5] tracing/events: make the filter files writable

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> We need the filter files to be writable, the current
> filter file permissions make are only set readable.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  kernel/trace/trace_events.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 594d78a..19f61dd 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -706,7 +706,7 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
>  
>  	system->preds = NULL;
>  
> -	entry = debugfs_create_file("filter", 0444, system->entry, system,
> +	entry = debugfs_create_file("filter", 0644, system->entry, system,
>  				    &ftrace_subsystem_filter_fops);
>  	if (!entry)
>  		pr_warning("Could not create debugfs "
> @@ -769,7 +769,7 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
>  		}
>  	}
>  
> -	entry = debugfs_create_file("filter", 0444, call->dir, call,
> +	entry = debugfs_create_file("filter", 0644, call->dir, call,

Interesting. Yesterday i tried the filter files and they worked - i 
was able to create a 'comm == bash' filter for example. Does debugfs 
ignore permissions perhaps?

	Ingo

Re: [PATCH 1/5] tracing/events: make the filter files writable

[Frederic Weisbecker]

On Mon, Mar 23, 2009 at 09:25:56AM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > We need the filter files to be writable, the current
> > filter file permissions make are only set readable.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> >  kernel/trace/trace_events.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 594d78a..19f61dd 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -706,7 +706,7 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
> >  
> >  	system->preds = NULL;
> >  
> > -	entry = debugfs_create_file("filter", 0444, system->entry, system,
> > +	entry = debugfs_create_file("filter", 0644, system->entry, system,
> >  				    &ftrace_subsystem_filter_fops);
> >  	if (!entry)
> >  		pr_warning("Could not create debugfs "
> > @@ -769,7 +769,7 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
> >  		}
> >  	}
> >  
> > -	entry = debugfs_create_file("filter", 0444, call->dir, call,
> > +	entry = debugfs_create_file("filter", 0644, call->dir, call,
> 
> Interesting. Yesterday i tried the filter files and they worked - i 
> was able to create a 'comm == bash' filter for example. Does debugfs 
> ignore permissions perhaps?
> 
> 	Ingo


That's the second patch I see this week wich proposes a 444 file supposed
to be writable.
Usually it shouldn't even have been testable, so yes I think there is a
little problem.

[tip:tracing/filters] tracing/events: make the filter files writable

[Frederic Weisbecker]

Commit-ID:  9bd7d099ab3f10dd666da399c064999bae427cd9
Gitweb:     http://git.kernel.org/tip/9bd7d099ab3f10dd666da399c064999bae427cd9
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sun, 22 Mar 2009 23:10:43 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 23 Mar 2009 09:22:14 +0100

tracing/events: make the filter files writable

We need the filter files to be writable, the current
filter file permissions are only set readable.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <1237759847-21025-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace_events.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 594d78a..19f61dd 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -706,7 +706,7 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
 
 	system->preds = NULL;
 
-	entry = debugfs_create_file("filter", 0444, system->entry, system,
+	entry = debugfs_create_file("filter", 0644, system->entry, system,
 				    &ftrace_subsystem_filter_fops);
 	if (!entry)
 		pr_warning("Could not create debugfs "
@@ -769,7 +769,7 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
 		}
 	}
 
-	entry = debugfs_create_file("filter", 0444, call->dir, call,
+	entry = debugfs_create_file("filter", 0644, call->dir, call,
 				    &ftrace_event_filter_fops);
 	if (!entry)
 		pr_warning("Could not create debugfs "