| Message ID | 20240123123337.1f785f98@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5f27b7f3fb7d88b29522baf3883cc0e2e28b1af0 |
| Headers | show |
| Series | libtracefs: Update the kbuf for previous read in trace_mmap_load_subbuf() | expand |
On Tue, Jan 23, 2024 at 12:33:37PM -0500, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > The trace_mmap() checks if the mmapped data has any previously read data > (the reader.read value of the meta page is non-zero), then it will pre-read > the kbuf to move its internal reader pointer to the same value. > > But when tracefs_cpu_read_buf() calls trace_mmap_load_subbuf(), its > kbuf->subbuffer will not be the same as the mapped data and > kbuffer_load_subbuffer() is called on it and it is returned. But that means > its read pointer has not been updated, and the read data will restart again. > > When the kbuf is updated in trace_mmap_load_subbuf() check the tmap->kbuf to > see if it already read any of the data, and move the kbuffer forward just > like the trace_mmap() does. > > Link: https://lore.kernel.org/linux-trace-devel/Za-Md51snPcIoYFn@google.com/ > > Fixes: 2ed14b59 ("libtracefs: Add ring buffer memory mapping APIs") > Reported-by: Vincent Donnefort <vdonnefort@google.com> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> That fixes fast-forward, event already read are now skipped. > --- > src/tracefs-mmap.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/tracefs-mmap.c b/src/tracefs-mmap.c > index d3af453..a288677 100644 > --- a/src/tracefs-mmap.c > +++ b/src/tracefs-mmap.c > @@ -165,6 +165,12 @@ __hidden int trace_mmap_load_subbuf(void *mapping, struct kbuffer *kbuf) > */ > if (data != kbuffer_subbuffer(kbuf)) { > kbuffer_load_subbuffer(kbuf, data); > + /* Move the read pointer forward if need be */ > + if (kbuffer_curr_index(tmap->kbuf)) { > + int size = kbuffer_curr_offset(tmap->kbuf); > + char tmpbuf[size]; > + kbuffer_read_buffer(kbuf, tmpbuf, size); > + } > return 1; > } > > -- > 2.43.0 >
On Tue, 23 Jan 2024 17:41:59 +0000 Vincent Donnefort <vdonnefort@google.com> wrote: > On Tue, Jan 23, 2024 at 12:33:37PM -0500, Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > > > The trace_mmap() checks if the mmapped data has any previously read data > > (the reader.read value of the meta page is non-zero), then it will pre-read > > the kbuf to move its internal reader pointer to the same value. > > > > But when tracefs_cpu_read_buf() calls trace_mmap_load_subbuf(), its > > kbuf->subbuffer will not be the same as the mapped data and > > kbuffer_load_subbuffer() is called on it and it is returned. But that means > > its read pointer has not been updated, and the read data will restart again. > > > > When the kbuf is updated in trace_mmap_load_subbuf() check the tmap->kbuf to > > see if it already read any of the data, and move the kbuffer forward just > > like the trace_mmap() does. > > > > Link: https://lore.kernel.org/linux-trace-devel/Za-Md51snPcIoYFn@google.com/ > > > > Fixes: 2ed14b59 ("libtracefs: Add ring buffer memory mapping APIs") > > Reported-by: Vincent Donnefort <vdonnefort@google.com> > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > That fixes fast-forward, event already read are now skipped. Can I also add your "Tested-by" tag then? -- Steve
On Wed, Jan 24, 2024 at 11:56:51AM -0500, Steven Rostedt wrote: > On Tue, 23 Jan 2024 17:41:59 +0000 > Vincent Donnefort <vdonnefort@google.com> wrote: > > > On Tue, Jan 23, 2024 at 12:33:37PM -0500, Steven Rostedt wrote: > > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > > > > > The trace_mmap() checks if the mmapped data has any previously read data > > > (the reader.read value of the meta page is non-zero), then it will pre-read > > > the kbuf to move its internal reader pointer to the same value. > > > > > > But when tracefs_cpu_read_buf() calls trace_mmap_load_subbuf(), its > > > kbuf->subbuffer will not be the same as the mapped data and > > > kbuffer_load_subbuffer() is called on it and it is returned. But that means > > > its read pointer has not been updated, and the read data will restart again. > > > > > > When the kbuf is updated in trace_mmap_load_subbuf() check the tmap->kbuf to > > > see if it already read any of the data, and move the kbuffer forward just > > > like the trace_mmap() does. > > > > > > Link: https://lore.kernel.org/linux-trace-devel/Za-Md51snPcIoYFn@google.com/ > > > > > > Fixes: 2ed14b59 ("libtracefs: Add ring buffer memory mapping APIs") > > > Reported-by: Vincent Donnefort <vdonnefort@google.com> > > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > > > That fixes fast-forward, event already read are now skipped. > > Can I also add your "Tested-by" tag then? > > -- Steve Of course, I wasn't sure you were using those tags for libtracefs. Will make it more explicit next time!
On Wed, 24 Jan 2024 17:54:00 +0000 Vincent Donnefort <vdonnefort@google.com> wrote: > > Can I also add your "Tested-by" tag then? > > > > -- Steve > > Of course, I wasn't sure you were using those tags for libtracefs. Will make it > more explicit next time! Thanks. Yeah, I try to follow the "kernel way" of doing things. -- Steve
diff --git a/src/tracefs-mmap.c b/src/tracefs-mmap.c index d3af453..a288677 100644 --- a/src/tracefs-mmap.c +++ b/src/tracefs-mmap.c @@ -165,6 +165,12 @@ __hidden int trace_mmap_load_subbuf(void *mapping, struct kbuffer *kbuf) */ if (data != kbuffer_subbuffer(kbuf)) { kbuffer_load_subbuffer(kbuf, data); + /* Move the read pointer forward if need be */ + if (kbuffer_curr_index(tmap->kbuf)) { + int size = kbuffer_curr_offset(tmap->kbuf); + char tmpbuf[size]; + kbuffer_read_buffer(kbuf, tmpbuf, size); + } return 1; }