[PATCH 1/3] tracing: make splice_read work when data is sufficient

[PATCH 1/3] tracing: make splice_read work when data is sufficient

[Lai Jiangshan]

If a cpu ring_buffer has some pages which can be consumed,
but when a piece of the reader page is consumed, splice_read()
on per_cpu/cpu#/trace_pipe_raw will read nothing.

It's a incorrect behavior. A usespace tool which uses
splice_read() can't work when this situation occurs.

This patch changes the meaning of "full". It's not required
the reader page is full with data. It's just required
the reader page is full written/full committed.

So when a piece of data is consumed, the splice_read()
still works.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index da2c59d..f1e1533 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2782,7 +2782,7 @@ rb_update_iter_read_stamp(struct ring_buffer_iter *iter,
 }
 
 static struct buffer_page *
-rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
+rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer, int full)
 {
 	struct buffer_page *reader = NULL;
 	unsigned long flags;
@@ -2799,20 +2799,20 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	 * a case where we will loop three times. There should be no
 	 * reason to loop four times (that I know of).
 	 */
-	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 3)) {
-		reader = NULL;
+	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 3))
+		goto out;
+
+	if (full && cpu_buffer->commit_page == cpu_buffer->reader_page)
 		goto out;
-	}
 
 	reader = cpu_buffer->reader_page;
 
 	/* If there's more to read, return this page */
-	if (cpu_buffer->reader_page->read < rb_page_size(reader))
+	if (reader->read < rb_page_size(reader))
 		goto out;
 
 	/* Never should we have an index greater than the size */
-	if (RB_WARN_ON(cpu_buffer,
-		       cpu_buffer->reader_page->read > rb_page_size(reader)))
+	if (RB_WARN_ON(cpu_buffer, reader->read > rb_page_size(reader)))
 		goto out;
 
 	/* check if we caught up to the tail */
@@ -2823,6 +2823,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	/*
 	 * Reset the reader page to size zero.
 	 */
+	cpu_buffer->reader_page->read = 0;
 	local_set(&cpu_buffer->reader_page->write, 0);
 	local_set(&cpu_buffer->reader_page->entries, 0);
 	local_set(&cpu_buffer->reader_page->page->commit, 0);
@@ -2832,6 +2833,11 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	 * Splice the empty reader page into the list around the head.
 	 */
 	reader = rb_set_head_page(cpu_buffer);
+	if (full && cpu_buffer->commit_page == reader) {
+		reader = NULL;
+		goto out;
+	}
+
 	cpu_buffer->reader_page->list.next = reader->list.next;
 	cpu_buffer->reader_page->list.prev = reader->list.prev;
 
@@ -2891,7 +2897,7 @@ static void rb_advance_reader(struct ring_buffer_per_cpu *cpu_buffer)
 	struct buffer_page *reader;
 	unsigned length;
 
-	reader = rb_get_reader_page(cpu_buffer);
+	reader = rb_get_reader_page(cpu_buffer, 0);
 
 	/* This function should not be called when buffer is empty */
 	if (RB_WARN_ON(cpu_buffer, !reader))
@@ -2973,7 +2979,7 @@ rb_buffer_peek(struct ring_buffer *buffer, int cpu, u64 *ts)
 	if (RB_WARN_ON(cpu_buffer, ++nr_loops > RB_TIMESTAMPS_PER_PAGE))
 		return NULL;
 
-	reader = rb_get_reader_page(cpu_buffer);
+	reader = rb_get_reader_page(cpu_buffer, 0);
 	if (!reader)
 		return NULL;
 
@@ -3642,7 +3648,7 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 
 	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 
-	reader = rb_get_reader_page(cpu_buffer);
+	reader = rb_get_reader_page(cpu_buffer, full);
 	if (!reader)
 		goto out_unlock;
 
@@ -3665,9 +3671,6 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 		unsigned int pos = 0;
 		unsigned int size;
 
-		if (full)
-			goto out_unlock;
-
 		if (len > (commit - read))
 			len = (commit - read);
 

Re: [PATCH 1/3] tracing: make splice_read work when data is sufficient

[Frederic Weisbecker]

