Re: [PATCH 04/32] ring-buffer: Redefine the unimplemented RINGBUF_TIME_TIME_STAMP

2017-07-02 Thread Joel Fernandes (Google) 
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

Re: [PATCH 04/32] ring-buffer: Redefine the unimplemented RINGBUF_TIME_TIME_STAMP

2017-07-02 Thread Joel Fernandes (Google) 
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

[PATCH 04/32] ring-buffer: Redefine the unimplemented RINGBUF_TIME_TIME_STAMP

2017-06-26 Thread Tom Zanussi 
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
  *
  * <= @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;
 
case RINGBUF_TYPE_DATA:
return rb_event_data_length(event);
@@ -210,7 +213,7 @@ static void rb_event_set_padding(struct ring_buffer_event 
*event)
 {
unsigned len = 0;
 
-   if (event->type_len == RINGBUF_TYPE_TIME_EXTEND) {
+   if (extended_time(event)) {
/* time extends include the data event after it */
len = RB_LEN_TIME_EXTEND;
event = skip_time_extend(event);
@@ -232,7 +235,7 @@ unsigned ring_buffer_event_length(struct ring_buffer_event 
*event)
 {
unsigned length;
 
-   if (event->type_len == RINGBUF_TYPE_TIME_EXTEND)
+   if (extended_time(event))
event = skip_time_extend(event);
 
length = rb_event_length(event);
@@ -249,7 +252,7 @@ unsigned ring_buffer_event_length(struct ring_buffer_event 
*event)
 static __always_inline void *
 rb_event_data(struct ring_buffer_event *event)
 {
-   if (event->type_len == RINGBUF_TYPE_TIME_EXTEND)
+   if (extended_time(event))
event = skip_time_extend(event);
BUG_ON(event->type_len

[PATCH 04/32] ring-buffer: Redefine the unimplemented RINGBUF_TIME_TIME_STAMP

2017-06-26 Thread Tom Zanussi 
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
  *
  * <= @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;
 
case RINGBUF_TYPE_DATA:
return rb_event_data_length(event);
@@ -210,7 +213,7 @@ static void rb_event_set_padding(struct ring_buffer_event 
*event)
 {
unsigned len = 0;
 
-   if (event->type_len == RINGBUF_TYPE_TIME_EXTEND) {
+   if (extended_time(event)) {
/* time extends include the data event after it */
len = RB_LEN_TIME_EXTEND;
event = skip_time_extend(event);
@@ -232,7 +235,7 @@ unsigned ring_buffer_event_length(struct ring_buffer_event 
*event)
 {
unsigned length;
 
-   if (event->type_len == RINGBUF_TYPE_TIME_EXTEND)
+   if (extended_time(event))
event = skip_time_extend(event);
 
length = rb_event_length(event);
@@ -249,7 +252,7 @@ unsigned ring_buffer_event_length(struct ring_buffer_event 
*event)
 static __always_inline void *
 rb_event_data(struct ring_buffer_event *event)
 {
-   if (event->type_len == RINGBUF_TYPE_TIME_EXTEND)
+   if (extended_time(event))
event = skip_time_extend(event);
BUG_ON(event->type_len >