Skip to contentSkip to conversation

tracing

PATCH In review Steven Rostedt 2 participants 7 messages Aug 3, 2013 – Aug 12, 2013

Subsystems (3)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover tracing

Hopefully this is the last of the fixes for 3.11. Famous last words. These are some minor bug fixes, but still should be in 3.11 and stable.

-- Steve

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
for-next

Head SHA1: 9457158bbc0ee04ecef76862d73eecd8076e9c7b

Alexander Z Lam (2):
tracing: Make TRACE_ITER_STOP_ON_FREE stop the correct buffer
tracing: Fix reset of time stamps during trace_clock changes

Andrew Vagin (1):
tracing: Fix fields of struct trace_iterator that are zeroed by mistake

Dhaval Giani (1):
tracing: Fix trace_dump_stack() proto when CONFIG_TRACING is not set

----

 include/linux/ftrace_event.h |   10 ++++++----
 include/linux/kernel.h       |    2 +-
 kernel/trace/trace.c         |   27 ++++++++++++++-------------
 3 files changed, 21 insertions(+), 18 deletions(-)
author maintainer
#1 lore raw

PATCH 1/4 tracing: Fix fields of struct trace_iterator that are zeroed by mistake

From: Andrew Vagin <[email protected]>

tracing_read_pipe zeros all fields bellow "seq". The declaration contains a comment about that, but it doesn't help.

The first field is "snapshot", it's true when current open file is snapshot. Looks obvious, that it should not be zeroed.

The second field is "started". It was converted from cpumask_t to cpumask_var_t (v2.6.28-4983-g4462344), in other words it was converted from cpumask to pointer on cpumask.

Currently the reference on "started" memory is lost after the first read from tracing_read_pipe and a proper object will never be freed.

The "started" is never dereferenced for trace_pipe, because trace_pipe can't have the TRACE_FILE_ANNOTATE options.

---
 include/linux/ftrace_event.h |   10 ++++++----
 kernel/trace/trace.c         |    1 +
 2 files changed, 7 insertions(+), 4 deletions(-)