On Thu, Aug 27, 2009 at 11:02:27AM +0800, Lai Jiangshan wrote:
> 
> If a cpu ring_buffer has some pages which can be consumed,
> but when a piece of the reader page is consumed, splice_read()
> on per_cpu/cpu#/trace_pipe_raw will read nothing.
> 
> It's a incorrect behavior. A usespace tool which uses
> splice_read() can't work when this situation occurs.
> 
> This patch changes the meaning of "full". It's not required
> the reader page is full with data. It's just required
> the reader page is full written/full committed.
> 
> So when a piece of data is consumed, the splice_read()
> still works.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index da2c59d..f1e1533 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -2782,7 +2782,7 @@ rb_update_iter_read_stamp(struct ring_buffer_iter *iter,
>  }
>  
>  static struct buffer_page *
> -rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
> +rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer, int full)
>  {
>  	struct buffer_page *reader = NULL;
>  	unsigned long flags;
> @@ -2799,20 +2799,20 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	 * a case where we will loop three times. There should be no
>  	 * reason to loop four times (that I know of).
>  	 */
> -	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 3)) {
> -		reader = NULL;
> +	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 3))
> +		goto out;
> +
> +	if (full && cpu_buffer->commit_page == cpu_buffer->reader_page)
>  		goto out;
> -	}
>  
>  	reader = cpu_buffer->reader_page;
>  
>  	/* If there's more to read, return this page */
> -	if (cpu_buffer->reader_page->read < rb_page_size(reader))
> +	if (reader->read < rb_page_size(reader))
>  		goto out;
>  
>  	/* Never should we have an index greater than the size */
> -	if (RB_WARN_ON(cpu_buffer,
> -		       cpu_buffer->reader_page->read > rb_page_size(reader)))
> +	if (RB_WARN_ON(cpu_buffer, reader->read > rb_page_size(reader)))
>  		goto out;
>  
>  	/* check if we caught up to the tail */
> @@ -2823,6 +2823,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	/*
>  	 * Reset the reader page to size zero.
>  	 */
> +	cpu_buffer->reader_page->read = 0;



Wasn't this reset done before?



>  	local_set(&cpu_buffer->reader_page->write, 0);
>  	local_set(&cpu_buffer->reader_page->entries, 0);
>  	local_set(&cpu_buffer->reader_page->page->commit, 0);
> @@ -2832,6 +2833,11 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	 * Splice the empty reader page into the list around the head.
>  	 */
>  	reader = rb_set_head_page(cpu_buffer);
> +	if (full && cpu_buffer->commit_page == reader) {
> +		reader = NULL;
> +		goto out;
> +	}
> +
>  	cpu_buffer->reader_page->list.next = reader->list.next;
>  	cpu_buffer->reader_page->list.prev = reader->list.prev;
>  
> @@ -2891,7 +2897,7 @@ static void rb_advance_reader(struct ring_buffer_per_cpu *cpu_buffer)
>  	struct buffer_page *reader;
>  	unsigned length;
>  
> -	reader = rb_get_reader_page(cpu_buffer);
> +	reader = rb_get_reader_page(cpu_buffer, 0);
>  
>  	/* This function should not be called when buffer is empty */
>  	if (RB_WARN_ON(cpu_buffer, !reader))
> @@ -2973,7 +2979,7 @@ rb_buffer_peek(struct ring_buffer *buffer, int cpu, u64 *ts)
>  	if (RB_WARN_ON(cpu_buffer, ++nr_loops > RB_TIMESTAMPS_PER_PAGE))
>  		return NULL;
>  
> -	reader = rb_get_reader_page(cpu_buffer);
> +	reader = rb_get_reader_page(cpu_buffer, 0);
>  	if (!reader)
>  		return NULL;
>  
> @@ -3642,7 +3648,7 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
>  
>  	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
>  
> -	reader = rb_get_reader_page(cpu_buffer);
> +	reader = rb_get_reader_page(cpu_buffer, full);
>  	if (!reader)
>  		goto out_unlock;
>  
> @@ -3665,9 +3671,6 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
>  		unsigned int pos = 0;
>  		unsigned int size;
>  
> -		if (full)
> -			goto out_unlock;
> -
>  		if (len > (commit - read))
>  			len = (commit - read);
>  


Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>

