[PATCH 0/2] git pull request for tip/tracing/urgent

[PATCH 0/2] git pull request for tip/tracing/urgent

[Steven Rostedt]

Ingo,

Here's the changes by Lai. The bug scared me enough to investigate
it further, to see if we had other bug reports that this could
have fixed. But I found out that ring_buffer_read_page currently
does not have any users. The only user I have is for my splice work,
which has not been included yet.

I guess Lai is adding one. These are true bug fixes to code that
is in the kernel, even if that code is currently "dead code". But I think
they should still be included in 29, since this will reflect on ftrace
when we have developers starting to use it (such as Lai).

The following patches are in:

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

    branch: tip/tracing/urgent/devel


Lai Jiangshan (2):
      ring_buffer: fix typing mistake
      ring_buffer: fix ring_buffer_read_page()

----
 kernel/trace/ring_buffer.c |   38 ++++++++++++++++++++++----------------
 1 files changed, 22 insertions(+), 16 deletions(-)


-- 

[PATCH 1/2] ring_buffer: fix typing mistake

[Steven Rostedt]

[-- Attachment #1: 0001-ring_buffer-fix-typing-mistake.patch --]
[-- Type: text/plain, Size: 2169 bytes --]

From: Lai Jiangshan <laijs@cn.fujitsu.com>

Impact: Fix bug

I found several very very curious line.
It's so curious that it may be brought by typing mistake.

When (cpu_buffer->reader_page == cpu_buffer->commit_page):

1) We haven't copied it for bpage is changed:
   bpage = cpu_buffer->reader_page->page;
   memcpy(bpage->data, cpu_buffer->reader_page->page->data + read ... )
2) We need update cpu_buffer->reader_page->read, but
   "cpu_buffer->reader_page += read;" is not right.

[
  This bug was a typo. The commit->reader_page is a page pointer
  and not an index into the page. The line should have been
  commit->reader_page->read += read.  The other changes
  by Lai are nice clean ups to the code.  - SDR
]

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ring_buffer.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index bd38c5c..e97acae 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2377,7 +2377,7 @@ void ring_buffer_free_read_page(struct ring_buffer *buffer, void *data)
  * to swap with a page in the ring buffer.
  *
  * for example:
- *	rpage = ring_buffer_alloc_page(buffer);
+ *	rpage = ring_buffer_alloc_read_page(buffer);
  *	if (!rpage)
  *		return error;
  *	ret = ring_buffer_read_page(buffer, &rpage, cpu, 0);
@@ -2432,18 +2432,17 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 	 */
 	if (cpu_buffer->reader_page == cpu_buffer->commit_page) {
 		unsigned int read = cpu_buffer->reader_page->read;
+		unsigned int commit = rb_page_commit(cpu_buffer->reader_page);
 
 		if (full)
 			goto out;
 		/* The writer is still on the reader page, we must copy */
-		bpage = cpu_buffer->reader_page->page;
 		memcpy(bpage->data,
 		       cpu_buffer->reader_page->page->data + read,
-		       local_read(&bpage->commit) - read);
+		       commit - read);
 
 		/* consume what was read */
