[RFC PATCH 0/2] splice patches for ftrace

[RFC PATCH 0/2] splice patches for ftrace

[Eduard - Gabriel Munteanu]

Hi,

This implements splice support so we can use it in kmemtrace-user. Please
check it out and tell me what you think.

The "RFC" in the subject reads like "I tested this the best I could,
although kmemtrace-user isn't still ready.". As we speak, I'm working on
making kmemtrace over ftrace and kmemtrace-user work together nicely.


	Cheers,
	Eduard

Eduard - Gabriel Munteanu (2):
  trace: Move pipe waiting code out of tracing_read_pipe().
  trace: splice support for tracing_pipe

 kernel/trace/trace.c |  205 +++++++++++++++++++++++++++++++++++++++++++-------
 kernel/trace/trace.h |    6 ++
 2 files changed, 182 insertions(+), 29 deletions(-)

[RFC PATCH 1/2] trace: Move pipe waiting code out of tracing_read_pipe().

[Eduard - Gabriel Munteanu]

This moves the pipe waiting code from tracing_read_pipe() into
tracing_wait_pipe(), which is useful to implement other fops, like
splice_read.

Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
---
 kernel/trace/trace.c |   69 +++++++++++++++++++++++++++++---------------------
 1 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index ef4dbac..130c4f5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2359,37 +2359,15 @@ tracing_poll_pipe(struct file *filp, poll_table *poll_table)
 	}
 }
 