Re: [PATCH 1/3] tracing: make splice_read work when data is sufficient

[Lai Jiangshan]

Frederic Weisbecker wrote:
>>  	 */
>> +	cpu_buffer->reader_page->read = 0;
> 
> 
> 
> Wasn't this reset done before?
> 
> 

This is not reset. It is still used just before this line.

> 
> 
> Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> 

Ingo, Could you apply this patch and fix the bug?

Re: [PATCH 1/3] tracing: make splice_read work when data is sufficient

[Frederic Weisbecker]

On Tue, Sep 01, 2009 at 08:29:53PM +0800, Lai Jiangshan wrote:
> Frederic Weisbecker wrote:
> >>  	 */
> >> +	cpu_buffer->reader_page->read = 0;
> > 
> > 
> > 
> > Wasn't this reset done before?
> > 
> > 
> 
> This is not reset. It is still used just before this line.



Yeah but after that it's not used anymore because this page
won't anymore be used as a reader page but will be integrated
as a writable ring buffer page.

One day it may be reused as a reader page still, but before
any use, rb_reset_reader_page() will be called to reset this offset.

So I just don't understand why you need to do that.


 
> > 
> > 
> > Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>
> > 
> > 
> 
> Ingo, Could you apply this patch and fix the bug?
> 

Re: [PATCH 1/3] tracing: make splice_read work when data is sufficient

[Lai Jiangshan]

Frederic Weisbecker wrote:
> On Tue, Sep 01, 2009 at 08:29:53PM +0800, Lai Jiangshan wrote:
>> Frederic Weisbecker wrote:
>>>>  	 */
>>>> +	cpu_buffer->reader_page->read = 0;
>>>
>>>
>>> Wasn't this reset done before?
>>>
>>>
>> This is not reset. It is still used just before this line.
> 
> 
> 
> Yeah but after that it's not used anymore because this page

It may be used in next rb_get_reader_page(), see next:

> won't anymore be used as a reader page but will be integrated
> as a writable ring buffer page.
> 
> One day it may be reused as a reader page still, but before
> any use, rb_reset_reader_page() will be called to reset this offset.
> 
> So I just don't understand why you need to do that.
> 
> 

Oh, I know what you mean.

My patch changes the rb_get_reader_page().

rb_get_reader_page(cpu_buffer, 0) is still the same behavior exactly as before.

But rb_get_reader_page(cpu_buffer, 1) returns NULL when
reader page is empty and head page is not full,
in this condition, we do local_set(&cpu_buffer->reader_page->page->commit, 0),
so we need reset cpu_buffer->reader_page->read. Because cpu_buffer->reader_page
will be accessed again in next rb_get_reader_page().

Lai

Re: [PATCH 1/3] tracing: make splice_read work when data is sufficient

[Steven Rostedt]

On Thu, 2009-08-27 at 11:02 +0800, Lai Jiangshan wrote:
> If a cpu ring_buffer has some pages which can be consumed,
> but when a piece of the reader page is consumed, splice_read()
> on per_cpu/cpu#/trace_pipe_raw will read nothing.

I'm not sure exactly what you mean here. Could you explain it a little
better. Are you saying that if I consume one entry from the reader page,
splice read will never return anything?

> 
> It's a incorrect behavior. A usespace tool which uses
> splice_read() can't work when this situation occurs.
> 
> This patch changes the meaning of "full". It's not required
> the reader page is full with data. It's just required
> the reader page is full written/full committed.
> 
> So when a piece of data is consumed, the splice_read()
> still works.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index da2c59d..f1e1533 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -2782,7 +2782,7 @@ rb_update_iter_read_stamp(struct ring_buffer_iter *iter,
>  }
>  
>  static struct buffer_page *
> -rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
> +rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer, int full)
>  {
>  	struct buffer_page *reader = NULL;
>  	unsigned long flags;
> @@ -2799,20 +2799,20 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	 * a case where we will loop three times. There should be no
>  	 * reason to loop four times (that I know of).
>  	 */
> -	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 3)) {
> -		reader = NULL;

You're removing the setting of NULL to reader. What happens if we loop
more than 3 times. reader will not be NULL.

> +	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 3))
> +		goto out;
> +
> +	if (full && cpu_buffer->commit_page == cpu_buffer->reader_page)
>  		goto out;

