Re: [PATCH] libtracefs: Add ring buffer memory mapping APIs

From: Vincent Donnefort <vdonnefort@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Linux Trace Devel <linux-trace-devel@vger.kernel.org>
Subject: Re: [PATCH] libtracefs: Add ring buffer memory mapping APIs
Date: Fri, 5 Jan 2024 14:25:15 +0000	[thread overview]
Message-ID: <ZZgRS_5qLV0O-iue@google.com> (raw)
In-Reply-To: <20240105084100.5fe6ffa0@gandalf.local.home>

[...]

> > > +	/*
> > > +	 * Perhaps the reader page had a write that added
> > > +	 * more data.
> > > +	 */
> > > +	kbuffer_refresh(kbuf);
> > > +
> > > +	/* Are there still events to read? */
> > > +	if (kbuffer_curr_size(kbuf))
> > > +		return 1;  
> > 
> > It does not seem to be enough, only kbuf->size is updated in kbuffer_refresh()
> > while kbuffer_curr_size is next - cur.
> 
> That's the size of the event, not what's on the buffer. Next just points to
> the end of the current event. Hmm, or you mean that we read the last event
> and something else was added? Yeah, should check if curr == size, then next
> would need to be updated. That's a bug in kbuffer_refresh().

Yeah, probably kbuffer_refresh should also update kbuf->next and not only
kbuf->size.

> 
> > 
> > > +
> > > +	/* See if a new page is ready? */
> > > +	if (ioctl(tmap->fd, TRACE_MMAP_IOCTL_GET_READER) < 0)
> > > +		return -1;  
> > 
> > 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?

> 
> 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

> 
> If something is reading the buffer outside this application, it's fine if
> we read the same events. Multiple readers of the same buffer already screw
> things up today. That's why I created instances.
> 
> -- Steve
> 
> 
> > 
> > > +	id = tmap->map->reader.id;
> > > +	data = tmap->data + tmap->map->subbuf_size * id;
> > > +  
> > 
> > [...]
> 