[PATCH 1/2] ring_buffer: fix typing mistake

[PATCH 1/2] ring_buffer: fix typing mistake

[Lai Jiangshan]

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.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 ring_buffer.c |    9 ++++-----
 1 file 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);

Re: [PATCH 1/2] ring_buffer: fix typing mistake

[Steven Rostedt]

On Mon, 9 Feb 2009, Lai Jiangshan wrote:

> 
> 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.

Ouch! That is a bug.

Ingo, this is an urgent fix. Want me to pull it in and send it off to you?

-- Steve


> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  ring_buffer.c |    9 ++++-----
>  1 file 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);
> 
> 
> 
> 
> 

Re: [PATCH 1/2] ring_buffer: fix typing mistake

[Ingo Molnar]

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

> 
> On Mon, 9 Feb 2009, Lai Jiangshan wrote:
> 
> > 
> > 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.
> 
> Ouch! That is a bug.
> 
> Ingo, this is an urgent fix. Want me to pull it in and send it off to you?

Please do, yes.

	Ingo

[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

--