On Fri, Jan 05, 2024 at 12:31:11PM -0500, Steven Rostedt wrote:
> On Fri, 5 Jan 2024 14:25:15 +0000
> Vincent Donnefort <vdonnefort@xxxxxxxxxx> wrote:
>
> > > > Maybe this ioctl should be called regardless if events are found on the current
> > > > reader page. This would at least update the reader->read field and make sure
> > > > subsequent readers are not getting the same events we already had here?
> > >
> > > If we call the ioctl() before we are finished reading events, the events on
> > > the reader page will be discarded and added back to the writer buffer if
> > > the writer is not on the reader page.
> >
> > Hum, I am a bit confused here. If the writer is not on the reader page, then
> > there's no way a writer could overwrite these events while we access them?
>
> Maybe I'm confused. Where do you want to call the ioctl again? If the
> writer is off the reader page and we call the ioctl() where no new events
> were added since our last read, the ioctl() will advance the reader page,
> will it not? That is, any events that were on the previous reader page that
> we did not read yet is lost.
I meant like the snippet you shared below.
If on a reader page, there are "unread" events, the ioctl will simply return the
same reader page, as long as there is something to read. It is then safe to call
the ioctl unconditionally.
>
> The trace_mmap_load_subbuf() is called to update the reader page. If for
> some reason the kbuf hasn't read all the data and calls the
> tracefs_cpu_read_buf() again, it should still return the same kbuf, but
> updated.
>
> kbuf = tracefs_cpu_read_buf() {
> ret = trace_mmap_load_subbuf(tcpu->mapping, tcpu->kbuf) {
> if (data != kbuffer_subbuffer(kbuf)) {
> kbuffer_load_subbuffer(kbuf, data);
> return 1;
> }
> return ret > 0 ? tcpu->kbuf : NULL;
> }
>
> record.data = kbuffer_read_event(kbuf, &record.ts);
> kbuffer_next_event(kbuf, NULL);
>
> kbuf = tracefs_cpu_read_buf() {
> ret = trace_mmap_load_subbuf(tcpu->mapping, tcpu->kbuf) {
> kbuffer_refresh(kbuf);
> /* Are there still events to read? */
> if (kbuffer_curr_size(kbuf))
> return 1;
>
> The above should return because the application has not read all of the
> kbuf yet. This is perfectly fine for the application to do this.
>
> If we don't return here, but instead do:
>
> ioctl(tmap->fd, TRACE_MMAP_IOCTL_GET_READER);
>
> The buffer that kbuf points to will be swapped out with a new page. And the
> data on the buffer is lost.
It shouldn't, the same reader page will be returned, only the reader_page->read
pointer would be updated.
>
> Hmm, but looking at how the code currently works without mapping, it looks
> like it would do the read again anyway assuming that the kbuf was
> completely read before reading again. Perhaps it would make the logic
> simpler to assume that the buffer was completely read before this gets
> called again. That is, we could have it do:
>
> if (data != kbuffer_subbuffer(kbuf)) {
> kbuffer_load_subbuffer(kbuf, data);
> return 1;
> }
>
> if (ioctl(tmap->fd, TRACE_MMAP_IOCTL_GET_READER) < 0)
> return -1;
>
> if (id != tmap->map->reader.id) {
> /* New page, just reload it */
> data = tmap->data + tmap->map->subbuf_size * id;
> kbuffer_load_subbuffer(kbuf, data);
> return 1;
> }
>
> /*
> * Perhaps the reader page had a write that added
> * more data.
> */
> kbuffer_refresh(kbuf);
>
> /* Are there still events to read? */
> return kbuffer_curr_size(kbuf) ? 1 : 0;
>
That sounds good
>
> >
> > >
> > > And there should only be one reader accessing the map. This is not thread
> > > safe. Once you load the subbuffer into kbuf, kbuf handles what was read. We
> > > don't want to use the reader page for that.
> >
> > I had in mind a scenario with two sequential read. In that case only the reader
> > page read could help to "save" what has been read so far.
> >
> > Currently, we have:
> >
> > <event A>
> > ./cpu-map
> > print event A
> >
> > <event B>
> > ./cpu-map
> > print event A
> > print event B
>
> The kbuffer keeps track of what was read from it, so I'm still confused by
> what you mean.
We probably want to keep track of what has already been consumed outside of
the remits of a single libtraceevent execution.
But now with the code snippet above, in addition to the fast-forward when the
kbuf is first loaded (kbuffer_read_buffer(kbuf, tmpbuf, tmap->map->reader.read)),
we shouldn't have that problem anymore.
<event A>
$ ./cpu-map
print event A
<event B>
$ ./cpu-map
print event B
Or even
<event A>
$ ./cpu-map
print event A
<event B>
$ cat /sys/kernel/tracing/trace_pipe
print event B
>
> -- Steve
|