| Message ID | 20221021123013.55fb6055@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 31029a8b2c7e656a0289194ef16415050ae4c4ac |
| Headers | show |
| Series | ring-buffer: Include dropped pages in counting dirty patches | expand |
On Fri, 21 Oct 2022 12:30:13 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > The function ring_buffer_nr_dirty_pages() was created to find out how many > pages are filled in the ring buffer. There's two running counters. One is > incremented whenever a new page is touched (pages_touched) and the other > is whenever a page is read (pages_read). The dirty count is the number > touched minus the number read. This is used to determine if a blocked task > should be woken up if the percentage of the ring buffer it is waiting for > is hit. > > The problem is that it does not take into account dropped pages (when the > new writes overwrite pages that were not read). And then the dirty pages > will always be greater than the percentage. > > Add a new counter to keep track of lost pages, and include that in the > accounting of dirty pages so that it is actually accurate. > > Fixes: 2c2b0a78b3739 ("ring-buffer: Add percentage of ring buffer full to wake up reader") > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> This looks good to me. But I have just a nitpick below. Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > kernel/trace/ring_buffer.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index b60047de897e..f712006f6dd3 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -519,6 +519,7 @@ struct ring_buffer_per_cpu { > local_t committing; > local_t commits; > local_t pages_touched; > + local_t pages_lost; > local_t pages_read; > long last_pages_touch; > size_t shortest_full; > @@ -894,10 +895,18 @@ size_t ring_buffer_nr_pages(struct trace_buffer *buffer, int cpu) > size_t ring_buffer_nr_dirty_pages(struct trace_buffer *buffer, int cpu) > { > size_t read; > + size_t lost; > size_t cnt; > > read = local_read(&buffer->buffers[cpu]->pages_read); > + lost = local_read(&buffer->buffers[cpu]->pages_lost); > cnt = local_read(&buffer->buffers[cpu]->pages_touched); > + > + if (WARN_ON_ONCE(cnt < lost)) > + return 0; > + > + cnt -= lost; > + > /* The reader can read an empty page, but not more than that */ > if (cnt < read) { > WARN_ON_ONCE(read > cnt + 1); > @@ -2020,6 +2029,7 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages) > */ > local_add(page_entries, &cpu_buffer->overrun); > local_sub(BUF_PAGE_SIZE, &cpu_buffer->entries_bytes); > + local_inc(&cpu_buffer->pages_lost); Maybe we can make this part a static helper function so that we don't repeat it below? > } > > /* > @@ -2504,6 +2514,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer, > */ > local_add(entries, &cpu_buffer->overrun); > local_sub(BUF_PAGE_SIZE, &cpu_buffer->entries_bytes); > + local_inc(&cpu_buffer->pages_lost); Thanks, > > /* > * The entries will be zeroed out when we move the > @@ -5254,6 +5265,7 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer) > local_set(&cpu_buffer->committing, 0); > local_set(&cpu_buffer->commits, 0); > local_set(&cpu_buffer->pages_touched, 0); > + local_set(&cpu_buffer->pages_lost, 0); > local_set(&cpu_buffer->pages_read, 0); > cpu_buffer->last_pages_touch = 0; > cpu_buffer->shortest_full = 0; > -- > 2.35.1 >
On Mon, 24 Oct 2022 22:13:28 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > @@ -2020,6 +2029,7 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages) > > */ > > local_add(page_entries, &cpu_buffer->overrun); > > local_sub(BUF_PAGE_SIZE, &cpu_buffer->entries_bytes); > > + local_inc(&cpu_buffer->pages_lost); > > Maybe we can make this part a static helper function so that we don't > repeat it below? Makes sense. I'll send a v2. -- Steve > > > } > > > > /* > > @@ -2504,6 +2514,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer, > > */ > > local_add(entries, &cpu_buffer->overrun); > > local_sub(BUF_PAGE_SIZE, &cpu_buffer->entries_bytes); > > + local_inc(&cpu_buffer->pages_lost);
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c index b60047de897e..f712006f6dd3 100644 --- a/kernel/trace/ring_buffer.c +++ b/kernel/trace/ring_buffer.c @@ -519,6 +519,7 @@ struct ring_buffer_per_cpu { local_t committing; local_t commits; local_t pages_touched; + local_t pages_lost; local_t pages_read; long last_pages_touch; size_t shortest_full; @@ -894,10 +895,18 @@ size_t ring_buffer_nr_pages(struct trace_buffer *buffer, int cpu) size_t ring_buffer_nr_dirty_pages(struct trace_buffer *buffer, int cpu) { size_t read; + size_t lost; size_t cnt; read = local_read(&buffer->buffers[cpu]->pages_read); + lost = local_read(&buffer->buffers[cpu]->pages_lost); cnt = local_read(&buffer->buffers[cpu]->pages_touched); + + if (WARN_ON_ONCE(cnt < lost)) + return 0; + + cnt -= lost; + /* The reader can read an empty page, but not more than that */ if (cnt < read) { WARN_ON_ONCE(read > cnt + 1); @@ -2020,6 +2029,7 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages) */ local_add(page_entries, &cpu_buffer->overrun); local_sub(BUF_PAGE_SIZE, &cpu_buffer->entries_bytes); + local_inc(&cpu_buffer->pages_lost); } /* @@ -2504,6 +2514,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer, */ local_add(entries, &cpu_buffer->overrun); local_sub(BUF_PAGE_SIZE, &cpu_buffer->entries_bytes); + local_inc(&cpu_buffer->pages_lost); /* * The entries will be zeroed out when we move the @@ -5254,6 +5265,7 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer) local_set(&cpu_buffer->committing, 0); local_set(&cpu_buffer->commits, 0); local_set(&cpu_buffer->pages_touched, 0); + local_set(&cpu_buffer->pages_lost, 0); local_set(&cpu_buffer->pages_read, 0); cpu_buffer->last_pages_touch = 0; cpu_buffer->shortest_full = 0;