-		cpu_buffer->reader_page += read;
-
+		cpu_buffer->reader_page->read = commit;
 	} else {
 		/* swap the pages */
 		rb_init_page(bpage);
-- 
1.5.6.5

-- 

[PATCH 2/2] ring_buffer: fix ring_buffer_read_page()

[Steven Rostedt]

[-- Attachment #1: 0002-ring_buffer-fix-ring_buffer_read_page.patch --]
[-- Type: text/plain, Size: 4034 bytes --]

From: Lai Jiangshan <laijs@cn.fujitsu.com>

Impact: change API and init bpage when copy

ring_buffer_read_page()/rb_remove_entries() may be called for
a partially consumed page.

Add a parameter for rb_remove_entries() and make it update
cpu_buffer->entries correctly for partially consumed pages.

ring_buffer_read_page() now returns the offset to the next event.

Init the bpage's time_stamp when return value is 0.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ring_buffer.c |   33 ++++++++++++++++++++-------------
 1 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index e97acae..eaec7e5 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2303,13 +2303,14 @@ int ring_buffer_swap_cpu(struct ring_buffer *buffer_a,
 EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
 
 static void rb_remove_entries(struct ring_buffer_per_cpu *cpu_buffer,
-			      struct buffer_data_page *bpage)
+			      struct buffer_data_page *bpage,
+			      unsigned int offset)
 {
 	struct ring_buffer_event *event;
 	unsigned long head;
 
 	__raw_spin_lock(&cpu_buffer->lock);
-	for (head = 0; head < local_read(&bpage->commit);
+	for (head = offset; head < local_read(&bpage->commit);
 	     head += rb_event_length(event)) {
 
 		event = __rb_data_page_index(bpage, head);
@@ -2381,8 +2382,8 @@ void ring_buffer_free_read_page(struct ring_buffer *buffer, void *data)
  *	if (!rpage)
  *		return error;
  *	ret = ring_buffer_read_page(buffer, &rpage, cpu, 0);
- *	if (ret)
- *		process_page(rpage);
+ *	if (ret >= 0)
+ *		process_page(rpage, ret);
  *
  * When @full is set, the function will not return true unless
  * the writer is off the reader page.
@@ -2393,8 +2394,8 @@ void ring_buffer_free_read_page(struct ring_buffer *buffer, void *data)
  *  responsible for that.
  *
  * Returns:
- *  1 if data has been transferred
- *  0 if no data has been transferred.
+ *  >=0 if data has been transferred, returns the offset of consumed data.
+ *  <0 if no data has been transferred.
  */
 int ring_buffer_read_page(struct ring_buffer *buffer,
 			    void **data_page, int cpu, int full)
@@ -2403,7 +2404,8 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 	struct ring_buffer_event *event;
 	struct buffer_data_page *bpage;
 	unsigned long flags;
-	int ret = 0;
+	unsigned int read;
+	int ret = -1;
 
 	if (!data_page)
 		return 0;
@@ -2425,24 +2427,29 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 	/* check for data */
 	if (!local_read(&cpu_buffer->reader_page->page->commit))
 		goto out;
+
+	read = cpu_buffer->reader_page->read;
 	/*
 	 * If the writer is already off of the read page, then simply
 	 * switch the read page with the given page. Otherwise
 	 * we need to copy the data from the reader to the writer.
 	 */
 	if (cpu_buffer->reader_page == cpu_buffer->commit_page) {
-		unsigned int read = cpu_buffer->reader_page->read;
 		unsigned int commit = rb_page_commit(cpu_buffer->reader_page);
+		struct buffer_data_page *rpage = cpu_buffer->reader_page->page;
 
 		if (full)
 			goto out;
 		/* The writer is still on the reader page, we must copy */
-		memcpy(bpage->data,
-		       cpu_buffer->reader_page->page->data + read,
-		       commit - read);
+		memcpy(bpage->data + read, rpage->data + read, commit - read);
 
 		/* consume what was read */
 		cpu_buffer->reader_page->read = commit;
+
+		/* update bpage */
+		local_set(&bpage->commit, commit);
+		if (!read)
+			bpage->time_stamp = rpage->time_stamp;
 	} else {
 		/* swap the pages */
 		rb_init_page(bpage);
@@ -2451,10 +2458,10 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 		cpu_buffer->reader_page->read = 0;
 		*data_page = bpage;
 	}
-	ret = 1;
+	ret = read;
 
 	/* update the entry counter */
-	rb_remove_entries(cpu_buffer, bpage);
+	rb_remove_entries(cpu_buffer, bpage, read);
  out:
 	spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
 
-- 
1.5.6.5

-- 

Re: [PATCH 0/2] git pull request for tip/tracing/urgent

[Steven Rostedt]

On Mon, 9 Feb 2009, Steven Rostedt wrote:
> Here's the changes by Lai. The bug scared me enough to investigate
> it further, to see if we had other bug reports that this could
> have fixed. But I found out that ring_buffer_read_page currently
> does not have any users. The only user I have is for my splice work,
> which has not been included yet.

Note, this may explain some of the bugs I've seen in my splice code. I 
just assumed it was my splice code that was broken, not the ring buffer 
;-)

-- Steve

Re: [PATCH 0/2] git pull request for tip/tracing/urgent

[Ingo Molnar]

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

> Ingo,
> 
> Here's the changes by Lai. The bug scared me enough to investigate
> it further, to see if we had other bug reports that this could
> have fixed. But I found out that ring_buffer_read_page currently
> does not have any users. The only user I have is for my splice work,
> which has not been included yet.
> 
> I guess Lai is adding one. These are true bug fixes to code that
> is in the kernel, even if that code is currently "dead code". But I think
> they should still be included in 29, since this will reflect on ftrace
> when we have developers starting to use it (such as Lai).
> 
> The following patches are in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: tip/tracing/urgent/devel

hm, the branch does not exist anymore:

  fatal: Couldn't find remote ref tip/tracing/urgent/devel
  fatal: Couldn't find remote ref tracing/urgent/devel
  fatal: Couldn't find remote ref tracing/urgent

In any case, please include this in the devel path not the urgent path.
Urgent patches are for real regressions that occur with the upstream
kernel and affect users. New uses of tracing code should be done on
top of the tracing tree.

	Ingo

Re: [PATCH 0/2] git pull request for tip/tracing/urgent

[Steven Rostedt]

On Tue, 10 Feb 2009, Ingo Molnar wrote:
> 
> hm, the branch does not exist anymore:
> 
>   fatal: Couldn't find remote ref tip/tracing/urgent/devel
>   fatal: Couldn't find remote ref tracing/urgent/devel
>   fatal: Couldn't find remote ref tracing/urgent

Grumble, it failed to push. I'll fix that.

> 
> In any case, please include this in the devel path not the urgent path.
> Urgent patches are for real regressions that occur with the upstream
> kernel and affect users. New uses of tracing code should be done on
> top of the tracing tree.

OK, good to know. I'll move those to the tip/tracing/ftrace branch. I do 
have a bug that needs fixing in urgent though. I'll get that out without 
these changes.

Thanks,

-- Steve