-/*
- * Consumer reader.
- */
-static ssize_t
-tracing_read_pipe(struct file *filp, char __user *ubuf,
-		  size_t cnt, loff_t *ppos)
+/* Must be called with trace_types_lock mutex held. */
+static int tracing_wait_pipe(struct file *filp)
 {
 	struct trace_iterator *iter = filp->private_data;
-	ssize_t sret;
-
-	/* return any leftover data */
-	sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
-	if (sret != -EBUSY)
-		return sret;
-
-	trace_seq_reset(&iter->seq);
 
-	mutex_lock(&trace_types_lock);
-	if (iter->trace->read) {
-		sret = iter->trace->read(iter, filp, ubuf, cnt, ppos);
-		if (sret)
-			goto out;
-	}
-
-waitagain:
-	sret = 0;
 	while (trace_empty(iter)) {
 
 		if ((filp->f_flags & O_NONBLOCK)) {
-			sret = -EAGAIN;
-			goto out;
+			return -EAGAIN;
 		}
 
 		/*
@@ -2414,12 +2392,11 @@ waitagain:
 		iter->tr->waiter = NULL;
 
 		if (signal_pending(current)) {
-			sret = -EINTR;
-			goto out;
+			return -EINTR;
 		}
 
 		if (iter->trace != current_trace)
-			goto out;
+			return 0;
 
 		/*
 		 * We block until we read something and tracing is disabled.
@@ -2436,9 +2413,43 @@ waitagain:
 		continue;
 	}
 
+	return 1;
+}
+
+/*
+ * Consumer reader.
+ */
+static ssize_t
+tracing_read_pipe(struct file *filp, char __user *ubuf,
+		  size_t cnt, loff_t *ppos)
+{
+	struct trace_iterator *iter = filp->private_data;
+	ssize_t sret;
+
+	/* return any leftover data */
+	sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
+	if (sret != -EBUSY)
+		return sret;
+
+	trace_seq_reset(&iter->seq);
+
+	mutex_lock(&trace_types_lock);
+	if (iter->trace->read) {
+		sret = iter->trace->read(iter, filp, ubuf, cnt, ppos);
+		if (sret)
+			goto out;
+	}
+
+waitagain:
+	sret = tracing_wait_pipe(filp);
+	if (sret <= 0)
+		goto out;
+
 	/* stop when tracing is finished */
-	if (trace_empty(iter))
+	if (trace_empty(iter)) {
+		sret = 0;
 		goto out;
+	}
 
 	if (cnt >= PAGE_SIZE)
 		cnt = PAGE_SIZE - 1;
-- 
1.6.0.6

[RFC PATCH 2/2] trace: splice support for tracing_pipe

[Eduard - Gabriel Munteanu]

Added and implemented tracing_pipe_fops->splice_read(). This allows
userspace programs to get tracing data more efficiently.

Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
---
 kernel/trace/trace.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/trace/trace.h |    6 ++
 2 files changed, 142 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 130c4f5..5b136ed 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -31,6 +31,7 @@
 #include <linux/fs.h>
 #include <linux/kprobes.h>
 #include <linux/writeback.h>
+#include <linux/splice.h>
 
 #include <linux/stacktrace.h>
 #include <linux/ring_buffer.h>
@@ -364,6 +365,25 @@ ssize_t trace_seq_to_user(struct trace_seq *s, char __user *ubuf, size_t cnt)
 	return cnt;
 }
 
+ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
+{
+	int len;
+	void *ret;
+
+	if (s->len <= s->readpos)
+		return -EBUSY;
+
+	len = s->len - s->readpos;
+	if (cnt > len)
+		cnt = len;
+	ret = memcpy(buf, s->buffer + s->readpos, cnt);
+	if (!ret)
+		return -EFAULT;
+
+	s->readpos += len;
+	return cnt;
+}
+
 static void
 trace_print_seq(struct seq_file *m, struct trace_seq *s)
 {
@@ -2495,6 +2515,121 @@ out:
 	return sret;
 }
 
+static void tracing_pipe_buf_release(struct pipe_inode_info *pipe,
+				     struct pipe_buffer *buf)
+{
+	__free_page(buf->page);
+}
+
+static void tracing_spd_release_pipe(struct splice_pipe_desc *spd,
+				     unsigned int idx)
+{
+	__free_page(spd->pages[idx]);
+}
+
+static struct pipe_buf_operations tracing_pipe_buf_ops = {
+	.can_merge = 0,
+	.map = generic_pipe_buf_map,
+	.unmap = generic_pipe_buf_unmap,
+	.confirm = generic_pipe_buf_confirm,
+	.release = tracing_pipe_buf_release,
+	.steal = generic_pipe_buf_steal,
+	.get = generic_pipe_buf_get,
+};
+
+static ssize_t tracing_splice_read_pipe(struct file *filp,
+					loff_t *ppos,
+					struct pipe_inode_info *pipe,
+					size_t len,
+					unsigned int flags)
+{
+	struct page *pages[PIPE_BUFFERS];
+	struct partial_page partial[PIPE_BUFFERS];
+	struct trace_iterator *iter = filp->private_data;
+	struct splice_pipe_desc spd = {
+		.pages = pages,
+		.partial = partial,
+		.nr_pages = 0, /* This gets updated below. */
+		.flags = flags,
+		.ops = &tracing_pipe_buf_ops,
+		.spd_release = tracing_spd_release_pipe,
+	};
+	ssize_t ret;
+	size_t count, rem;
+	unsigned int i;
+
+	mutex_lock(&trace_types_lock);
+
+	if (iter->trace->splice_read) {
+		ret = iter->trace->splice_read(iter, filp,
+					       ppos, pipe, len, flags);
+		if (ret)
+			goto out;
+	}
+
+	ret = tracing_wait_pipe(filp);
+	if (ret <= 0)
+		goto out;
+
+	if (!iter->ent && !find_next_entry_inc(iter)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	/* Fill as many pages as possible. */
+	for (i = 0, rem = len; i < PIPE_BUFFERS && rem; i++) {
+		pages[i] = alloc_page(GFP_KERNEL);
+
+		/* Seq buffer is page-sized, exactly what we need. */
+		for (;;) {
+			count = iter->seq.len;
+			ret = print_trace_line(iter);
+			count = iter->seq.len - count;
+			if (rem < count) {
+				rem = 0;
+				iter->seq.len -= count;
+				break;
+			}
+			if (ret == TRACE_TYPE_PARTIAL_LINE) {
+				iter->seq.len -= count;
+				break;
+			}
+
+			trace_consume(iter);
+			rem -= count;
+			if (!find_next_entry_inc(iter))	{
+				rem = 0;
+				iter->ent = NULL;
+				break;
+			}
+		}
+
+		/* Copy the data into the page, so we can start over. */
+		ret = trace_seq_to_buffer(&iter->seq,
+					  page_address(pages[i]),
+					  iter->seq.len);
+		if (ret < 0) {
+			__free_page(pages[i]);
+			break;
+		}
+		partial[i].offset = 0;
+		partial[i].len = iter->seq.len;
+
+		trace_seq_reset(&iter->seq);
+	}
+
+	mutex_unlock(&trace_types_lock);
+
+	spd.nr_pages = i;
+
+	return splice_to_pipe(pipe, &spd);
+
+out:
+	mutex_unlock(&trace_types_lock);
+
+	return ret;
+}
+
 static ssize_t
 tracing_entries_read(struct file *filp, char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
@@ -2658,6 +2793,7 @@ static struct file_operations tracing_pipe_fops = {
 	.open		= tracing_open_pipe,
 	.poll		= tracing_poll_pipe,
 	.read		= tracing_read_pipe,
+	.splice_read	= tracing_splice_read_pipe,
 	.release	= tracing_release_pipe,
 };
 
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index f2742fb..d58c305 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -352,6 +352,12 @@ struct tracer {
 	ssize_t			(*read)(struct trace_iterator *iter,
 					struct file *filp, char __user *ubuf,
 					size_t cnt, loff_t *ppos);
+	ssize_t			(*splice_read)(struct trace_iterator *iter,
+					       struct file *filp,
+					       loff_t *ppos,
+					       struct pipe_inode_info *pipe,
+					       size_t len,
+					       unsigned int flags);
 #ifdef CONFIG_FTRACE_STARTUP_TEST
 	int			(*selftest)(struct tracer *trace,
 					    struct trace_array *tr);
-- 
1.6.0.6

Re: [RFC PATCH 2/2] trace: splice support for tracing_pipe

[Ingo Molnar]

* Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro> wrote:

> Added and implemented tracing_pipe_fops->splice_read(). This allows
> userspace programs to get tracing data more efficiently.

Cool!

The structure of the splice state machine looks good - havent tested it yet.

A few comments:

> +static struct pipe_buf_operations tracing_pipe_buf_ops = {
> +	.can_merge = 0,
> +	.map = generic_pipe_buf_map,
> +	.unmap = generic_pipe_buf_unmap,
> +	.confirm = generic_pipe_buf_confirm,
> +	.release = tracing_pipe_buf_release,
> +	.steal = generic_pipe_buf_steal,
> +	.get = generic_pipe_buf_get,
> +};

this looks nicer:

 static struct pipe_buf_operations tracing_pipe_buf_ops = {
	.can_merge	= 0,
	.map		= generic_pipe_buf_map,
	.unmap		= generic_pipe_buf_unmap,
	.confirm	= generic_pipe_buf_confirm,
	.release	= tracing_pipe_buf_release,
	.steal		= generic_pipe_buf_steal,
	.get		= generic_pipe_buf_get,
 };

(and this is how we do these things elsewhere in the tracing code)

ditto:

> +	struct splice_pipe_desc spd = {
> +		.pages = pages,
> +		.partial = partial,
> +		.nr_pages = 0, /* This gets updated below. */
> +		.flags = flags,
> +		.ops = &tracing_pipe_buf_ops,
> +		.spd_release = tracing_spd_release_pipe,
> +	};

+       for (i = 0, rem = len; i < PIPE_BUFFERS && rem; i++) {
+               pages[i] = alloc_page(GFP_KERNEL);
+
+               /* Seq buffer is page-sized, exactly what we need. */
+               for (;;) {

Ok, that's a minor bug: should check for NULL alloc_page() return.

> +out:
> +	mutex_unlock(&trace_types_lock);
> +
> +	return ret;

please name this label as 'out_err' - which it really is. We generally use the 
'out:' label for error-less exits.

Btw., this is how we do structure initializations:

>  	.open		= tracing_open_pipe,
>  	.poll		= tracing_poll_pipe,
>  	.read		= tracing_read_pipe,
> +	.splice_read	= tracing_splice_read_pipe,
>  	.release	= tracing_release_pipe,
>  };

One more thing, i'd suggest to split this loop (which is an iterator in a larger 
loop):

+               /* Seq buffer is page-sized, exactly what we need. */
+               for (;;) {
+                       count = iter->seq.len;
+                       ret = print_trace_line(iter);
[...]
+		}

Out into a separate helper inline function. This really is a "fill in pipe page" 
logic and having it separate makes it easier to review the splice-loop code pattern 
itself.

Thanks,

	Ingo

Re: [RFC PATCH 0/2] splice patches for ftrace

[Steven Rostedt]

On Mon, 9 Feb 2009, Eduard - Gabriel Munteanu wrote:

> Hi,
> 
> This implements splice support so we can use it in kmemtrace-user. Please
> check it out and tell me what you think.
> 
> The "RFC" in the subject reads like "I tested this the best I could,
> although kmemtrace-user isn't still ready.". As we speak, I'm working on
> making kmemtrace over ftrace and kmemtrace-user work together nicely.

Hi Eduard!

Thanks, I'll give it a test. I've been playing on my spare time with
a ring buffer splice interface. But I've not had the time to get it 
working fully. My version would be a binary only interface, but I really 
like the text interface you have.

You can look at my old changes at:

 git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

  The branch tip/splice-broken

As the name suggests, it is still broken ;-)

-- Steve

Re: [RFC PATCH 0/2] splice patches for ftrace

[Ingo Molnar]

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

> 
> On Mon, 9 Feb 2009, Eduard - Gabriel Munteanu wrote:
> 
> > Hi,
> > 
> > This implements splice support so we can use it in kmemtrace-user. Please
> > check it out and tell me what you think.
> > 
> > The "RFC" in the subject reads like "I tested this the best I could,
> > although kmemtrace-user isn't still ready.". As we speak, I'm working on
> > making kmemtrace over ftrace and kmemtrace-user work together nicely.
> 
> Hi Eduard!
> 
> Thanks, I'll give it a test. I've been playing on my spare time with
> a ring buffer splice interface. But I've not had the time to get it 
> working fully. My version would be a binary only interface, but I really 
> like the text interface you have.
> 
> You can look at my old changes at:
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>   The branch tip/splice-broken
> 
> As the name suggests, it is still broken ;-)

I'd suggest we pick up Eduard's, with the fixes i suggested - because
they clearly work for his kmemtrace utility!

Then pick up whatever is left in your branch.

	Ingo

Re: [RFC PATCH 0/2] splice patches for ftrace

[Pekka Enberg]

On Mon, 2009-02-09 at 08:15 +0200, Eduard - Gabriel Munteanu wrote:
> This implements splice support so we can use it in kmemtrace-user. Please
> check it out and tell me what you think.

I am not a splice or ftrace expert but modulo the comments from Ingo:

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

Re: [RFC PATCH 2/2] trace: splice support for tracing_pipe

[Steven Rostedt]

On Mon, 9 Feb 2009, Eduard - Gabriel Munteanu wrote:
> +
> +static ssize_t tracing_splice_read_pipe(struct file *filp,
> +					loff_t *ppos,
> +					struct pipe_inode_info *pipe,
> +					size_t len,
> +					unsigned int flags)
> +{
> +	struct page *pages[PIPE_BUFFERS];
> +	struct partial_page partial[PIPE_BUFFERS];
> +	struct trace_iterator *iter = filp->private_data;
> +	struct splice_pipe_desc spd = {
> +		.pages = pages,
> +		.partial = partial,
> +		.nr_pages = 0, /* This gets updated below. */
> +		.flags = flags,
> +		.ops = &tracing_pipe_buf_ops,
> +		.spd_release = tracing_spd_release_pipe,
> +	};

Note, this is getting a little stack heavy. It is still in bounds,
but I get very nervous when I see structure arrays on the stack.
If either the structure or the array grows large, we can be in trouble.

-- Steve

Re: [RFC PATCH 2/2] trace: splice support for tracing_pipe

[Ingo Molnar]

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

> On Mon, 9 Feb 2009, Eduard - Gabriel Munteanu wrote:
> > +
> > +static ssize_t tracing_splice_read_pipe(struct file *filp,
> > +					loff_t *ppos,
> > +					struct pipe_inode_info *pipe,
> > +					size_t len,
> > +					unsigned int flags)
> > +{
> > +	struct page *pages[PIPE_BUFFERS];
> > +	struct partial_page partial[PIPE_BUFFERS];
> > +	struct trace_iterator *iter = filp->private_data;
> > +	struct splice_pipe_desc spd = {
> > +		.pages = pages,
> > +		.partial = partial,
> > +		.nr_pages = 0, /* This gets updated below. */
> > +		.flags = flags,
> > +		.ops = &tracing_pipe_buf_ops,
> > +		.spd_release = tracing_spd_release_pipe,
> > +	};
> 
> Note, this is getting a little stack heavy. It is still in bounds,
> but I get very nervous when I see structure arrays on the stack.
> If either the structure or the array grows large, we can be in trouble.

Good point - but note that this is how splice support is implemented in
a number of other files - so if PIPE_BUFFERS or partial_page grows in
size, those places will fail too.

	Ingo

Re: [RFC PATCH 2/2] trace: splice support for tracing_pipe

[Frederic Weisbecker]

On Wed, Feb 11, 2009 at 01:35:26PM +0100, Ingo Molnar wrote:
> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Mon, 9 Feb 2009, Eduard - Gabriel Munteanu wrote:
> > > +
> > > +static ssize_t tracing_splice_read_pipe(struct file *filp,
> > > +					loff_t *ppos,
> > > +					struct pipe_inode_info *pipe,
> > > +					size_t len,
> > > +					unsigned int flags)
> > > +{
> > > +	struct page *pages[PIPE_BUFFERS];
> > > +	struct partial_page partial[PIPE_BUFFERS];
> > > +	struct trace_iterator *iter = filp->private_data;
> > > +	struct splice_pipe_desc spd = {
> > > +		.pages = pages,
> > > +		.partial = partial,
> > > +		.nr_pages = 0, /* This gets updated below. */
> > > +		.flags = flags,
> > > +		.ops = &tracing_pipe_buf_ops,
> > > +		.spd_release = tracing_spd_release_pipe,
> > > +	};
> > 
> > Note, this is getting a little stack heavy. It is still in bounds,
> > but I get very nervous when I see structure arrays on the stack.
> > If either the structure or the array grows large, we can be in trouble.
> 
> Good point - but note that this is how splice support is implemented in
> a number of other files - so if PIPE_BUFFERS or partial_page grows in
> size, those places will fail too.
> 
> 	Ingo

I first thought they can be declared as static, since the read_pipe functions
are not supposed to be reentrant, only one reader is allowed inside the pipe
(serialized with a global mutex).
But the mutex is released while waiting, and still, I guess it should be
reentrant one day if we plan to let the user having one trace_pipe per
cpu trace.

Would a dynamic allocation be too much overhead for that?