This may have some funny effects. The first time in, if the reader_page
is on the commit page, we return NULL.

If the reader page is not on the commit page, but the reader page has
been read completely, and we hit the "goto again", but now the reader
page contains the commit_page, this will return the reader page with the
commit page.

> -	}
>  
>  	reader = cpu_buffer->reader_page;
>  
>  	/* If there's more to read, return this page */
> -	if (cpu_buffer->reader_page->read < rb_page_size(reader))
> +	if (reader->read < rb_page_size(reader))
>  		goto out;
>  
>  	/* Never should we have an index greater than the size */
> -	if (RB_WARN_ON(cpu_buffer,
> -		       cpu_buffer->reader_page->read > rb_page_size(reader)))
> +	if (RB_WARN_ON(cpu_buffer, reader->read > rb_page_size(reader)))
>  		goto out;
>  
>  	/* check if we caught up to the tail */
> @@ -2823,6 +2823,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	/*
>  	 * Reset the reader page to size zero.
>  	 */
> +	cpu_buffer->reader_page->read = 0;

rb_reset_reader_page should handle the read case. Here the reader page
is about to go back into the ring buffer. When we get the new reader
page, we reset read to 0. This isn't needed.

>  	local_set(&cpu_buffer->reader_page->write, 0);
>  	local_set(&cpu_buffer->reader_page->entries, 0);
>  	local_set(&cpu_buffer->reader_page->page->commit, 0);
> @@ -2832,6 +2833,11 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	 * Splice the empty reader page into the list around the head.
>  	 */
>  	reader = rb_set_head_page(cpu_buffer);
> +	if (full && cpu_buffer->commit_page == reader) {

I guess this case is to prevent that funny effect I mentioned above,
but...

If we hit this condition, we just changed read to be zero. This can
cause us to reread data that has already been read. That will also kill
the accounting of entries in the buffer, since we will be counting
entries read twice.

> +		reader = NULL;
> +		goto out;
> +	}
> +
>  	cpu_buffer->reader_page->list.next = reader->list.next;
>  	cpu_buffer->reader_page->list.prev = reader->list.prev;
>  
> @@ -2891,7 +2897,7 @@ static void rb_advance_reader(struct ring_buffer_per_cpu *cpu_buffer)
>  	struct buffer_page *reader;
>  	unsigned length;
>  
> -	reader = rb_get_reader_page(cpu_buffer);
> +	reader = rb_get_reader_page(cpu_buffer, 0);
>  
>  	/* This function should not be called when buffer is empty */
>  	if (RB_WARN_ON(cpu_buffer, !reader))
> @@ -2973,7 +2979,7 @@ rb_buffer_peek(struct ring_buffer *buffer, int cpu, u64 *ts)
>  	if (RB_WARN_ON(cpu_buffer, ++nr_loops > RB_TIMESTAMPS_PER_PAGE))
>  		return NULL;
>  
> -	reader = rb_get_reader_page(cpu_buffer);
> +	reader = rb_get_reader_page(cpu_buffer, 0);
>  	if (!reader)
>  		return NULL;
>  
> @@ -3642,7 +3648,7 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
>  
>  	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
>  
> -	reader = rb_get_reader_page(cpu_buffer);
> +	reader = rb_get_reader_page(cpu_buffer, full);

I'm not sure exactly what you are trying to do. rb_get_reader_page will
return a page if there's more to read from.

/me is just confused.

-- Steve

>  	if (!reader)
>  		goto out_unlock;
>  
> @@ -3665,9 +3671,6 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
>  		unsigned int pos = 0;
>  		unsigned int size;
>  
> -		if (full)
> -			goto out_unlock;
> -
>  		if (len > (commit - read))
>  			len = (commit - read);
>  
> 
> 
> 
> 
>