[PATCH v3 -tip 0/4] tracing: make a snapshot feature available from userspace

[PATCH v3 -tip 0/4] tracing: make a snapshot feature available from userspace

[Hiraku Toyooka]

Hi, Steven,

Thank you for your review.
I applied your review comments.

These patches depend on the next patch.

tracing: Add a resize function to make one buffer equivalent to another buffer
http://lkml.kernel.org/r/20121017025616.2627.91226.stgit@falsita

v2->v3:
 [1/4] tracing: add checks if tr->buffer is NULL in tracing_reset{_online_cpus}
        - (new patch)
 [2/4] tracing: replace static old_tracer with strcmp
        - (new patch)
 [3/4] tracing: make a snapshot feature available from userspace
        - changed snapshot file I/F (removed "snapshot_allocate")
        - changed CONFIG_TRACER_SNAPSHOT's location and description
        - changed an integer flag of __tracing_open() to bool
        - switched to use seq_read() directly for reading snapshot
 [4/4] tracing: add description of snapshot to Documentation/trace/ftrace.txt
        - updated documentation of the snapshot

ToDo:
 - adding "trace_snapshot" kernel parameter to allocate spare buffer on boot. 

(v1: https://lkml.org/lkml/2012/10/2/67)
(v2: https://lkml.org/lkml/2012/10/16/585)

---

Hiraku Toyooka (4):
      tracing: add checks if tr->buffer is NULL in tracing_reset{_online_cpus}
      tracing: replace static old_tracer with strcmp
      tracing: make a snapshot feature available from userspace
      tracing: add description of snapshot to Documentation/trace/ftrace.txt


 Documentation/trace/ftrace.txt |   83 +++++++++++++++++++++
 include/linux/ftrace_event.h   |    3 +
 kernel/trace/Kconfig           |   10 +++
 kernel/trace/trace.c           |  158 ++++++++++++++++++++++++++++++++++------
 kernel/trace/trace.h           |    1 
 5 files changed, 231 insertions(+), 24 deletions(-)

-- 
Hiraku TOYOOKA
Linux Technology Center
Yokohama Research Laboratory
Hitachi Ltd.

[PATCH v3 -tip 1/4] tracing: add checks if tr->buffer is NULL in tracing_reset{_online_cpus}

[Hiraku Toyooka]

max_tr->buffer could be NULL in the tracing_reset{_online_cpus}. In this
case, a NULL pointer dereference happens, so we should return immediately
from these functions.

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c705c7a..a8ce008 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -922,6 +922,9 @@ void tracing_reset(struct trace_array *tr, int cpu)
 {
 	struct ring_buffer *buffer = tr->buffer;
 
+	if (!buffer)
+		return;
+
 	ring_buffer_record_disable(buffer);
 
 	/* Make sure all commits have finished */
@@ -936,6 +939,9 @@ void tracing_reset_online_cpus(struct trace_array *tr)
 	struct ring_buffer *buffer = tr->buffer;
 	int cpu;
 
+	if (!buffer)
+		return;
+
 	ring_buffer_record_disable(buffer);
 
 	/* Make sure all commits have finished */

[PATCH v3 -tip 2/4] tracing: replace static old_tracer with strcmp

[Hiraku Toyooka]

Currently, read functions for trace buffer use static "old_tracer"
for detecting changes of current tracer. This is because we can
assume that these functions are used from only one file ("trace").

But we are adding snapshot feature for ftrace, then those functions
are called from two files. So we remove all static "old_tracer", and
replace those with string comparison between current and previous
tracers.

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a8ce008..8d05a44 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1948,7 +1948,6 @@ void tracing_iter_reset(struct trace_iterator *iter, int cpu)
 static void *s_start(struct seq_file *m, loff_t *pos)
 {
 	struct trace_iterator *iter = m->private;
-	static struct tracer *old_tracer;
 	int cpu_file = iter->cpu_file;
 	void *p = NULL;
 	loff_t l = 0;
@@ -1956,10 +1955,9 @@ static void *s_start(struct seq_file *m, loff_t *pos)
 
 	/* copy the tracer to avoid using a global lock all around */
 	mutex_lock(&trace_types_lock);
-	if (unlikely(old_tracer != current_trace && current_trace)) {
-		old_tracer = current_trace;
+	if (unlikely(current_trace &&
+		     strcmp(iter->trace->name, current_trace->name) != 0))
 		*iter->trace = *current_trace;
-	}
 	mutex_unlock(&trace_types_lock);
 
 	atomic_inc(&trace_record_cmdline_disabled);
@@ -3481,7 +3479,6 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
 		  size_t cnt, loff_t *ppos)
 {
 	struct trace_iterator *iter = filp->private_data;
-	static struct tracer *old_tracer;
 	ssize_t sret;
 
 	/* return any leftover data */
@@ -3493,10 +3490,9 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
 
 	/* copy the tracer to avoid using a global lock all around */
 	mutex_lock(&trace_types_lock);
-	if (unlikely(old_tracer != current_trace && current_trace)) {
-		old_tracer = current_trace;
+	if (unlikely(current_trace &&
+		     strcmp(iter->trace->name, current_trace->name) != 0))
 		*iter->trace = *current_trace;
-	}
 	mutex_unlock(&trace_types_lock);
 
 	/*
@@ -3652,7 +3648,6 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 		.ops		= &tracing_pipe_buf_ops,
 		.spd_release	= tracing_spd_release_pipe,
 	};
-	static struct tracer *old_tracer;
 	ssize_t ret;
 	size_t rem;
 	unsigned int i;
@@ -3662,10 +3657,9 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 
 	/* copy the tracer to avoid using a global lock all around */
 	mutex_lock(&trace_types_lock);
-	if (unlikely(old_tracer != current_trace && current_trace)) {
-		old_tracer = current_trace;
+	if (unlikely(current_trace &&
+		     strcmp(iter->trace->name, current_trace->name) != 0))
 		*iter->trace = *current_trace;
-	}
 	mutex_unlock(&trace_types_lock);
 
 	mutex_lock(&iter->mutex);

[PATCH v3 -tip 3/4] tracing: make a snapshot feature available from userspace

[Hiraku Toyooka]

Ftrace has a snapshot feature available from kernel space and
latency tracers (e.g. irqsoff) are using it. This patch enables
user applictions to take a snapshot via debugfs.

Add "snapshot" debugfs file in "tracing" directory.

  snapshot:
    This is used to take a snapshot and to read the output of the
    snapshot.

     # echo 1 > snapshot

    This will allocate the spare buffer for snapshot (if it is
    not allocated), and take a snapshot.

     # cat snapshot

    This will show contents of the snapshot.

     # echo 0 > snapshot

    This will free the snapshot if it is allocated.

    Any other positive values will clear the snapshot contents if
    the snapshot is allocated, or return EINVAL if it is not allocated.

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: David Sharp <dhsharp@google.com>
Cc: linux-kernel@vger.kernel.org
---
 include/linux/ftrace_event.h |    3 +
 kernel/trace/Kconfig         |   10 +++
 kernel/trace/trace.c         |  134 ++++++++++++++++++++++++++++++++++++++----
 kernel/trace/trace.h         |    1 
 4 files changed, 136 insertions(+), 12 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index a3d4895..9bebadd 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -84,6 +84,9 @@ struct trace_iterator {
 	long			idx;
 
 	cpumask_var_t		started;
+
+	/* it's true when current open file is snapshot */
+	bool			snapshot;
 };
 
 enum trace_iter_flags {
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 5d89335..82a8ff5 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -250,6 +250,16 @@ config FTRACE_SYSCALLS
 	help
 	  Basic tracer to catch the syscall entry and exit events.
 
+config TRACER_SNAPSHOT
+	bool "Create a snapshot trace buffer"
+	select TRACER_MAX_TRACE
+	help
+	  Allow tracing users to take snapshot of the current buffer using the
+	  ftrace interface, e.g.:
+
+	      echo 1 > /sys/kernel/debug/tracing/snapshot
+	      cat snapshot
+
 config TRACE_BRANCH_PROFILING
 	bool
 	select GENERIC_TRACER
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8d05a44..0e5abce 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -709,7 +709,7 @@ update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu)
 		return;
 
 	WARN_ON_ONCE(!irqs_disabled());
-	if (!current_trace->use_max_tr) {
+	if (!current_trace->allocated_snapshot) {
 		WARN_ON_ONCE(1);
 		return;
 	}
@@ -739,7 +739,7 @@ update_max_tr_single(struct trace_array *tr, struct task_struct *tsk, int cpu)
 		return;
 
 	WARN_ON_ONCE(!irqs_disabled());
-	if (!current_trace->use_max_tr) {
+	if (!current_trace->allocated_snapshot) {
 		WARN_ON_ONCE(1);
 		return;
 	}
@@ -1960,7 +1960,11 @@ static void *s_start(struct seq_file *m, loff_t *pos)
 		*iter->trace = *current_trace;
 	mutex_unlock(&trace_types_lock);
 
-	atomic_inc(&trace_record_cmdline_disabled);
+	if (iter->snapshot && iter->trace->use_max_tr)
+		return ERR_PTR(-EBUSY);
+
+	if (!iter->snapshot)
+		atomic_inc(&trace_record_cmdline_disabled);
 
 	if (*pos != iter->pos) {
 		iter->ent = NULL;
@@ -1999,7 +2003,11 @@ static void s_stop(struct seq_file *m, void *p)
 {
 	struct trace_iterator *iter = m->private;
 
-	atomic_dec(&trace_record_cmdline_disabled);
+	if (iter->snapshot && iter->trace->use_max_tr)
+		return;
+
+	if (!iter->snapshot)
+		atomic_dec(&trace_record_cmdline_disabled);
 	trace_access_unlock(iter->cpu_file);
 	trace_event_read_unlock();
 }
@@ -2434,7 +2442,7 @@ static const struct seq_operations tracer_seq_ops = {
 };
 
 static struct trace_iterator *
-__tracing_open(struct inode *inode, struct file *file)
+__tracing_open(struct inode *inode, struct file *file, bool snapshot)
 {
 	long cpu_file = (long) inode->i_private;
 	struct trace_iterator *iter;
@@ -2467,10 +2475,11 @@ __tracing_open(struct inode *inode, struct file *file)
 	if (!zalloc_cpumask_var(&iter->started, GFP_KERNEL))
 		goto fail;
 
-	if (current_trace && current_trace->print_max)
+	if ((current_trace && current_trace->print_max) || snapshot)
 		iter->tr = &max_tr;
 	else
 		iter->tr = &global_trace;
+	iter->snapshot = snapshot;
 	iter->pos = -1;
 	mutex_init(&iter->mutex);
 	iter->cpu_file = cpu_file;
@@ -2487,8 +2496,9 @@ __tracing_open(struct inode *inode, struct file *file)
 	if (trace_clocks[trace_clock_id].in_ns)
 		iter->iter_flags |= TRACE_FILE_TIME_IN_NS;
 
-	/* stop the trace while dumping */
-	tracing_stop();
+	/* stop the trace while dumping if we are not opening "snapshot" */
+	if (!iter->snapshot)
+		tracing_stop();
 
 	if (iter->cpu_file == TRACE_PIPE_ALL_CPU) {
 		for_each_tracing_cpu(cpu) {
@@ -2551,8 +2561,9 @@ static int tracing_release(struct inode *inode, struct file *file)
 	if (iter->trace && iter->trace->close)
 		iter->trace->close(iter);
 
-	/* reenable tracing if it was previously enabled */
-	tracing_start();
+	if (!iter->snapshot)
+		/* reenable tracing if it was previously enabled */
+		tracing_start();
 	mutex_unlock(&trace_types_lock);
 
 	mutex_destroy(&iter->mutex);
@@ -2580,7 +2591,7 @@ static int tracing_open(struct inode *inode, struct file *file)
 	}
 
 	if (file->f_mode & FMODE_READ) {
-		iter = __tracing_open(inode, file);
+		iter = __tracing_open(inode, file, false);
 		if (IS_ERR(iter))
 			ret = PTR_ERR(iter);
 		else if (trace_flags & TRACE_ITER_LATENCY_FMT)
@@ -3211,7 +3222,7 @@ static int tracing_set_tracer(const char *buf)
 	trace_branch_disable();
 	if (current_trace && current_trace->reset)
 		current_trace->reset(tr);
-	if (current_trace && current_trace->use_max_tr) {
+	if (current_trace && current_trace->allocated_snapshot) {
 		/*
 		 * We don't free the ring buffer. instead, resize it because
 		 * The max_tr ring buffer has some state (e.g. ring->clock) and
@@ -3219,6 +3230,8 @@ static int tracing_set_tracer(const char *buf)
 		 */
 		ring_buffer_resize(max_tr.buffer, 1, RING_BUFFER_ALL_CPUS);
 		set_buffer_entries(&max_tr, 1);
+		tracing_reset_online_cpus(&max_tr);
+		current_trace->allocated_snapshot = false;
 	}
 	destroy_trace_option_files(topts);
 
@@ -3231,6 +3244,7 @@ static int tracing_set_tracer(const char *buf)
 						   RING_BUFFER_ALL_CPUS);
 		if (ret < 0)
 			goto out;
+		t->allocated_snapshot = true;
 	}
 
 	if (t->init) {
@@ -4052,6 +4066,87 @@ static int tracing_clock_open(struct inode *inode, struct file *file)
 	return single_open(file, tracing_clock_show, NULL);
 }
 
+#ifdef CONFIG_TRACER_SNAPSHOT
+static int tracing_snapshot_open(struct inode *inode, struct file *file)
+{
+	struct trace_iterator *iter;
+	int ret = 0;
+
+	if (file->f_mode & FMODE_READ) {
+		iter = __tracing_open(inode, file, true);
+		if (IS_ERR(iter))
+			ret = PTR_ERR(iter);
+	}
+	return ret;
+}
+
+static ssize_t
+tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
+		       loff_t *ppos)
+{
+	unsigned long val;
+	int ret;
+
+	ret = tracing_update_buffers();
+	if (ret < 0)
+		return ret;
+
+	ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&trace_types_lock);
+
+	if (current_trace && current_trace->use_max_tr) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	switch (val) {
+	case 0:
+		if (current_trace->allocated_snapshot) {
+			/* free spare buffer */
+			ring_buffer_resize(max_tr.buffer, 1,
+					   RING_BUFFER_ALL_CPUS);
+			set_buffer_entries(&max_tr, 1);
+			tracing_reset_online_cpus(&max_tr);
+			current_trace->allocated_snapshot = false;
+		}
+		break;
+	case 1:
+		if (!current_trace->allocated_snapshot) {
+			/* allocate spare buffer */
+			ret = resize_buffer_duplicate_size(&max_tr,
+					&global_trace, RING_BUFFER_ALL_CPUS);
+			if (ret < 0)
+				break;
+			current_trace->allocated_snapshot = true;
+		}
+
+		local_irq_disable();
+		/* Now, we're going to swap */
+		update_max_tr(&global_trace, current, smp_processor_id());
+		local_irq_enable();
+		break;
+	default:
+		if (current_trace->allocated_snapshot)
+			tracing_reset_online_cpus(&max_tr);
+		else
+			ret = -EINVAL;
+		break;
+	}
+
+	if (ret >= 0) {
+		*ppos += cnt;
+		ret = cnt;
+	}
+out:
+	mutex_unlock(&trace_types_lock);
+	return ret;
+}
+#endif /* CONFIG_TRACER_SNAPSHOT */
+
+
 static const struct file_operations tracing_max_lat_fops = {
 	.open		= tracing_open_generic,
 	.read		= tracing_max_lat_read,
@@ -4108,6 +4203,16 @@ static const struct file_operations trace_clock_fops = {
 	.write		= tracing_clock_write,
 };
 
+#ifdef CONFIG_TRACER_SNAPSHOT
+static const struct file_operations snapshot_fops = {
+	.open		= tracing_snapshot_open,
+	.read		= seq_read,
+	.write		= tracing_snapshot_write,
+	.llseek		= tracing_seek,
+	.release	= tracing_release,
+};
+#endif /* CONFIG_TRACER_SNAPSHOT */
+
 struct ftrace_buffer_info {
 	struct trace_array	*tr;
 	void			*spare;
@@ -4899,6 +5004,11 @@ static __init int tracer_init_debugfs(void)
 			&ftrace_update_tot_cnt, &tracing_dyn_info_fops);
 #endif
 
+#ifdef CONFIG_TRACER_SNAPSHOT
+	trace_create_file("snapshot", 0644, d_tracer,
+			  (void *) TRACE_PIPE_ALL_CPU, &snapshot_fops);
+#endif
+
 	create_trace_options_dir();
 
 	for_each_tracing_cpu(cpu)
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index c75d798..512afe5 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -287,6 +287,7 @@ struct tracer {
 	struct tracer_flags	*flags;
 	bool			print_max;
 	bool			use_max_tr;
+	bool			allocated_snapshot;
 };
 
 

[PATCH v3 -tip 4/4] tracing: add description of snapshot to Documentation/trace/ftrace.txt

[Hiraku Toyooka]

This patch adds snapshot description in ftrace documentation.
This description includes what the snapshot is and how to use it.

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Rob Landley <rob@landley.net>
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/trace/ftrace.txt |   83 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt
index 6f51fed..53d6a3c 100644
--- a/Documentation/trace/ftrace.txt
+++ b/Documentation/trace/ftrace.txt
@@ -1842,6 +1842,89 @@ an error.
  # cat buffer_size_kb
 85
 
+Snapshot
+--------
+CONFIG_TRACER_SNAPSHOT makes a generic snapshot feature
+available to all non latency tracers. (Latency tracers which
+record max latency, such as "irqsoff" or "wakeup", can't use
+this feature, since those are already using the snapshot
+mechanism internally.)
+
+Snapshot preserves a current trace buffer at a particular point
+in time without stopping tracing. Ftrace swaps the current
+buffer with a spare buffer, and tracing continues in the new
+current (=previous spare) buffer.
+
+The following debugfs files in "tracing" are related to this
+feature:
+
+  snapshot:
+
+	This is used to take a snapshot and to read the output
+	of the snapshot. Echo 1 into this file to allocate a
+	spare buffer and to take a snapshot (swap), then read
+	the snapshot from this file in the same format as
+	"trace" (described above in the section "The File
+	System"). Both reads snapshot and tracing are executable
+	in parallel. When the spare buffer is allocated, echoing
+	0 frees it, and echoing else (positive) values clear the
+	snapshot contents.
+	More details are shown in the table below.
+
+	status\input  |     0      |     1      |    else    |
+	--------------+------------+------------+------------+
+	not allocated |(do nothing)| alloc+swap |   EINVAL   |
+	--------------+------------+------------+------------+
+	allocated     |    free    |    swap    |   clear    |
+	--------------+------------+------------+------------+
+
+Here is an example of using the snapshot feature.
+
+ # echo 1 > events/sched/enable
+ # echo 1 > snapshot
+ # cat snapshot
+# tracer: nop
+#
+# entries-in-buffer/entries-written: 71/71   #P:8
+#
+#                              _-----=> irqs-off
+#                             / _----=> need-resched
+#                            | / _---=> hardirq/softirq
+#                            || / _--=> preempt-depth
+#                            ||| /     delay
+#           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
+#              | |       |   ||||       |         |
+          <idle>-0     [005] d...  2440.603828: sched_switch: prev_comm=swapper/5 prev_pid=0 prev_prio=120 prev_state=R ==> next_comm=snapshot-test-2 next_pid=2242 next_prio=120
+           sleep-2242  [005] d...  2440.603846: sched_switch: prev_comm=snapshot-test-2 prev_pid=2242 prev_prio=120 prev_state=R ==> next_comm=kworker/5:1 next_pid=60 next_prio=120
+[...]
+          <idle>-0     [002] d...  2440.707230: sched_switch: prev_comm=swapper/2 prev_pid=0 prev_prio=120 prev_state=R ==> next_comm=snapshot-test-2 next_pid=2229 next_prio=120
+
+ # cat trace
+# tracer: nop
+#
+# entries-in-buffer/entries-written: 77/77   #P:8
+#
+#                              _-----=> irqs-off
+#                             / _----=> need-resched
+#                            | / _---=> hardirq/softirq
+#                            || / _--=> preempt-depth
+#                            ||| /     delay
+#           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
+#              | |       |   ||||       |         |
+          <idle>-0     [007] d...  2440.707395: sched_switch: prev_comm=swapper/7 prev_pid=0 prev_prio=120 prev_state=R ==> next_comm=snapshot-test-2 next_pid=2243 next_prio=120
+ snapshot-test-2-2229  [002] d...  2440.707438: sched_switch: prev_comm=snapshot-test-2 prev_pid=2229 prev_prio=120 prev_state=S ==> next_comm=swapper/2 next_pid=0 next_prio=120
+[...]
+
+
+If you try to use this snapshot feature when current tracer is
+one of the latency tracers, you will get the following results.
+
+ # echo wakeup > current_tracer
+ # echo 1 > snapshot
+bash: echo: write error: Device or resource busy
+ # cat snapshot
+cat: snapshot: Device or resource busy
+
 -----------
 
 More details can be found in the source code, in the

Re: [PATCH v3 -tip 2/4] tracing: replace static old_tracer with strcmp

[Steven Rostedt]

On Wed, 2012-12-19 at 16:02 +0900, Hiraku Toyooka wrote:
> Currently, read functions for trace buffer use static "old_tracer"
> for detecting changes of current tracer. This is because we can
> assume that these functions are used from only one file ("trace").
> 
> But we are adding snapshot feature for ftrace, then those functions
> are called from two files. So we remove all static "old_tracer", and
> replace those with string comparison between current and previous
> tracers.

I reworded what you wrote:

---
Currently the trace buffer read functions use a static variable
"old_tracer" for detecting if the current tracer changes. This was
suitable for a single trace file ("trace"), but to add a snapshot
feature that will use the same function for its file, a check against
a static variable is not sufficient.

To use the output functions for two different files, instead of storing
the current tracer in a static variable, as the trace iterator
descriptor
contains a pointer to the original current tracer's name, that pointer
can now be used to check if the current tracer has changed between
different reads of the trace file.
---

And I also realized a more efficient approach.

> 
> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/trace/trace.c |   18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index a8ce008..8d05a44 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1948,7 +1948,6 @@ void tracing_iter_reset(struct trace_iterator *iter, int cpu)
>  static void *s_start(struct seq_file *m, loff_t *pos)
>  {
>  	struct trace_iterator *iter = m->private;
> -	static struct tracer *old_tracer;
>  	int cpu_file = iter->cpu_file;
>  	void *p = NULL;
>  	loff_t l = 0;
> @@ -1956,10 +1955,9 @@ static void *s_start(struct seq_file *m, loff_t *pos)
>  
>  	/* copy the tracer to avoid using a global lock all around */
>  	mutex_lock(&trace_types_lock);
> -	if (unlikely(old_tracer != current_trace && current_trace)) {
> -		old_tracer = current_trace;
> +	if (unlikely(current_trace &&
> +		     strcmp(iter->trace->name, current_trace->name) != 0))
>  		*iter->trace = *current_trace;

As iter->trace is a copy of current_trace, it points to everything that
the current_trace pointed to. As the current_trace->name is a pointer to
a const char string that never changes, we don't need to do the strcmp()
you can simply do a direct comparison:

	if (unlikely(current_trace && iter->trace->name != current_trace->name)) {

I would add a comment about that too:
	/*
	 * iter->trace is a copy of current_trace, the pointer to the
	 * name may be used instead of a strcmp(), as iter->trace->name
	 * will point to the same string as current_trace->name.
	 */

-- Steve


> -	}
>  	mutex_unlock(&trace_types_lock);
>  
>  	atomic_inc(&trace_record_cmdline_disabled);
> @@ -3481,7 +3479,6 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
>  		  size_t cnt, loff_t *ppos)
>  {
>  	struct trace_iterator *iter = filp->private_data;
> -	static struct tracer *old_tracer;
>  	ssize_t sret;
>  
>  	/* return any leftover data */
> @@ -3493,10 +3490,9 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
>  
>  	/* copy the tracer to avoid using a global lock all around */
>  	mutex_lock(&trace_types_lock);
> -	if (unlikely(old_tracer != current_trace && current_trace)) {
> -		old_tracer = current_trace;
> +	if (unlikely(current_trace &&
> +		     strcmp(iter->trace->name, current_trace->name) != 0))
>  		*iter->trace = *current_trace;
> -	}
>  	mutex_unlock(&trace_types_lock);
>  
>  	/*
> @@ -3652,7 +3648,6 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
>  		.ops		= &tracing_pipe_buf_ops,
>  		.spd_release	= tracing_spd_release_pipe,
>  	};
> -	static struct tracer *old_tracer;
>  	ssize_t ret;
>  	size_t rem;
>  	unsigned int i;
> @@ -3662,10 +3657,9 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
>  
>  	/* copy the tracer to avoid using a global lock all around */
>  	mutex_lock(&trace_types_lock);
> -	if (unlikely(old_tracer != current_trace && current_trace)) {
> -		old_tracer = current_trace;
> +	if (unlikely(current_trace &&
> +		     strcmp(iter->trace->name, current_trace->name) != 0))
>  		*iter->trace = *current_trace;
> -	}
>  	mutex_unlock(&trace_types_lock);
>  
>  	mutex_lock(&iter->mutex);

Re: [PATCH v3 -tip 2/4] tracing: replace static old_tracer with strcmp

[Hiraku Toyooka]

Hi,

(12/21/2012 12:04 PM), Steven Rostedt wrote:
> On Wed, 2012-12-19 at 16:02 +0900, Hiraku Toyooka wrote:
>> Currently, read functions for trace buffer use static "old_tracer"
>> for detecting changes of current tracer. This is because we can
>> assume that these functions are used from only one file ("trace").
>>
>> But we are adding snapshot feature for ftrace, then those functions
>> are called from two files. So we remove all static "old_tracer", and
>> replace those with string comparison between current and previous
>> tracers.
>
> I reworded what you wrote:
>

Thank you very much.

> ---
> Currently the trace buffer read functions use a static variable
> "old_tracer" for detecting if the current tracer changes. This was
> suitable for a single trace file ("trace"), but to add a snapshot
> feature that will use the same function for its file, a check against
> a static variable is not sufficient.
>
> To use the output functions for two different files, instead of storing
> the current tracer in a static variable, as the trace iterator
> descriptor
> contains a pointer to the original current tracer's name, that pointer
> can now be used to check if the current tracer has changed between
> different reads of the trace file.
> ---
>
> And I also realized a more efficient approach.
>
>>
>> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   kernel/trace/trace.c |   18 ++++++------------
>>   1 file changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index a8ce008..8d05a44 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -1948,7 +1948,6 @@ void tracing_iter_reset(struct trace_iterator *iter, int cpu)
>>   static void *s_start(struct seq_file *m, loff_t *pos)
>>   {
>>   	struct trace_iterator *iter = m->private;
>> -	static struct tracer *old_tracer;
>>   	int cpu_file = iter->cpu_file;
>>   	void *p = NULL;
>>   	loff_t l = 0;
>> @@ -1956,10 +1955,9 @@ static void *s_start(struct seq_file *m, loff_t *pos)
>>
>>   	/* copy the tracer to avoid using a global lock all around */
>>   	mutex_lock(&trace_types_lock);
>> -	if (unlikely(old_tracer != current_trace && current_trace)) {
>> -		old_tracer = current_trace;
>> +	if (unlikely(current_trace &&
>> +		     strcmp(iter->trace->name, current_trace->name) != 0))
>>   		*iter->trace = *current_trace;
>
> As iter->trace is a copy of current_trace, it points to everything that
> the current_trace pointed to. As the current_trace->name is a pointer to
> a const char string that never changes, we don't need to do the strcmp()
> you can simply do a direct comparison:
>
> 	if (unlikely(current_trace && iter->trace->name != current_trace->name)) {
>

Yes. I'll update my patch to use this efficient way.

> I would add a comment about that too:
> 	/*
> 	 * iter->trace is a copy of current_trace, the pointer to the
> 	 * name may be used instead of a strcmp(), as iter->trace->name
> 	 * will point to the same string as current_trace->name.
> 	 */

OK. I'll include this comment in my patch.


Thanks,
Hiraku Toyooka

[tip:perf/core] tracing: Add checks if tr-> buffer is NULL in tracing_reset{_online_cpus}

[tip-bot for Hiraku Toyooka]

Commit-ID:  a54164114b96b4693b42cdb553260eec41ea4393
Gitweb:     http://git.kernel.org/tip/a54164114b96b4693b42cdb553260eec41ea4393
Author:     Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
AuthorDate: Wed, 19 Dec 2012 16:02:34 +0900
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Mon, 21 Jan 2013 13:22:32 -0500

tracing: Add checks if tr->buffer is NULL in tracing_reset{_online_cpus}

max_tr->buffer could be NULL in the tracing_reset{_online_cpus}. In this
case, a NULL pointer dereference happens, so we should return immediately
from these functions.

Note, the current code does not call tracing_reset*() with max_tr when
its buffer is NULL, but future code will. This patch is needed to prevent
the future code from crashing.

Link: http://lkml.kernel.org/r/20121219070234.31200.93863.stgit@liselsia

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index f8b7c62..72b171b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -922,6 +922,9 @@ void tracing_reset(struct trace_array *tr, int cpu)
 {
 	struct ring_buffer *buffer = tr->buffer;
 
+	if (!buffer)
+		return;
+
 	ring_buffer_record_disable(buffer);
 
 	/* Make sure all commits have finished */
@@ -936,6 +939,9 @@ void tracing_reset_online_cpus(struct trace_array *tr)
 	struct ring_buffer *buffer = tr->buffer;
 	int cpu;
 
+	if (!buffer)
+		return;
+
 	ring_buffer_record_disable(buffer);
 
 	/* Make sure all commits have finished */