Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752228AbdGBIv5 (ORCPT ); Sun, 2 Jul 2017 04:51:57 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33069 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751606AbdGBIvx (ORCPT ); Sun, 2 Jul 2017 04:51:53 -0400 MIME-Version: 1.0 In-Reply-To: <6272b132695d1404c64430f9c81a9365a1686dee.1498510759.git.tom.zanussi@linux.intel.com> References: <6272b132695d1404c64430f9c81a9365a1686dee.1498510759.git.tom.zanussi@linux.intel.com> From: "Joel Fernandes (Google)" Date: Sun, 2 Jul 2017 01:51:51 -0700 Message-ID: Subject: Re: [PATCH 04/32] ring-buffer: Redefine the unimplemented RINGBUF_TIME_TIME_STAMP To: Tom Zanussi Cc: rostedt@goodmis.org, tglx@linutronix.de, mhiramat@kernel.org, namhyung@kernel.org, vedang.patel@intel.com, Linux Kernel Mailing List , linux-rt-users@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4426 Lines: 108 On Mon, Jun 26, 2017 at 3:49 PM, Tom Zanussi wrote: > RINGBUF_TYPE_TIME_STAMP is defined but not used, and from what I can > gather was reserved for something like an absolute timestamp feature > for the ring buffer, if not a complete replacement of the current > time_delta scheme. > > This code redefines RINGBUF_TYPE_TIME_STAMP to implement absolute time > stamps. Another way to look at it is that it essentially forces > extended time_deltas for all events. > > The motivation for doing this is to enable time_deltas that aren't > dependent on previous events in the ring buffer, making it feasible to > use the ring_buffer_event timetamps in a more random-access way, for > purposes other than serial event printing. > > To set/reset this mode, use tracing_set_timestamp_abs() from the > previous interface patch. > > Signed-off-by: Tom Zanussi > --- > include/linux/ring_buffer.h | 12 ++--- > kernel/trace/ring_buffer.c | 107 +++++++++++++++++++++++++++++++------------- > 2 files changed, 83 insertions(+), 36 deletions(-) > > diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h > index 28e3472..74bc276 100644 > --- a/include/linux/ring_buffer.h > +++ b/include/linux/ring_buffer.h > @@ -36,10 +36,12 @@ struct ring_buffer_event { > * array[0] = time delta (28 .. 59) > * size = 8 bytes > * > - * @RINGBUF_TYPE_TIME_STAMP: Sync time stamp with external clock > - * array[0] = tv_nsec > - * array[1..2] = tv_sec > - * size = 16 bytes > + * @RINGBUF_TYPE_TIME_STAMP: Absolute timestamp > + * Same format as TIME_EXTEND except that the > + * value is an absolute timestamp, not a delta > + * event.time_delta contains bottom 27 bits > + * array[0] = top (28 .. 59) bits > + * size = 8 bytes > * Let's also change it here in ring_buffer.c? enum { RB_LEN_TIME_EXTEND = 8, RB_LEN_TIME_STAMP = 16, <- change to 8 }; > * <= @RINGBUF_TYPE_DATA_TYPE_LEN_MAX: > * Data record > @@ -56,12 +58,12 @@ enum ring_buffer_type { > RINGBUF_TYPE_DATA_TYPE_LEN_MAX = 28, > RINGBUF_TYPE_PADDING, > RINGBUF_TYPE_TIME_EXTEND, > - /* FIXME: RINGBUF_TYPE_TIME_STAMP not implemented */ > RINGBUF_TYPE_TIME_STAMP, > }; > > unsigned ring_buffer_event_length(struct ring_buffer_event *event); > void *ring_buffer_event_data(struct ring_buffer_event *event); > +u64 ring_buffer_event_time_stamp(struct ring_buffer_event *event); > > /* > * ring_buffer_discard_commit will remove an event that has not > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index f544738..df848f0 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -42,6 +42,8 @@ int ring_buffer_print_entry_header(struct trace_seq *s) > RINGBUF_TYPE_PADDING); > trace_seq_printf(s, "\ttime_extend : type == %d\n", > RINGBUF_TYPE_TIME_EXTEND); > + trace_seq_printf(s, "\ttime_stamp : type == %d\n", > + RINGBUF_TYPE_TIME_STAMP); > trace_seq_printf(s, "\tdata max type_len == %d\n", > RINGBUF_TYPE_DATA_TYPE_LEN_MAX); > > @@ -147,6 +149,9 @@ enum { > #define skip_time_extend(event) \ > ((struct ring_buffer_event *)((char *)event + RB_LEN_TIME_EXTEND)) > > +#define extended_time(event) \ > + (event->type_len >= RINGBUF_TYPE_TIME_EXTEND) > + > static inline int rb_null_event(struct ring_buffer_event *event) > { > return event->type_len == RINGBUF_TYPE_PADDING && !event->time_delta; > @@ -187,10 +192,8 @@ static void rb_event_set_padding(struct ring_buffer_event *event) > return event->array[0] + RB_EVNT_HDR_SIZE; > > case RINGBUF_TYPE_TIME_EXTEND: > - return RB_LEN_TIME_EXTEND; > - > case RINGBUF_TYPE_TIME_STAMP: > - return RB_LEN_TIME_STAMP; > + return RB_LEN_TIME_EXTEND; And then this change can be dropped. Or a new single enum can be defined for both TIME_EXTEND and TIME_STAMP and then used. thanks, -Joel