Messages in this thread |  | | Date | Wed, 28 May 2025 09:22:09 +0900 | | From | Masami Hiramatsu (Google) <> | | Subject | Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot |
| |
On Tue, 27 May 2025 09:47:57 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 27 May 2025 12:54:44 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > Here is what I meant. As far as I ran my test, it looks good (it prevents > > over-read by `cat per_cpu/cpu0/trace_pipe_raw`) > > > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > > index 5034bae02f08..de1831eb3446 100644 > > --- a/kernel/trace/ring_buffer.c > > +++ b/kernel/trace/ring_buffer.c > > @@ -5405,6 +5405,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) > > unsigned long flags; > > int nr_loops = 0; > > bool ret; > > + u64 ts; > > > > local_irq_save(flags); > > arch_spin_lock(&cpu_buffer->lock); > > @@ -5423,6 +5424,18 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) > > > > reader = cpu_buffer->reader_page; > > > > + /* > > + * Now the page->commit is not cleared when it read. > > + * Check whether timestamp is newer instead. We also don't > > + * care the head_page is overwritten. In that case, timestamp > > + * should be newer than reader timestamp too. > > + */ > > + ts = cpu_buffer->head_page->page->time_stamp; > > + if (ts < reader->page->time_stamp) { > > Hmm, I think this test may be too fragile. The head_page can be moved > by the writer, and this would need to handle races.
Good point! Can we pick the page out from ring buffer as same as reader_page? If its timestamp is newer, we push the reader page (swap reader and head), or push back the header page.
> > I found an issue with commit overflow and have a couple of bugs to fix that > touches some of this code. Let's revisit after I get those fixed.
OK, let's review it.
BTW, we need a ring buffer test tool not depending on perf tool.
Thank you,
> > Thanks, > > -- Steve > > > > > + reader = NULL; > > + goto out; > > + } > > + > > /* If there's more to read, return this page */ > > if (cpu_buffer->reader_page->read < rb_page_size(reader)) > > goto out; > > >
-- Masami Hiramatsu (Google) <mhiramat@kernel.org>
|  |