tracing: make splice_read work when data is sufficient
Subsystems (2)
PATCH 1/3 tracing: make splice_read work when data is sufficient
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.
---
kernel/trace/ring_buffer.c +16 -13
@@ -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);
Quoted 58 lines · Lai Jiangshan
If a cpu
ring_bufferhas 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 <[email protected]>---
kernel/trace/ring_buffer.c +7
-7@@ -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 */
kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2826
Quoted 4 lines
kernel/trace/ring_buffer.c +1
-0@@ -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?
Quoted 43 lines
kernel/trace/ring_buffer.c +8
-3local_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;
kernel/trace/ring_buffer.c int ring_buffer_read_page(struct ring_buffer *buffer, · line 3676
Quoted 9 lines
kernel/trace/ring_buffer.c +0
-3@@ -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);
Quoted 8 lines · Frederic Weisbecker
kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2826
Quoted 2 lines
kernel/trace/ring_buffer.c +1
-0*/ + cpu_buffer->reader_page->read =0 ;Wasn't this reset done before?
This is not reset. It is still used just before this line.
Quoted 5 lines
Reviewed-by Frederic Weisbecker <[email protected]>
Ingo, Could you apply this patch and fix the bug?
Quoted 11 lines · Lai Jiangshan
Quoted 8 lines · Frederic Weisbecker
kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2826
Quoted 2 lines
kernel/trace/ring_buffer.c +1
-0*/ + 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.
Quoted 8 lines
Quoted 5 lines
Reviewed-by Frederic Weisbecker <[email protected]>Ingo, Could you apply this patch and fix the bug?
Quoted 9 lines · Frederic Weisbecker
Quoted 8 lines · Lai Jiangshan
Quoted 7 lines · Frederic Weisbecker
kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2826
Quoted 2 lines
kernel/trace/ring_buffer.c +1
-0*/ + cpu_buffer->reader_page->read =0 ;Wasn't this reset done before?
Quoted 5 lines
Quoted 1 line
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:
Quoted 9 lines
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
Steven Rostedt
maintainerQuoted 3 lines · Lai Jiangshan
If a cpu
ring_bufferhas 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?
Quoted 27 lines
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 <[email protected]>---
kernel/trace/ring_buffer.c +1
-1@@ -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;
kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2803
Quoted 5 lines
kernel/trace/ring_buffer.c +0
-2@@ -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.
kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2806
Quoted 5 lines
kernel/trace/ring_buffer.c +4
-0+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.
Quoted 17 lines
kernel/trace/ring_buffer.c +2
-4- } 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 */
kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2826
Quoted 4 lines
kernel/trace/ring_buffer.c +1
-0@@ -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.
kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2836
Quoted 8 lines
kernel/trace/ring_buffer.c +1
-0local_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.
Quoted 26 lines
kernel/trace/ring_buffer.c +6
-2+ 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 ;
kernel/trace/ring_buffer.c int ring_buffer_read_page(struct ring_buffer *buffer, · line 3651
Quoted 5 lines
kernel/trace/ring_buffer.c +1
-1@@ -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
Quoted 18 lines
kernel/trace/ring_buffer.c +0
-3if (!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);