2009-12-09 06:35:44

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [tip:tracing/urgent] tracing: Add pipe_close interface

> Commit-ID: c521efd1700a8c0f7ce26f011f5eaecca17fabfa
> Gitweb: http://git.kernel.org/tip/c521efd1700a8c0f7ce26f011f5eaecca17fabfa
> Author: Steven Rostedt <[email protected]>
> AuthorDate: Mon, 7 Dec 2009 09:06:24 -0500
> Committer: Steven Rostedt <[email protected]>
> CommitDate: Mon, 7 Dec 2009 12:01:35 -0500
>
> tracing: Add pipe_close interface
>
> An ftrace plugin can add a pipe_open interface when the user opens
> trace_pipe. But if the plugin allocates something within the pipe_open
> it can not free it because there exists no pipe_close. The hook to
> the trace file open has a corresponding close. The closing of the
> trace_pipe file should also have a corresponding close.
>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> kernel/trace/trace.c | 4 ++++
> kernel/trace/trace.h | 2 ++
> 2 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 874f289..f804b40 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2898,6 +2898,10 @@ static int tracing_release_pipe(struct inode *inode, struct file *file)
> else
> cpumask_clear_cpu(iter->cpu_file, tracing_reader_cpumask);
>
> +
> + if (iter->trace->pipe_open)
> + iter->trace->pipe_close(iter);
> +

What's happen if pipe_close is NULL? Wny following straightforward check
is wrong?
I mean the above description explain pipe_close is only useful if plugin
allocate something at pipe_open. then allowing NULL seems natural.

if (iter->trace->pipe_close)
iter->trace->pipe_close(iter);

just nit.




2009-12-09 14:08:37

by Steven Rostedt

[permalink] [raw]
Subject: Re: [tip:tracing/urgent] tracing: Add pipe_close interface

On Wed, 2009-12-09 at 15:35 +0900, KOSAKI Motohiro wrote:

> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 874f289..f804b40 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -2898,6 +2898,10 @@ static int tracing_release_pipe(struct inode *inode, struct file *file)
> > else
> > cpumask_clear_cpu(iter->cpu_file, tracing_reader_cpumask);
> >
> > +
> > + if (iter->trace->pipe_open)
> > + iter->trace->pipe_close(iter);
> > +
>
> What's happen if pipe_close is NULL? Wny following straightforward check
> is wrong?
> I mean the above description explain pipe_close is only useful if plugin
> allocate something at pipe_open. then allowing NULL seems natural.
>
> if (iter->trace->pipe_close)
> iter->trace->pipe_close(iter);

Ug, good point (stupid cut & paste should be illegal).

Ingo, I'll fix this up too in the next patch set. Want me to rebase it
or just start with this fix?

-- Steve



2009-12-09 14:16:34

by Ingo Molnar

[permalink] [raw]
Subject: Re: [tip:tracing/urgent] tracing: Add pipe_close interface


* Steven Rostedt <[email protected]> wrote:

> On Wed, 2009-12-09 at 15:35 +0900, KOSAKI Motohiro wrote:
>
> > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > > index 874f289..f804b40 100644
> > > --- a/kernel/trace/trace.c
> > > +++ b/kernel/trace/trace.c
> > > @@ -2898,6 +2898,10 @@ static int tracing_release_pipe(struct inode *inode, struct file *file)
> > > else
> > > cpumask_clear_cpu(iter->cpu_file, tracing_reader_cpumask);
> > >
> > > +
> > > + if (iter->trace->pipe_open)
> > > + iter->trace->pipe_close(iter);
> > > +
> >
> > What's happen if pipe_close is NULL? Wny following straightforward check
> > is wrong?
> > I mean the above description explain pipe_close is only useful if plugin
> > allocate something at pipe_open. then allowing NULL seems natural.
> >
> > if (iter->trace->pipe_close)
> > iter->trace->pipe_close(iter);
>
> Ug, good point (stupid cut & paste should be illegal).
>
> Ingo, I'll fix this up too in the next patch set. Want me to rebase it
> or just start with this fix?

I'd suggest to start with a fix. Thanks,

Ingo

2009-12-10 07:49:17

by Steven Rostedt

[permalink] [raw]
Subject: [tip:tracing/core] tracing: Only call pipe_close if pipe_close is defined

Commit-ID: 29bf4a5e3fed3dde3eb629a0cb1762c1e9217458
Gitweb: http://git.kernel.org/tip/29bf4a5e3fed3dde3eb629a0cb1762c1e9217458
Author: Steven Rostedt <[email protected]>
AuthorDate: Wed, 9 Dec 2009 12:37:43 -0500
Committer: Steven Rostedt <[email protected]>
CommitDate: Wed, 9 Dec 2009 12:47:35 -0500

tracing: Only call pipe_close if pipe_close is defined

This fixes a cut and paste error that had pipe_close get called
if pipe_open was defined (not pipe_close).

Reported-by: Kosaki Motohiro <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
kernel/trace/trace.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index f804b40..dc937e1 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2899,7 +2899,7 @@ static int tracing_release_pipe(struct inode *inode, struct file *file)
cpumask_clear_cpu(iter->cpu_file, tracing_reader_cpumask);


- if (iter->trace->pipe_open)
+ if (iter->trace->pipe_close)
iter->trace->pipe_close(iter);

mutex_unlock(&trace_types_lock);