include/linux/ftrace_event.h +6 -4
index f98ab06..120d57a 100644
@@ -78,6 +78,11 @@ struct trace_iterator {
 	/* trace_seq for __print_flags() and __print_symbolic() etc. */
 	struct trace_seq	tmp_seq;
 
+	cpumask_var_t		started;
+
+	/* it's true when current open file is snapshot */
+	bool			snapshot;
+
 	/* The below is zeroed out in pipe_read */
 	struct trace_seq	seq;
 	struct trace_entry	*ent;
@@ -90,10 +95,7 @@ struct trace_iterator {
 	loff_t			pos;
 	long			idx;
 
-	cpumask_var_t		started;
-
-	/* it's true when current open file is snapshot */
-	bool			snapshot;
+	/* All new field here will be zeroed out in pipe_read */
 };
 
 enum trace_iter_flags {
kernel/trace/trace.c +1 -0
index 882ec1d..f5b35a5 100644
@@ -4151,6 +4151,7 @@ waitagain:
 	memset(&iter->seq, 0,
 	       sizeof(struct trace_iterator) -
 	       offsetof(struct trace_iterator, seq));
+	cpumask_clear(iter->started);
 	iter->pos = -1;
 
 	trace_event_read_lock();
Signature · 1 line
1.7.10.4
author maintainer
#2 lore raw

PATCH 2/4 tracing: Fix trace_dump_stack() proto when CONFIG_TRACING is not set

In reply to Steven Rostedt

From: Dhaval Giani <[email protected]>

When CONFIG_TRACING is not enabled, the stub prototype for trace_dump_stack() is incorrect. It has (void) when it should be (int).

---
 include/linux/kernel.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/kernel.h +1 -1
index 3bef14c..482ad2d 100644
@@ -629,7 +629,7 @@ extern void ftrace_dump(enum ftrace_dump_mode oops_dump_mode);
 static inline void tracing_start(void) { }
 static inline void tracing_stop(void) { }
 static inline void ftrace_off_permanent(void) { }
-static inline void trace_dump_stack(void) { }
+static inline void trace_dump_stack(int skip) { }
 
 static inline void tracing_on(void) { }
 static inline void tracing_off(void) { }
Signature · 1 line
1.7.10.4
author maintainer
#3 lore raw

PATCH 3/4 tracing: Make TRACE_ITER_STOP_ON_FREE stop the correct buffer

In reply to Steven Rostedt

From: Alexander Z Lam <[email protected]>

Releasing the free_buffer file in an instance causes the global buffer to be stopped when TRACE_ITER_STOP_ON_FREE is enabled. Operate on the correct buffer.

Cc Vaibhav Nagarnaik <[email protected]>
Cc David Sharp <[email protected]>
Cc Alexander Z Lam <[email protected]>
---
 kernel/trace/trace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace.c +1 -1
index f5b35a5..531c9e6 100644
@@ -4469,7 +4469,7 @@ tracing_free_buffer_release(struct inode *inode, struct file *filp)
 
 	/* disable tracing ? */
 	if (trace_flags & TRACE_ITER_STOP_ON_FREE)
-		tracing_off();
+		tracer_tracing_off(tr);
 	/* resize the ring buffer to 0 */
 	tracing_resize_ring_buffer(tr, 0, RING_BUFFER_ALL_CPUS);
 
Signature · 1 line
1.7.10.4
author maintainer
#4 lore raw

PATCH 4/4 tracing: Fix reset of time stamps during trace_clock changes

In reply to Steven Rostedt

From: Alexander Z Lam <[email protected]>

Fixed two issues with changing the timestamp clock with trace_clock:

- The global buffer was reset on instance clock changes. Change this to pass the correct per-instance buffer
- ftrace_now() is used to set buf->time_start in tracing_reset_online_cpus(). This was incorrect because ftrace_now() used the global buffer's clock to return the current time. Change this to use buffer_ftrace_now() which returns the current time for the correct per-instance buffer.

Also removed tracing_reset_current() because it is not used anywhere

Cc Vaibhav Nagarnaik <[email protected]>
Cc David Sharp <[email protected]>
Cc Alexander Z Lam <[email protected]>
---
 kernel/trace/trace.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
kernel/trace/trace.c +12 -12
index 531c9e6..496f94d 100644
@@ -243,20 +243,25 @@ int filter_current_check_discard(struct ring_buffer *buffer,
 }
 EXPORT_SYMBOL_GPL(filter_current_check_discard);
 
-cycle_t ftrace_now(int cpu)
+cycle_t buffer_ftrace_now(struct trace_buffer *buf, int cpu)
 {
 	u64 ts;
 
 	/* Early boot up does not have a buffer yet */
-	if (!global_trace.trace_buffer.buffer)
+	if (!buf->buffer)
 		return trace_clock_local();
 
-	ts = ring_buffer_time_stamp(global_trace.trace_buffer.buffer, cpu);
-	ring_buffer_normalize_time_stamp(global_trace.trace_buffer.buffer, cpu, &ts);
+	ts = ring_buffer_time_stamp(buf->buffer, cpu);
+	ring_buffer_normalize_time_stamp(buf->buffer, cpu, &ts);
 
 	return ts;
 }
 
+cycle_t ftrace_now(int cpu)
+{
+	return buffer_ftrace_now(&global_trace.trace_buffer, cpu);
+}
+
 /**
  * tracing_is_enabled - Show if global_trace has been disabled
  *
@@ -1211,7 +1216,7 @@ void tracing_reset_online_cpus(struct trace_buffer *buf)
 	/* Make sure all commits have finished */
 	synchronize_sched();
 
-	buf->time_start = ftrace_now(buf->cpu);
+	buf->time_start = buffer_ftrace_now(buf, buf->cpu);
 
 	for_each_online_cpu(cpu)
 		ring_buffer_reset_cpu(buffer, cpu);
@@ -1219,11 +1224,6 @@ void tracing_reset_online_cpus(struct trace_buffer *buf)
 	ring_buffer_record_enable(buffer);
 }
 
-void tracing_reset_current(int cpu)
-{
-	tracing_reset(&global_trace.trace_buffer, cpu);
-}
-
 /* Must have trace_types_lock held */
 void tracing_reset_all_online_cpus(void)
 {
@@ -4634,12 +4634,12 @@ static ssize_t tracing_clock_write(struct file *filp, const char __user *ubuf,
 	 * New clock may not be consistent with the previous clock.
 	 * Reset the buffer so that it doesn't have incomparable timestamps.
 	 */
-	tracing_reset_online_cpus(&global_trace.trace_buffer);
+	tracing_reset_online_cpus(&tr->trace_buffer);
 
 #ifdef CONFIG_TRACER_MAX_TRACE
 	if (tr->flags & TRACE_ARRAY_FL_GLOBAL && tr->max_buffer.buffer)
 		ring_buffer_set_clock(tr->max_buffer.buffer, trace_clocks[i].func);
-	tracing_reset_online_cpus(&global_trace.max_buffer);
+	tracing_reset_online_cpus(&tr->max_buffer);
 #endif
 
 	mutex_unlock(&trace_types_lock);
Signature · 1 line
1.7.10.4
author maintainer
#5 lore raw

Re: [for-next-3.11][PATCH 4/4] tracing: Fix reset of time stamps during trace_clock changes

Greg,

I'm testing the backports, and I found that this patch depends on commit 10246fa35d4ffdfe472185d4cbf9c2dfd9a9f023 "tracing: Use flag buffer_disabled for irqsoff tracer"

Please add that to 3.10 before applying this fix.

Thanks!

-- Steve

Quoted 97 lines · Steven Rostedt

plain text document attachment (0004-tracing-Fix-reset-of-time-stamps-during-trace_clock-.patch)
From: Alexander Z Lam <[email protected]>

Fixed two issues with changing the timestamp clock with trace_clock:

- The global buffer was reset on instance clock changes. Change this to pass the correct per-instance buffer
- ftrace_now() is used to set buf->time_start in tracing_reset_online_cpus(). This was incorrect because ftrace_now() used the global buffer's clock to return the current time. Change this to use buffer_ftrace_now() which returns the current time for the correct per-instance buffer.

Also removed tracing_reset_current() because it is not used anywhere

Cc Vaibhav Nagarnaik <[email protected]>
Cc David Sharp <[email protected]>
Cc Alexander Z Lam <[email protected]>
---
 kernel/trace/trace.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
kernel/trace/trace.c +12 -12
index 531c9e6..496f94d 100644
@@ -243,20 +243,25 @@ int filter_current_check_discard(struct ring_buffer *buffer,
 }
 EXPORT_SYMBOL_GPL(filter_current_check_discard);
 
-cycle_t ftrace_now(int cpu)
+cycle_t buffer_ftrace_now(struct trace_buffer *buf, int cpu)
 {
 	u64 ts;
 
 	/* Early boot up does not have a buffer yet */
-	if (!global_trace.trace_buffer.buffer)
+	if (!buf->buffer)
 		return trace_clock_local();
 
-	ts = ring_buffer_time_stamp(global_trace.trace_buffer.buffer, cpu);
-	ring_buffer_normalize_time_stamp(global_trace.trace_buffer.buffer, cpu, &ts);
+	ts = ring_buffer_time_stamp(buf->buffer, cpu);
+	ring_buffer_normalize_time_stamp(buf->buffer, cpu, &ts);
 
 	return ts;
 }
 
+cycle_t ftrace_now(int cpu)
+{
+	return buffer_ftrace_now(&global_trace.trace_buffer, cpu);
+}
+
 /**
  * tracing_is_enabled - Show if global_trace has been disabled
  *
@@ -1211,7 +1216,7 @@ void tracing_reset_online_cpus(struct trace_buffer *buf)
 	/* Make sure all commits have finished */
 	synchronize_sched();
 
-	buf->time_start = ftrace_now(buf->cpu);
+	buf->time_start = buffer_ftrace_now(buf, buf->cpu);
 
 	for_each_online_cpu(cpu)
 		ring_buffer_reset_cpu(buffer, cpu);
@@ -1219,11 +1224,6 @@ void tracing_reset_online_cpus(struct trace_buffer *buf)
 	ring_buffer_record_enable(buffer);
 }
 
-void tracing_reset_current(int cpu)
-{
-	tracing_reset(&global_trace.trace_buffer, cpu);
-}
-
 /* Must have trace_types_lock held */
 void tracing_reset_all_online_cpus(void)
 {
@@ -4634,12 +4634,12 @@ static ssize_t tracing_clock_write(struct file *filp, const char __user *ubuf,
 	 * New clock may not be consistent with the previous clock.
 	 * Reset the buffer so that it doesn't have incomparable timestamps.
 	 */
-	tracing_reset_online_cpus(&global_trace.trace_buffer);
+	tracing_reset_online_cpus(&tr->trace_buffer);
 
 #ifdef CONFIG_TRACER_MAX_TRACE
 	if (tr->flags & TRACE_ARRAY_FL_GLOBAL && tr->max_buffer.buffer)
 		ring_buffer_set_clock(tr->max_buffer.buffer, trace_clocks[i].func);
-	tracing_reset_online_cpus(&global_trace.max_buffer);
+	tracing_reset_online_cpus(&tr->max_buffer);
 #endif
 
 	mutex_unlock(&trace_types_lock);
#6 lore raw

Re: [for-next-3.11][PATCH 4/4] tracing: Fix reset of time stamps during trace_clock changes

Quoted 7 lines · Steven Rostedt

Greg,

I'm testing the backports, and I found that this patch depends on commit 10246fa35d4ffdfe472185d4cbf9c2dfd9a9f023 "tracing: Use flag buffer_disabled for irqsoff tracer"

Please add that to 3.10 before applying this fix.

Oops, I sent out the "FAILED" email before seeing this, sorry about that. I've applied this patch, and now the original one applies as well, thanks.

greg k-h