[PATCH 0/5] tracing: Revmoe absolute timestamp logic and interface

[PATCH 0/5] tracing: Revmoe absolute timestamp logic and interface

[sunliming]

Commit efe6196a6bc5 ("ring-buffer: Allow ring_buffer_event_time_stamp() to
return time stamp of all events") make ring_buffer_event_time_stamp() have
the ability to return time stamp of all events. The hist trigger had
also switch to the new logic to get time_stamp. The absolute timestamp
logic had bocome useless, so it showed be removed.

[PATCH 1/5] tracing/histogram: Fix incorrect description in histogram.rst

[sunliming]

Commit b94bc80df648 ("tracing: Use a no_filter_buffering_ref to stop using the
filter buffer") switch the hist trigger to not use absolute time stamps but
left the documents incorrect, fix it.

Signed-off-by: sunliming <sunliming@kylinos.cn>
---
 Documentation/trace/histogram.rst | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/trace/histogram.rst b/Documentation/trace/histogram.rst
index c1b685a38f6b..5eecf2e1c2cc 100644
--- a/Documentation/trace/histogram.rst
+++ b/Documentation/trace/histogram.rst
@@ -1667,12 +1667,11 @@ features have been added to the hist trigger support:
 
 A note on inter-event timestamps: If common_timestamp is used in a
 histogram, the trace buffer is automatically switched over to using
-absolute timestamps and the "global" trace clock, in order to avoid
-bogus timestamp differences with other clocks that aren't coherent
-across CPUs.  This can be overridden by specifying one of the other
-trace clocks instead, using the "clock=XXX" hist trigger attribute,
-where XXX is any of the clocks listed in the tracing/trace_clock
-pseudo-file.
+the "global" trace clock, in order to avoid bogus timestamp differences
+with other clocks that aren't coherent across CPUs. This can be overridden
+by specifying one of the other trace clocks instead, using the "clock=XXX"
+hist trigger attribute, where XXX is any of the clocks listed in the
+tracing/trace_clock pseudo-file.
 
 These features are described in more detail in the following sections.
 
-- 
2.25.1

[PATCH 2/5] ring-buffer: Remove absolute timestamp from add_timestamp logic

[sunliming]

Remove absolute timestamp from add_timestamp logic.

Signed-off-by: sunliming <sunliming@kylinos.cn>
---
 kernel/trace/ring_buffer.c | 49 +++++++++++++-------------------------
 1 file changed, 17 insertions(+), 32 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 199759c73519..ab0aef15f82a 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -436,14 +436,12 @@ struct rb_event_info {
  * Used for the add_timestamp
  *  NONE
  *  EXTEND - wants a time extend
- *  ABSOLUTE - the buffer requests all events to have absolute time stamps
  *  FORCE - force a full time stamp.
  */
 enum {
 	RB_ADD_STAMP_NONE		= 0,
 	RB_ADD_STAMP_EXTEND		= BIT(1),
-	RB_ADD_STAMP_ABSOLUTE		= BIT(2),
-	RB_ADD_STAMP_FORCE		= BIT(3)
+	RB_ADD_STAMP_FORCE		= BIT(2)
 };
 /*
  * Used for which event context the event is in.
@@ -2832,8 +2830,7 @@ static void rb_add_timestamp(struct ring_buffer_per_cpu *cpu_buffer,
 				      u64 *delta,
 				      unsigned int *length)
 {
-	bool abs = info->add_timestamp &
-		(RB_ADD_STAMP_FORCE | RB_ADD_STAMP_ABSOLUTE);
+	bool abs = info->add_timestamp & RB_ADD_STAMP_FORCE;
 
 	if (unlikely(info->delta > (1ULL << 59))) {
 		/*
@@ -3435,8 +3432,7 @@ static void check_buffer(struct ring_buffer_per_cpu *cpu_buffer,
 	if (tail == CHECK_FULL_PAGE) {
 		full = true;
 		tail = local_read(&bpage->commit);
-	} else if (info->add_timestamp &
-		   (RB_ADD_STAMP_FORCE | RB_ADD_STAMP_ABSOLUTE)) {
+	} else if (info->add_timestamp & RB_ADD_STAMP_FORCE) {
 		/* Ignore events with absolute time stamps */
 		return;
 	}
@@ -3535,23 +3531,19 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,
 	barrier();
 	info->ts = rb_time_stamp(cpu_buffer->buffer);
 
-	if ((info->add_timestamp & RB_ADD_STAMP_ABSOLUTE)) {
-		info->delta = info->ts;
+	/*
+	 * If interrupting an event time update, we may need an
+	 * absolute timestamp.
+	 * Don't bother if this is the start of a new page (w == 0).
+	 */
+	if (unlikely(!a_ok || !b_ok || (info->before != info->after && w))) {
+		info->add_timestamp |= RB_ADD_STAMP_FORCE | RB_ADD_STAMP_EXTEND;
+		info->length += RB_LEN_TIME_EXTEND;
 	} else {
-		/*
-		 * If interrupting an event time update, we may need an
-		 * absolute timestamp.
-		 * Don't bother if this is the start of a new page (w == 0).
-		 */
-		if (unlikely(!a_ok || !b_ok || (info->before != info->after && w))) {
-			info->add_timestamp |= RB_ADD_STAMP_FORCE | RB_ADD_STAMP_EXTEND;
+		info->delta = info->ts - info->after;
+		if (unlikely(test_time_stamp(info->delta))) {
+			info->add_timestamp |= RB_ADD_STAMP_EXTEND;
 			info->length += RB_LEN_TIME_EXTEND;
-		} else {
-			info->delta = info->ts - info->after;
-			if (unlikely(test_time_stamp(info->delta))) {
-				info->add_timestamp |= RB_ADD_STAMP_EXTEND;
-				info->length += RB_LEN_TIME_EXTEND;
-			}
 		}
 	}
 
@@ -3586,8 +3578,7 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,
 		barrier();
  /*E*/		s_ok = rb_time_read(&cpu_buffer->before_stamp, &save_before);
 		RB_WARN_ON(cpu_buffer, !s_ok);
-		if (likely(!(info->add_timestamp &
-			     (RB_ADD_STAMP_FORCE | RB_ADD_STAMP_ABSOLUTE))))
+		if (likely(!(info->add_timestamp & RB_ADD_STAMP_FORCE)))
 			/* This did not interrupt any time update */
 			info->delta = info->ts - info->after;
 		else
@@ -3644,8 +3635,7 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,
 	 * If this is the first commit on the page, then it has the same
 	 * timestamp as the page itself.
 	 */
-	if (unlikely(!tail && !(info->add_timestamp &
-				(RB_ADD_STAMP_FORCE | RB_ADD_STAMP_ABSOLUTE))))
+	if (unlikely(!tail && !(info->add_timestamp & RB_ADD_STAMP_FORCE)))
 		info->delta = 0;
 
 	/* We reserved something on the buffer */
@@ -3698,12 +3688,7 @@ rb_reserve_next_event(struct trace_buffer *buffer,
 
 	info.length = rb_calculate_event_length(length);
 
-	if (ring_buffer_time_stamp_abs(cpu_buffer->buffer)) {
-		add_ts_default = RB_ADD_STAMP_ABSOLUTE;
-		info.length += RB_LEN_TIME_EXTEND;
-	} else {
-		add_ts_default = RB_ADD_STAMP_NONE;
-	}
+	add_ts_default = RB_ADD_STAMP_NONE;
 
  again:
 	info.add_timestamp = add_ts_default;
-- 
2.25.1

[PATCH 3/5] ring-buffer: Remove add_ts_default to simplify code

[sunliming]

The variable add_ts_default is not necessary when absolute timestamp is
removed, so remove it to simplify code.

Signed-off-by: sunliming <sunliming@kylinos.cn>
---
 kernel/trace/ring_buffer.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index ab0aef15f82a..cb261456216f 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3666,7 +3666,6 @@ rb_reserve_next_event(struct trace_buffer *buffer,
 	struct ring_buffer_event *event;
 	struct rb_event_info info;
 	int nr_loops = 0;
-	int add_ts_default;
 
 	rb_start_commit(cpu_buffer);
 	/* The commit page can not change after this */
@@ -3688,10 +3687,8 @@ rb_reserve_next_event(struct trace_buffer *buffer,
 
 	info.length = rb_calculate_event_length(length);
 
-	add_ts_default = RB_ADD_STAMP_NONE;
-
  again:
-	info.add_timestamp = add_ts_default;
+	info.add_timestamp = RB_ADD_STAMP_NONE;
 	info.delta = 0;
 
 	/*
-- 
2.25.1

[PATCH 4/5] tracing: Delete timestamp_mode trace file

[sunliming]

The timestamp_mode trace file is not necessary duo to the removing of
the absolute timestamp.

Signed-off-by: sunliming <sunliming@kylinos.cn>
---
 Documentation/trace/ftrace.rst | 24 ------------------
 kernel/trace/trace.c           | 45 ----------------------------------
 2 files changed, 69 deletions(-)

diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
index 60bceb018d6a..9b4a4e9c1cbd 100644
--- a/Documentation/trace/ftrace.rst
+++ b/Documentation/trace/ftrace.rst
@@ -615,30 +615,6 @@ of ftrace. Here is a list of some of the key files:
 
 	See events.rst for more information.
 
-  timestamp_mode:
-
-	Certain tracers may change the timestamp mode used when
-	logging trace events into the event buffer.  Events with
-	different modes can coexist within a buffer but the mode in
-	effect when an event is logged determines which timestamp mode
-	is used for that event.  The default timestamp mode is
-	'delta'.
-
-	Usual timestamp modes for tracing:
-
-	  # cat timestamp_mode
-	  [delta] absolute
-
-	  The timestamp mode with the square brackets around it is the
-	  one in effect.
-
-	  delta: Default timestamp mode - timestamp is a delta against
-	         a per-buffer timestamp.
-
-	  absolute: The timestamp is a full timestamp, not a delta
-                 against some other value.  As such it takes up more
-                 space and is less efficient.
-
   hwlat_detector:
 
 	Directory for the Hardware Latency Detector.
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 47a44b055a1d..af8cbcad0bad 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5494,9 +5494,6 @@ static const char readme_msg[] =
 #ifdef CONFIG_X86_64
 	"     x86-tsc:   TSC cycle counter\n"
 #endif
-	"\n  timestamp_mode\t- view the mode used to timestamp events\n"
-	"       delta:   Delta difference against a buffer-wide timestamp\n"
-	"    absolute:   Absolute (standalone) timestamp\n"
 	"\n  trace_marker\t\t- Writes into this file writes into the kernel buffer\n"
 	"\n  trace_marker_raw\t\t- Writes into this file writes binary data into the kernel buffer\n"
 	"  tracing_cpumask\t- Limit which CPUs to trace\n"
@@ -7319,38 +7316,6 @@ static int tracing_clock_open(struct inode *inode, struct file *file)
 	return ret;
 }
 
-static int tracing_time_stamp_mode_show(struct seq_file *m, void *v)
-{
-	struct trace_array *tr = m->private;
-
-	mutex_lock(&trace_types_lock);
-
-	if (ring_buffer_time_stamp_abs(tr->array_buffer.buffer))
-		seq_puts(m, "delta [absolute]\n");
-	else
-		seq_puts(m, "[delta] absolute\n");
-
-	mutex_unlock(&trace_types_lock);
-
-	return 0;
-}
-
-static int tracing_time_stamp_mode_open(struct inode *inode, struct file *file)
-{
-	struct trace_array *tr = inode->i_private;
-	int ret;
-
-	ret = tracing_check_open_get_tr(tr);
-	if (ret)
-		return ret;
-
-	ret = single_open(file, tracing_time_stamp_mode_show, inode->i_private);
-	if (ret < 0)
-		trace_array_put(tr);
-
-	return ret;
-}
-
 u64 tracing_event_time_stamp(struct trace_buffer *buffer, struct ring_buffer_event *rbe)
 {
 	if (rbe == this_cpu_read(trace_buffered_event))
@@ -7643,13 +7608,6 @@ static const struct file_operations trace_clock_fops = {
 	.write		= tracing_clock_write,
 };
 
-static const struct file_operations trace_time_stamp_mode_fops = {
-	.open		= tracing_time_stamp_mode_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= tracing_single_release_tr,
-};
-
 #ifdef CONFIG_TRACER_SNAPSHOT
 static const struct file_operations snapshot_fops = {
 	.open		= tracing_snapshot_open,
@@ -9580,9 +9538,6 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
 	trace_create_file("tracing_on", TRACE_MODE_WRITE, d_tracer,
 			  tr, &rb_simple_fops);
 
-	trace_create_file("timestamp_mode", TRACE_MODE_READ, d_tracer, tr,
-			  &trace_time_stamp_mode_fops);
-
 	tr->buffer_percent = 50;
 
 	trace_create_file("buffer_percent", TRACE_MODE_READ, d_tracer,
-- 
2.25.1

Re: [PATCH 4/5] tracing: Delete timestamp_mode trace file

[Steven Rostedt]

On Tue, 18 Oct 2022 20:00:55 +0800
sunliming <sunliming@kylinos.cn> wrote:

> The timestamp_mode trace file is not necessary duo to the removing of
> the absolute timestamp.

This is user space exposed API. Which means we do not just simply "remove
it". This file is the reason I kept the absolute timestamps around in the
first place. Because user space may want them.

How do you know that user space isn't using this? Is there something else
you are planning on adding that makes taking the risk we might break user
space worth while?

-- Steve

Re: [PATCH 4/5] tracing: Delete timestamp_mode trace file

[sunliming]

Got it.

Steven Rostedt <rostedt@goodmis.org> 于2022年10月18日周二 21:26写道：
>
> On Tue, 18 Oct 2022 20:00:55 +0800
> sunliming <sunliming@kylinos.cn> wrote:
>
> > The timestamp_mode trace file is not necessary duo to the removing of
> > the absolute timestamp.
>
> This is user space exposed API. Which means we do not just simply "remove
> it". This file is the reason I kept the absolute timestamps around in the
> first place. Because user space may want them.
>
> How do you know that user space isn't using this? Is there something else
> you are planning on adding that makes taking the risk we might break user
> space worth while?
>
> -- Steve

[PATCH 5/5] ring-buffer: Delete interface for setting absolute time stamps

[sunliming]

Commit efe6196a6bc5 ("ring-buffer: Allow ring_buffer_event_time_stamp() to
return time stamp of all events") make ring_buffer_event_time_stamp() have
the ability to return time stamp of all events. And there is no user about
this interface. So remove it.

Signed-off-by: sunliming <sunliming@kylinos.cn>
---
 include/linux/ring_buffer.h |  2 --
 kernel/trace/ring_buffer.c  | 11 -----------
 2 files changed, 13 deletions(-)

diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
index 2504df9a0453..b7bcf2ee0945 100644
--- a/include/linux/ring_buffer.h
+++ b/include/linux/ring_buffer.h
@@ -186,8 +186,6 @@ void ring_buffer_normalize_time_stamp(struct trace_buffer *buffer,
 				      int cpu, u64 *ts);
 void ring_buffer_set_clock(struct trace_buffer *buffer,
 			   u64 (*clock)(void));
-void ring_buffer_set_time_stamp_abs(struct trace_buffer *buffer, bool abs);
-bool ring_buffer_time_stamp_abs(struct trace_buffer *buffer);
 
 size_t ring_buffer_nr_pages(struct trace_buffer *buffer, int cpu);
 size_t ring_buffer_nr_dirty_pages(struct trace_buffer *buffer, int cpu);
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index cb261456216f..68947652e46d 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -551,7 +551,6 @@ struct trace_buffer {
 	u64				(*clock)(void);
 
 	struct rb_irq_work		irq_work;
-	bool				time_stamp_abs;
 };
 
 struct ring_buffer_iter {
@@ -1876,16 +1875,6 @@ void ring_buffer_set_clock(struct trace_buffer *buffer,
 	buffer->clock = clock;
 }
 
-void ring_buffer_set_time_stamp_abs(struct trace_buffer *buffer, bool abs)
-{
-	buffer->time_stamp_abs = abs;
-}
-
-bool ring_buffer_time_stamp_abs(struct trace_buffer *buffer)
-{
-	return buffer->time_stamp_abs;
-}
-
 static void rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer);
 
 static inline unsigned long rb_page_entries(struct buffer_page *bpage)
-- 
2.25.1