On Fri, 23 May 2025 19:20:53 -0400
Steven Rostedt <rost...@goodmis.org> wrote:
> On Fri, 23 May 2025 16:54:25 -0400
> Steven Rostedt <rost...@goodmis.org> wrote:
>
> > > spin:
> > > @@ -6642,7 +6739,6 @@ int ring_buffer_read_page(struct trace_buffer
> > > *buffer,
> > > cpu_buffer->read_bytes += rb_page_size(reader);
> > >
> > > /* swap the pages */
> > > - rb_init_page(bpage);
> >
> > This isn't needed, because the persistent ring buffer is never swapped. And
> > this is what caused my test to fail. For some reason (and I'm still trying
> > to figure out exactly why), it causes my stress test that runs:
> >
> > perf record -o perf-test.dat -a -- trace-cmd record -e all -p function
> > /work/c/hackbench 10 || exit -1
> >
> > To never end after tracing is stopped, and it fills up all the disk space.
Thanks for sharing the test code. So it means perf reads the ring buffer
endlessly?
[reader commit=X1, ts=1]
[head commit=X2, ts=2]...[tail commit=Xn, ts=n]
| |
|---------------------------|
I thought that the read will end when it hits tail or ts < ts_current.
> >
> > But again, this part isn't needed because the persistent ring buffer
> > doesn't do the swapping. This replaces what the user passed in with the
> > current page.
> >
> > > bpage = reader->page;
> > > reader->page = data_page->data;
> > > local_set(&reader->write, 0);
>
> So I analyzed why this fails and we need to reset the commit here.
>
> Adding a bunch of trace_printk() (and using the new trace_printk_dest
> option where I can have trace_printk go into an instance) I was able to see
> why this was an issue.
>
> This part of the code swaps the reader page with what was passed in by the
> caller. The page doesn't go back into the write part of the ring buffer.
> The "commit" field is used to know if there's more data or not.
>
> By not resetting the "commit" field, we have:
>
> reader = rb_get_reader_page(cpu_buffer)
> if (cpu_buffer->reader_page->read < rb_page_size(reader))
> return reader;
> // swap passed in page with reader
>
> Without resetting "commit", the caller consumes the page and then uses that
> same page to pass back to this function. Since the "commit" field is never
> zero'd, it the above if statement always returns true! And this function
> just keeps swapping the reader each time and goes into an infinite loop
> (this loop requires user space to do a read or splice on this page so it's
> not a kernel infinite loop).
Ah, in rb_get_reader_page(cpu_buffer),
/* If there's more to read, return this page */
if (cpu_buffer->reader_page->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)))
goto out;
/* check if we caught up to the tail */
reader = NULL;
if (cpu_buffer->commit_page == cpu_buffer->reader_page)
goto out;
It checks remaining (unread) data first, and move to the next.
>
> Now the question is, can this affect the persistent ring buffer too? I'll
> memory map the buffer and see if it causes the same issue.
Yeah, it can happen, but I didn't hit that.
Let me test it too.
Hmm, BTW, is there any possible solution? records the consumed
bytes in meta data?
>
> -- Steve
--
Masami Hiramatsu (Google) <mhira...@kernel.org>
