Re: [PATCH v3 1/2] tracing: ring_buffer: Rewind persistent ring buffer when reboot

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Tue, 27 May 2025 12:15:49 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:


> > What about this? It seems if we keep the reader->page->commit, when we
> > catch up the tail page, rb_get_reader_page() returns new reader (== tail)
> > because reader->read(==0) < reader->page->commit. But we should not return
> > new reader.
> > 
> > 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 5034bae02f08..179142db9586 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -5522,6 +5522,12 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
> >                 cpu_buffer->last_overrun = overwrite;
> >         }
> >  
> > +       /* But if we catch up to the tail, do not return reader page. */
> > +       if (cpu_buffer->commit_page == cpu_buffer->reader_page) {
> 
> 
> Hmm, I found this might not work if it is called twice.
> Maybe rb_get_reader_page() can check the reader_page->ts > head_page->ts
> and return NULL (or reader_page if read < commit) at first.

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) {
+               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>