On Tue, 3 Jun 2025 12:00:49 -0400 Steven Rostedt <[email protected]> wrote:
> On Fri, 23 May 2025 00:54:44 +0900 > "Masami Hiramatsu (Google)" <[email protected]> wrote: > > > @@ -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); > > bpage = reader->page; > > reader->page = data_page->data; > > local_set(&reader->write, 0); > > Here's the bug we were looking for. We definitely need to keep the > rb_init_page() here! Ah, thanks for finding the bug! > > That's because this is the } else { part of the if condition that on true > will do a copy and not a swap of the reader page. > > ring_buffer_read_page() has: > > if (read || (len < (commit - read)) || > cpu_buffer->reader_page == cpu_buffer->commit_page || > cpu_buffer->mapped) { > > // Copy the buffer to the passed in data_page > > } else { > > // swap the data_page with the reader page > > } > > The else part is here, and it's swapping the passed in data_page with the > current reader_page. We have to initialize the data_page here. Oops, I confused the "swap" meant reader <-> head, but that should be done in rb_get_reader_page(), not here. > > What we see happening is: > > info->spare = ring_buffer_alloc_read_page(); <-- this is the data_page > > [..] > > ret = ring_buffer_read_page(..., info->spare, ...); > > Since this is a normal buffer, we swap the reader_page with info->spare, > where info->spare now has the reader_page. > > It consumes the data via: > > trace_data = ring_buffer_read_data(info->spare); > > Then reads the buffer again: > > ret = ring_buffer_read_page(..., info->spare, ...); > > Now it hits the if statement again: > > if (read || (len < (commit - read)) || > cpu_buffer->reader_page == cpu_buffer->commit_page || > cpu_buffer->mapped) { > > // Copy the buffer to the passed in data_page So all mapped buffers (including persistent ring buffer) pass this block. > > } else { > > // swap the data_page with the reader page > > ---->> here it just swaps the last data_page with the current one. So the data_page should not have any data. its commit should be 0. > > } > > > As we never clear the "commit" part of the page, it still thinks it has > content on the page as the "read" was set to zero, and it reads the old > content again. Got it. > > There's no reason not to clear the "commit" of the data_page passed in. It > is not old data that is about to be lost. We most definitely need to call > rb_init_page() on it. Yeah, that data should be considered as cleared. > > After adding that back, trace-cmd record works properly again. > > Care to send a v4 without removing this rb_init_page(bpage); ? OK, let me remove that (also remove timestamp check). Thank you! > > -- Steve -- Masami Hiramatsu (Google) <[email protected]>
