[PATCH 0/3] git pull request for tip/tracing/ftrace

[PATCH 0/3] git pull request for tip/tracing/ftrace

[Steven Rostedt]

Ingo,

I pulled in the splice code from Eduard into a branch that is
based off of tip's tracing/ftrace branch.

I kept Eduard's patches as is, and implemented the suggested clean
ups at the end.

My branch names were getting too long so I hacked off the "/devel"
part at the end that I usually have.

-- Steve


The following patches are in:

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

    branch: tip/tracing/ftrace


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

Steven Rostedt (1):
      tracing: clean up splice code

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

-- 

[PATCH 1/3] tracing: splice support for tracing_pipe

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-splice-support-for-tracing_pipe.patch --]
[-- Type: text/plain, Size: 4977 bytes --]

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

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>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 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 5b1e9a9..9e29fdb 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)
 {
@@ -2493,6 +2513,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)
@@ -2656,6 +2791,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 7b0518a..dbff020 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -353,6 +353,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.5.6.5

-- 

[PATCH 2/3] tracing: Move pipe waiting code out of tracing_read_pipe().

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-Move-pipe-waiting-code-out-of-tracing_read_.patch --]
[-- Type: text/plain, Size: 2629 bytes --]

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

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>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 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 9e29fdb..11fde0a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2388,37 +2388,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;
 		}
 
 		/*
@@ -2443,12 +2421,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.
@@ -2465,9 +2442,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.5.6.5

-- 

[PATCH 3/3] tracing: clean up splice code

[Steven Rostedt]

[-- Attachment #1: 0003-tracing-clean-up-splice-code.patch --]
[-- Type: text/plain, Size: 3956 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Ingo Molnar suggested a series of clean ups for the splice code.
This patch implements those suggestions.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace.c |   96 ++++++++++++++++++++++++++++---------------------
 1 files changed, 55 insertions(+), 41 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 11fde0a..d898212 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2537,15 +2537,49 @@ static void tracing_spd_release_pipe(struct splice_pipe_desc *spd,
 }
 
 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,
+	.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 size_t
+tracing_fill_pipe_page(struct page *pages, size_t rem,
+			struct trace_iterator *iter)
+{
+	size_t count;
+	int ret;
+
+	/* 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;
+		}
+	}
+
+	return rem;
+}
+
 static ssize_t tracing_splice_read_pipe(struct file *filp,
 					loff_t *ppos,
 					struct pipe_inode_info *pipe,
@@ -2556,15 +2590,15 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 	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,
+		.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;
+	size_t rem;
 	unsigned int i;
 
 	mutex_lock(&trace_types_lock);
@@ -2573,45 +2607,25 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 		ret = iter->trace->splice_read(iter, filp,
 					       ppos, pipe, len, flags);
 		if (ret)
-			goto out;
+			goto out_err;
 	}
 
 	ret = tracing_wait_pipe(filp);
 	if (ret <= 0)
-		goto out;
+		goto out_err;
 
 	if (!iter->ent && !find_next_entry_inc(iter)) {
 		ret = -EFAULT;
-		goto out;
+		goto out_err;
 	}
 
 	/* Fill as many pages as possible. */
 	for (i = 0, rem = len; i < PIPE_BUFFERS && rem; i++) {
 		pages[i] = alloc_page(GFP_KERNEL);
+		if (!pages[i])
+			break;
 
-		/* 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;
-			}
-		}
+		rem = tracing_fill_pipe_page(pages[i], rem, iter);
 
 		/* Copy the data into the page, so we can start over. */
 		ret = trace_seq_to_buffer(&iter->seq,
@@ -2633,7 +2647,7 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 
 	return splice_to_pipe(pipe, &spd);
 
-out:
+out_err:
 	mutex_unlock(&trace_types_lock);
 
 	return ret;
-- 
1.5.6.5

-- 

Re: [PATCH 2/3] tracing: Move pipe waiting code out of tracing_read_pipe().

[Frederic Weisbecker]

On Mon, Feb 09, 2009 at 12:28:32PM -0500, Steven Rostedt wrote:
> From: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
> 
> 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>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  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 9e29fdb..11fde0a 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2388,37 +2388,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;
>  		}
>  
>  		/*
> @@ -2443,12 +2421,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.
> @@ -2465,9 +2442,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.5.6.5
> 
> -- 

Hmm, ok I will base my changes on top of your tree.
I finally chose to use the common waiting interface, with an
exception concerning sched_switch, function and function_graph.

Even if this can be racy when the tracer is switched, we have a security
inside trace_wake_up().

Re: [PATCH 3/3] tracing: clean up splice code

[Eduard - Gabriel Munteanu]

On Mon, Feb 09, 2009 at 12:28:33PM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> Ingo Molnar suggested a series of clean ups for the splice code.
> This patch implements those suggestions.
> 
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  kernel/trace/trace.c |   96 ++++++++++++++++++++++++++++---------------------
>  1 files changed, 55 insertions(+), 41 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 11fde0a..d898212 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2537,15 +2537,49 @@ static void tracing_spd_release_pipe(struct splice_pipe_desc *spd,
>  }
>  
>  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,
> +	.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 size_t
> +tracing_fill_pipe_page(struct page *pages, size_t rem,
> +			struct trace_iterator *iter)
> +{
> +	size_t count;
> +	int ret;
> +
> +	/* 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;
> +		}
> +	}
> +
> +	return rem;
> +}
> +
>  static ssize_t tracing_splice_read_pipe(struct file *filp,
>  					loff_t *ppos,
>  					struct pipe_inode_info *pipe,
> @@ -2556,15 +2590,15 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
>  	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,
> +		.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;
> +	size_t rem;
>  	unsigned int i;
>  
>  	mutex_lock(&trace_types_lock);
> @@ -2573,45 +2607,25 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
>  		ret = iter->trace->splice_read(iter, filp,
>  					       ppos, pipe, len, flags);
>  		if (ret)
> -			goto out;
> +			goto out_err;
>  	}
>  
>  	ret = tracing_wait_pipe(filp);
>  	if (ret <= 0)
> -		goto out;
> +		goto out_err;
>  
>  	if (!iter->ent && !find_next_entry_inc(iter)) {
>  		ret = -EFAULT;
> -		goto out;
> +		goto out_err;
>  	}
>  
>  	/* Fill as many pages as possible. */
>  	for (i = 0, rem = len; i < PIPE_BUFFERS && rem; i++) {
>  		pages[i] = alloc_page(GFP_KERNEL);
> +		if (!pages[i])
> +			break;

I believe you should decrement 'i' before breaking, since we fill
spd.nr_pages just after the loop. In case the current page couldn't be
allocated, spd.nr_pages will be one too many (that is, 'i').

>  
> -		/* 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;
> -			}
> -		}
> +		rem = tracing_fill_pipe_page(pages[i], rem, iter);
>  
>  		/* Copy the data into the page, so we can start over. */
>  		ret = trace_seq_to_buffer(&iter->seq,
> @@ -2633,7 +2647,7 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
>  
>  	return splice_to_pipe(pipe, &spd);
>  
> -out:
> +out_err:
>  	mutex_unlock(&trace_types_lock);
>  
>  	return ret;
> -- 
> 1.5.6.5
> 
> -- 

Otherwise, it looks okay to me.


	Thanks,
	Eduard

Re: [PATCH 3/3] tracing: clean up splice code

[Pekka Enberg]

Eduard - Gabriel Munteanu wrote:
>> @@ -2573,45 +2607,25 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
>>  		ret = iter->trace->splice_read(iter, filp,
>>  					       ppos, pipe, len, flags);
>>  		if (ret)
>> -			goto out;
>> +			goto out_err;
>>  	}
>>  
>>  	ret = tracing_wait_pipe(filp);
>>  	if (ret <= 0)
>> -		goto out;
>> +		goto out_err;
>>  
>>  	if (!iter->ent && !find_next_entry_inc(iter)) {
>>  		ret = -EFAULT;
>> -		goto out;
>> +		goto out_err;
>>  	}
>>  
>>  	/* Fill as many pages as possible. */
>>  	for (i = 0, rem = len; i < PIPE_BUFFERS && rem; i++) {
>>  		pages[i] = alloc_page(GFP_KERNEL);
>> +		if (!pages[i])
>> +			break;
> 
> I believe you should decrement 'i' before breaking, since we fill
> spd.nr_pages just after the loop. In case the current page couldn't be
> allocated, spd.nr_pages will be one too many (that is, 'i').

But, but, if we fail for i == 1, for example, we want ->nr_pages == 1, no?

			Pekka

Re: [PATCH 3/3] tracing: clean up splice code

[Eduard - Gabriel Munteanu]

On Mon, Feb 09, 2009 at 08:26:20PM +0200, Pekka Enberg wrote:
> Eduard - Gabriel Munteanu wrote:
>>> @@ -2573,45 +2607,25 @@ static ssize_t tracing_splice_read_pipe(struct 
>>> file *filp,
>>>  		ret = iter->trace->splice_read(iter, filp,
>>>  					       ppos, pipe, len, flags);
>>>  		if (ret)
>>> -			goto out;
>>> +			goto out_err;
>>>  	}
>>>   	ret = tracing_wait_pipe(filp);
>>>  	if (ret <= 0)
>>> -		goto out;
>>> +		goto out_err;
>>>   	if (!iter->ent && !find_next_entry_inc(iter)) {
>>>  		ret = -EFAULT;
>>> -		goto out;
>>> +		goto out_err;
>>>  	}
>>>   	/* Fill as many pages as possible. */
>>>  	for (i = 0, rem = len; i < PIPE_BUFFERS && rem; i++) {
>>>  		pages[i] = alloc_page(GFP_KERNEL);
>>> +		if (!pages[i])
>>> +			break;
>> I believe you should decrement 'i' before breaking, since we fill
>> spd.nr_pages just after the loop. In case the current page couldn't be
>> allocated, spd.nr_pages will be one too many (that is, 'i').
>
> But, but, if we fail for i == 1, for example, we want ->nr_pages == 1, no?
>
> 			Pekka

Sure, my bad. I even made the same assumption myself about the outer
'for' loop when I wrote it ('i' would end up last page + 1). The patch is
okay. :-)


	Eduard

Re: [PATCH 3/3] tracing: clean up splice code

[Steven Rostedt]

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

> On Mon, Feb 09, 2009 at 08:26:20PM +0200, Pekka Enberg wrote:
> > Eduard - Gabriel Munteanu wrote:
> >>> @@ -2573,45 +2607,25 @@ static ssize_t tracing_splice_read_pipe(struct 
> >>> file *filp,
> >>>  		ret = iter->trace->splice_read(iter, filp,
> >>>  					       ppos, pipe, len, flags);
> >>>  		if (ret)
> >>> -			goto out;
> >>> +			goto out_err;
> >>>  	}
> >>>   	ret = tracing_wait_pipe(filp);
> >>>  	if (ret <= 0)
> >>> -		goto out;
> >>> +		goto out_err;
> >>>   	if (!iter->ent && !find_next_entry_inc(iter)) {
> >>>  		ret = -EFAULT;
> >>> -		goto out;
> >>> +		goto out_err;
> >>>  	}
> >>>   	/* Fill as many pages as possible. */
> >>>  	for (i = 0, rem = len; i < PIPE_BUFFERS && rem; i++) {
> >>>  		pages[i] = alloc_page(GFP_KERNEL);
> >>> +		if (!pages[i])
> >>> +			break;
> >> I believe you should decrement 'i' before breaking, since we fill
> >> spd.nr_pages just after the loop. In case the current page couldn't be
> >> allocated, spd.nr_pages will be one too many (that is, 'i').
> >
> > But, but, if we fail for i == 1, for example, we want ->nr_pages == 1, no?
> >
> > 			Pekka
> 
> Sure, my bad. I even made the same assumption myself about the outer
> 'for' loop when I wrote it ('i' would end up last page + 1). The patch is
> okay. :-)

Heh, I was just about to say, if this is wrong then so is the if below it 
;-)


               pages[i] = alloc_page(GFP_KERNEL);
                if (!pages[i])
                        break;

                rem = tracing_fill_pipe_page(pages[i], rem, iter);

                /* 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]);

Same 'i'.
                        break;
                }


Thanks Eduard!

-- Steve

Re: [PATCH 0/3] git pull request for tip/tracing/ftrace

[Ingo Molnar]

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

> Ingo,
> 
> I pulled in the splice code from Eduard into a branch that is
> based off of tip's tracing/ftrace branch.
> 
> I kept Eduard's patches as is, and implemented the suggested clean
> ups at the end.
> 
> My branch names were getting too long so I hacked off the "/devel"
> part at the end that I usually have.

> Eduard - Gabriel Munteanu (2):
>       tracing: splice support for tracing_pipe
>       tracing: Move pipe waiting code out of tracing_read_pipe().
> 
> Steven Rostedt (1):
>       tracing: clean up splice code
> 
> ----
>  kernel/trace/trace.c |  219 +++++++++++++++++++++++++++++++++++++++++++-------
>  kernel/trace/trace.h |    6 ++
>  2 files changed, 196 insertions(+), 29 deletions(-)

Pulled into tip:tracing/ftrace, thanks guys!

A small request regarding the commit logs, could you please
in the future make the titles more consistent:

 34cd499: tracing: clean up splice code
 ff98781: tracing: Move pipe waiting code out of tracing_read_pipe().
 3c56819: tracing: splice support for tracing_pipe

the closing period in the second commit is unnecessary, and
capitalization should be consistent as well.

Thanks,

	Ingo

Re: [PATCH 0/3] git pull request for tip/tracing/ftrace

[Steven Rostedt]

On Tue, 10 Feb 2009, Ingo Molnar wrote:
> 
> A small request regarding the commit logs, could you please
> in the future make the titles more consistent:
> 
>  34cd499: tracing: clean up splice code
>  ff98781: tracing: Move pipe waiting code out of tracing_read_pipe().
>  3c56819: tracing: splice support for tracing_pipe
> 
> the closing period in the second commit is unnecessary, and
> capitalization should be consistent as well.

Ah sorry, my fault. I even updated his subject to s/trace/tracing/. I 
missed the period and the Capital :-/

-- Steve