On Wed, 14 May 2025 09:00:50 -0400
Steven Rostedt <rost...@goodmis.org> wrote:
> On Wed, 14 May 2025 15:00:59 +0900
> Masami Hiramatsu (Google) <mhira...@kernel.org> wrote:
> > >
> > > Is that a problem? I'm thinking that the data in the buffer should not be
> > > used.
> >
> > Yes, even if we read (dump) the previous boot data, the data is
> > in the buffer. Thus the kernel rebooted before reusing the buffer
> > the dumped pages are recovered again. Unless comparing with the
> > previous dump data, we can not know this data is older boot or not.
> > Anyway, user can avoid this issue by clearing the trace buffer
> > explicitly.
>
> What we could do, and I don't think this would be too hard, is once the
> buffer is empty and it's still LAST_BOOT buffer, we simply clear it in
> the kernel.
Ah, that sounds good :-D
>
> That way after a reboot, a read of trace_pipe that reads the entire
> buffer will end up resetting the buffer, and I think that will solve
> this problem.
>
>
>
> > > +
> > > + /* Stop rewind if the page is invalid. */
> > > + ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu);
> > > + if (ret < 0)
> > > + break;
> > > +
> > > + /* Recover the number of entries. */
> > > + local_set(&head_page->entries, ret);
> > > + if (ret)
> > > + local_inc(&cpu_buffer->pages_touched);
> > > + entries += ret;
> > > + entry_bytes += rb_page_commit(head_page);
> >
> > If we validate the pages again later (because fixing head_page),
> > we can skip this part.
>
> The validator takes a bit of time. I would rather not do another loop
> if we don't have to. If this is duplicate code, lets just make a static
> inline helper function that does it and use that in both places.
OK, I think we can just restart validating unread part from
orig_head.
>
> >
> > > + }
> > > +
> > > + /* The last rewind page must be skipped. */
> > > + if (head_page != orig_head)
> > > + rb_inc_page(&head_page);
> > > +
> > > + if (head_page != orig_head) {
> >
> > Ah, I forgot this part (setup new reader_page)
> >
> > > + struct buffer_page *bpage = orig_head;
> > > +
> > > + rb_dec_page(&bpage);
> > > + /*
> > > + * Move the reader page between the orig_head and the page
> > > + * before it.
> > > + */
> > -----
> > > + cpu_buffer->reader_page->list.next = &orig_head->list;
> > > + cpu_buffer->reader_page->list.prev = orig_head->list.prev;
> > > + orig_head->list.prev = &cpu_buffer->reader_page->list;
> > > +
> > > + bpage->list.next = &cpu_buffer->reader_page->list;
> > -----
> > These seems the same as (because head_page->list.prev->next encodes
> > flags, but we don't read that pointer.);
> >
> > list_insert(&orig_head->list, &cpu_buffer->reader_page->list);
>
> I thought about this, but because the pointers are used to encode
> flags, I try to avoid using the list_*() functions all together on
> these. Just to remind everyone that these are "special" lists.
>
> I prefer it open coded because that way I can see exactly what it is
> doing. Note, this is not just assigning pointers, it is also clearing
> flags in the process.
OK. And I found list_insert() is not in the kernel.
(tools/firmware/list.h has that)
>
> We could add a comment that states something like:
>
> /*
> * This is the same as:
> * list_insert(&orig_head->list, &cpu_buffer->read_page->list);
> * but as it is also clearing flags, its open coded so that
> * there's no chance that list_insert() gets optimized where
> * it doesn't do the extra work that this is doing.
> */
>
> ?
Yeah, anyway I will leave a comment.
Thank you,
>
> -- Steve
>
>
> >
> > > +
> > > + /* Make the head_page the new reader page */
> > > + cpu_buffer->reader_page = head_page;
> > > + bpage = head_page;
> > > + rb_inc_page(&head_page);
> > > + head_page->list.prev = bpage->list.prev;
> > > + rb_dec_page(&bpage);
> > > + bpage->list.next = &head_page->list;
> > > + rb_set_list_to_head(&bpage->list);
> > > +
> > > + cpu_buffer->head_page = head_page;
> > > + meta->head_buffer = (unsigned long)head_page->page;
> > > +
> > > + /* Reset all the indexes */
> > > + bpage = cpu_buffer->reader_page;
> > > + meta->buffers[0] = rb_meta_subbuf_idx(meta, bpage->page);
> > > + bpage->id = 0;
> > > +
> > > + for (i = 0, bpage = head_page; i < meta->nr_subbufs;
> > > + i++, rb_inc_page(&bpage)) {
> > > + meta->buffers[i + 1] = rb_meta_subbuf_idx(meta,
> > > bpage->page);
> > > + bpage->id = i + 1;
> > > + }
> > > + head_page = orig_head;
> > > + }
> > > +
> > > /* Iterate until finding the commit page */
> > > for (i = 0; i < meta->nr_subbufs + 1; i++, rb_inc_page(&head_page)) {
> > >
> > > @@ -5348,7 +5439,6 @@ rb_get_reader_page(struct ring_buffer_per_cpu
> > > *cpu_buffer)
> > > */
> > > local_set(&cpu_buffer->reader_page->write, 0);
> > > local_set(&cpu_buffer->reader_page->entries, 0);
> > > - local_set(&cpu_buffer->reader_page->page->commit, 0);
> > > cpu_buffer->reader_page->real_end = 0;
> > >
> > > spin:
> > > @@ -6642,7 +6732,7 @@ int ring_buffer_read_page(struct trace_buffer
> > > *buffer,
> > > cpu_buffer->read_bytes += rb_page_size(reader);
> > >
> > > /* swap the pages */
> > > - rb_init_page(bpage);
> > > +// rb_init_page(bpage);
> > > bpage = reader->page;
> > > reader->page = data_page->data;
> > > local_set(&reader->write, 0);
> >
> > Thank you,
> >
> >
> >
>
--
Masami Hiramatsu (Google) <mhira...@kernel.org>
