linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] [GIT PULL][v3.6] tracing: updates
@ 2012-06-28 23:16 Steven Rostedt
  2012-06-28 23:16 ` [PATCH 1/4] tracing/selftest: Add a WARN_ON() if a tracer test fails Steven Rostedt
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Steven Rostedt @ 2012-06-28 23:16 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]


Ingo,

Please pull the latest tip/perf/core tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/perf/core

Head SHA1: a5fb833172eca69136e9ee1ada778e404086ab8a


Namhyung Kim (1):
      tracing/kvm: Use __print_hex() for kvm_emulate_insn tracepoint

Steven Rostedt (3):
      tracing/selftest: Add a WARN_ON() if a tracer test fails
      tracing: Remove NR_CPUS array from trace_iterator
      ring-buffer: Fix uninitialized read_stamp

----
 arch/x86/kvm/trace.h                 |   12 +-----------
 include/linux/ftrace_event.h         |    2 +-
 include/trace/ftrace.h               |    1 +
 kernel/trace/ring_buffer.c           |    4 ++++
 kernel/trace/trace.c                 |   29 ++++++++++++++++++++---------
 kernel/trace/trace.h                 |    8 ++++++++
 kernel/trace/trace_functions_graph.c |    2 +-
 7 files changed, 36 insertions(+), 22 deletions(-)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/4] tracing/selftest: Add a WARN_ON() if a tracer test fails
  2012-06-28 23:16 [PATCH 0/4] [GIT PULL][v3.6] tracing: updates Steven Rostedt
@ 2012-06-28 23:16 ` Steven Rostedt
  2012-06-28 23:16 ` [PATCH 2/4] tracing/kvm: Use __print_hex() for kvm_emulate_insn tracepoint Steven Rostedt
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2012-06-28 23:16 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Add a WARN_ON() output on test failures so that they are easier to detect
in automated tests. Although, the WARN_ON() will not print if the test
causes the system to crash, obviously.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 49249c2..748f640 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -830,6 +830,8 @@ int register_tracer(struct tracer *type)
 		current_trace = saved_tracer;
 		if (ret) {
 			printk(KERN_CONT "FAILED!\n");
+			/* Add the warning after printing 'FAILED' */
+			WARN_ON(1);
 			goto out;
 		}
 		/* Only reset on passing, to avoid touching corrupted buffers */
-- 
1.7.10



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/4] tracing/kvm: Use __print_hex() for kvm_emulate_insn tracepoint
  2012-06-28 23:16 [PATCH 0/4] [GIT PULL][v3.6] tracing: updates Steven Rostedt
  2012-06-28 23:16 ` [PATCH 1/4] tracing/selftest: Add a WARN_ON() if a tracer test fails Steven Rostedt
@ 2012-06-28 23:16 ` Steven Rostedt
  2012-06-28 23:16 ` [PATCH 3/4] tracing: Remove NR_CPUS array from trace_iterator Steven Rostedt
  2012-06-28 23:16 ` [PATCH 4/4] ring-buffer: Fix uninitialized read_stamp Steven Rostedt
  3 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2012-06-28 23:16 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker,
	Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Namhyung Kim, Avi Kivity, Namhyung Kim

[-- Attachment #1: Type: text/plain, Size: 3640 bytes --]

From: Namhyung Kim <namhyung.kim@lge.com>

The kvm_emulate_insn tracepoint used __print_insn()
for printing its instructions. However it makes the
format of the event hard to parse as it reveals TP
internals.

Fortunately, kernel provides __print_hex for almost
same purpose, we can use it instead of open coding
it. The user-space can be changed to parse it later.

That means raw kernel tracing will not be affected
by this change:

 # cd /sys/kernel/debug/tracing/
 # cat events/kvm/kvm_emulate_insn/format
 name: kvm_emulate_insn
 ID: 29
 format:
	...
 print fmt: "%x:%llx:%s (%s)%s", REC->csbase, REC->rip, __print_hex(REC->insn, REC->len), \
 __print_symbolic(REC->flags, { 0, "real" }, { (1 << 0) | (1 << 1), "vm16" }, \
 { (1 << 0), "prot16" }, { (1 << 0) | (1 << 2), "prot32" }, { (1 << 0) | (1 << 3), "prot64" }), \
 REC->failed ? " failed" : ""

 # echo 1 > events/kvm/kvm_emulate_insn/enable
 # cat trace
 # tracer: nop
 #
 # entries-in-buffer/entries-written: 2183/2183   #P:12
 #
 #                              _-----=> irqs-off
 #                             / _----=> need-resched
 #                            | / _---=> hardirq/softirq
 #                            || / _--=> preempt-depth
 #                            ||| /     delay
 #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
 #              | |       |   ||||       |         |
         qemu-kvm-1782  [002] ...1   140.931636: kvm_emulate_insn: 0:c102fa25:89 10 (prot32)
         qemu-kvm-1781  [004] ...1   140.931637: kvm_emulate_insn: 0:c102fa25:89 10 (prot32)

Link: http://lkml.kernel.org/n/tip-wfw6y3b9ugtey8snaow9nmg5@git.kernel.org
Link: http://lkml.kernel.org/r/1340757701-10711-2-git-send-email-namhyung@kernel.org

Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: kvm@vger.kernel.org
Acked-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kvm/trace.h   |   12 +-----------
 include/trace/ftrace.h |    1 +
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 911d264..62d02e3 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -710,16 +710,6 @@ TRACE_EVENT(kvm_skinit,
 		  __entry->rip, __entry->slb)
 );
 
-#define __print_insn(insn, ilen) ({		                 \
-	int i;							 \
-	const char *ret = p->buffer + p->len;			 \
-								 \
-	for (i = 0; i < ilen; ++i)				 \
-		trace_seq_printf(p, " %02x", insn[i]);		 \
-	trace_seq_printf(p, "%c", 0);				 \
-	ret;							 \
-	})
-
 #define KVM_EMUL_INSN_F_CR0_PE (1 << 0)
 #define KVM_EMUL_INSN_F_EFL_VM (1 << 1)
 #define KVM_EMUL_INSN_F_CS_D   (1 << 2)
@@ -786,7 +776,7 @@ TRACE_EVENT(kvm_emulate_insn,
 
 	TP_printk("%x:%llx:%s (%s)%s",
 		  __entry->csbase, __entry->rip,
-		  __print_insn(__entry->insn, __entry->len),
+		  __print_hex(__entry->insn, __entry->len),
 		  __print_symbolic(__entry->flags,
 				   kvm_trace_symbol_emul_flags),
 		  __entry->failed ? " failed" : ""
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 7697249..c6bc2fa 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -571,6 +571,7 @@ static inline void ftrace_test_probe_##call(void)			\
 
 #undef __print_flags
 #undef __print_symbolic
+#undef __print_hex
 #undef __get_dynamic_array
 #undef __get_str
 
-- 
1.7.10



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/4] tracing: Remove NR_CPUS array from trace_iterator
  2012-06-28 23:16 [PATCH 0/4] [GIT PULL][v3.6] tracing: updates Steven Rostedt
  2012-06-28 23:16 ` [PATCH 1/4] tracing/selftest: Add a WARN_ON() if a tracer test fails Steven Rostedt
  2012-06-28 23:16 ` [PATCH 2/4] tracing/kvm: Use __print_hex() for kvm_emulate_insn tracepoint Steven Rostedt
@ 2012-06-28 23:16 ` Steven Rostedt
  2012-06-28 23:16 ` [PATCH 4/4] ring-buffer: Fix uninitialized read_stamp Steven Rostedt
  3 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2012-06-28 23:16 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, Dave Jones

[-- Attachment #1: Type: text/plain, Size: 5724 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Replace the NR_CPUS array of buffer_iter from the trace_iterator
with an allocated array. This will just create an array of
possible CPUS instead of the max number specified.

The use of NR_CPUS in that array caused allocation failures for
machines that were tight on memory. This did not cause any failures
to the system itself (no crashes), but caused unnecessary failures
for reading the trace files.

Added a helper function called 'trace_buffer_iter()' that returns
the buffer_iter item or NULL if it is not defined or the array was
not allocated. Some routines do not require the array
(tracing_open_pipe() for one).

Reported-by: Dave Jones <davej@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace_event.h         |    2 +-
 kernel/trace/trace.c                 |   27 ++++++++++++++++++---------
 kernel/trace/trace.h                 |    8 ++++++++
 kernel/trace/trace_functions_graph.c |    2 +-
 4 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 1aff183..af961d6 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -65,7 +65,7 @@ struct trace_iterator {
 	void			*private;
 	int			cpu_file;
 	struct mutex		mutex;
-	struct ring_buffer_iter	*buffer_iter[NR_CPUS];
+	struct ring_buffer_iter	**buffer_iter;
 	unsigned long		iter_flags;
 
 	/* trace_seq for __print_flags() and __print_symbolic() etc. */
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 748f640..b2af14e 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1710,9 +1710,11 @@ EXPORT_SYMBOL_GPL(trace_vprintk);
 
 static void trace_iterator_increment(struct trace_iterator *iter)
 {
+	struct ring_buffer_iter *buf_iter = trace_buffer_iter(iter, iter->cpu);
+
 	iter->idx++;
-	if (iter->buffer_iter[iter->cpu])
-		ring_buffer_read(iter->buffer_iter[iter->cpu], NULL);
+	if (buf_iter)
+		ring_buffer_read(buf_iter, NULL);
 }
 
 static struct trace_entry *
@@ -1720,7 +1722,7 @@ peek_next_entry(struct trace_iterator *iter, int cpu, u64 *ts,
 		unsigned long *lost_events)
 {
 	struct ring_buffer_event *event;
-	struct ring_buffer_iter *buf_iter = iter->buffer_iter[cpu];
+	struct ring_buffer_iter *buf_iter = trace_buffer_iter(iter, cpu);
 
 	if (buf_iter)
 		event = ring_buffer_iter_peek(buf_iter, ts);
@@ -1858,10 +1860,10 @@ void tracing_iter_reset(struct trace_iterator *iter, int cpu)
 
 	tr->data[cpu]->skipped_entries = 0;
 
-	if (!iter->buffer_iter[cpu])
+	buf_iter = trace_buffer_iter(iter, cpu);
+	if (!buf_iter)
 		return;
 
-	buf_iter = iter->buffer_iter[cpu];
 	ring_buffer_iter_reset(buf_iter);
 
 	/*
@@ -2207,13 +2209,15 @@ static enum print_line_t print_bin_fmt(struct trace_iterator *iter)
 
 int trace_empty(struct trace_iterator *iter)
 {
+	struct ring_buffer_iter *buf_iter;
 	int cpu;
 
 	/* If we are looking at one CPU buffer, only check that one */
 	if (iter->cpu_file != TRACE_PIPE_ALL_CPU) {
 		cpu = iter->cpu_file;
-		if (iter->buffer_iter[cpu]) {
-			if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
+		buf_iter = trace_buffer_iter(iter, cpu);
+		if (buf_iter) {
+			if (!ring_buffer_iter_empty(buf_iter))
 				return 0;
 		} else {
 			if (!ring_buffer_empty_cpu(iter->tr->buffer, cpu))
@@ -2223,8 +2227,9 @@ int trace_empty(struct trace_iterator *iter)
 	}
 
 	for_each_tracing_cpu(cpu) {
-		if (iter->buffer_iter[cpu]) {
-			if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
+		buf_iter = trace_buffer_iter(iter, cpu);
+		if (buf_iter) {
+			if (!ring_buffer_iter_empty(buf_iter))
 				return 0;
 		} else {
 			if (!ring_buffer_empty_cpu(iter->tr->buffer, cpu))
@@ -2383,6 +2388,8 @@ __tracing_open(struct inode *inode, struct file *file)
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 
+	iter->buffer_iter = kzalloc(sizeof(*iter->buffer_iter) * num_possible_cpus(),
+				    GFP_KERNEL);
 	/*
 	 * We make a copy of the current tracer to avoid concurrent
 	 * changes on it while we are reading.
@@ -2443,6 +2450,7 @@ __tracing_open(struct inode *inode, struct file *file)
  fail:
 	mutex_unlock(&trace_types_lock);
 	kfree(iter->trace);
+	kfree(iter->buffer_iter);
 	seq_release_private(inode, file);
 	return ERR_PTR(-ENOMEM);
 }
@@ -2483,6 +2491,7 @@ static int tracing_release(struct inode *inode, struct file *file)
 	mutex_destroy(&iter->mutex);
 	free_cpumask_var(iter->started);
 	kfree(iter->trace);
+	kfree(iter->buffer_iter);
 	seq_release_private(inode, file);
 	return 0;
 }
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 5aec220..55e1f7f 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -317,6 +317,14 @@ struct tracer {
 
 #define TRACE_PIPE_ALL_CPU	-1
 
+static inline struct ring_buffer_iter *
+trace_buffer_iter(struct trace_iterator *iter, int cpu)
+{
+	if (iter->buffer_iter && iter->buffer_iter[cpu])
+		return iter->buffer_iter[cpu];
+	return NULL;
+}
+
 int tracer_init(struct tracer *t, struct trace_array *tr);
 int tracing_is_enabled(void);
 void trace_wake_up(void);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index a7d2a4c..ce27c8b 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -538,7 +538,7 @@ get_return_for_leaf(struct trace_iterator *iter,
 		next = &data->ret;
 	} else {
 
-		ring_iter = iter->buffer_iter[iter->cpu];
+		ring_iter = trace_buffer_iter(iter, iter->cpu);
 
 		/* First peek to compare current entry and the next one */
 		if (ring_iter)
-- 
1.7.10



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/4] ring-buffer: Fix uninitialized read_stamp
  2012-06-28 23:16 [PATCH 0/4] [GIT PULL][v3.6] tracing: updates Steven Rostedt
                   ` (2 preceding siblings ...)
  2012-06-28 23:16 ` [PATCH 3/4] tracing: Remove NR_CPUS array from trace_iterator Steven Rostedt
@ 2012-06-28 23:16 ` Steven Rostedt
  2012-07-02 16:16   ` Steven Rostedt
  3 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2012-06-28 23:16 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, David Sharp

[-- Attachment #1: Type: text/plain, Size: 2553 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The ring buffer reader page is used to swap a page from the writable
ring buffer. If the writer happens to be on that page, it ends up on the
reader page, but will simply move off of it, back into the writable ring
buffer as writes are added.

The time stamp passed back to the readers is stored in the cpu_buffer per
CPU descriptor. This stamp is updated when a swap of the reader page takes
place, and it reads the current stamp from the page taken from the writable
ring buffer. Everytime a writer goes to a new page, it updates the time stamp
of that page.

The problem happens if a reader reads a page from an empty per CPU ring buffer.
If the buffer is empty, the swap still takes place, placing the writer at the
start of the reader page. If at a later time, a write happens, it updates the
page's time stamp and continues. But the problem is that the read_stamp does
not get updated, because the page was already swapped.

The solution to this was to not swap the page if the ring buffer happens to
be empty. This also removes the side effect that the writes on the reader
page will not get updated because the writer never gets back on the reader
page without a swap. That is, if a read happens on an empty buffer, but then
no reads happen for a while. If a swap took place, and the writer were to start
writing a lot of data (function tracer), it will start overflowing the ring buffer
and overwrite the older data. But because the writer never goes back onto the
reader page, the data left on the reader page never gets overwritten. This
causes the reader to see really old data, followed by a jump to newer data.

Link: http://lkml.kernel.org/r/1340060577-9112-1-git-send-email-dhsharp@google.com
Google-Bug-Id: 6410455
Reported-by: David Sharp <dhsharp@google.com>
tested-by: David Sharp <dhsharp@google.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ring_buffer.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 1d0f6a8..82a3e0c 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3239,6 +3239,10 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	if (cpu_buffer->commit_page == cpu_buffer->reader_page)
 		goto out;
 
+	/* Don't bother swapping if the ring buffer is empty */
+	if (rb_num_of_entries(cpu_buffer) == 0)
+		goto out;
+
 	/*
 	 * Reset the reader page to size zero.
 	 */
-- 
1.7.10



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/4] ring-buffer: Fix uninitialized read_stamp
  2012-06-28 23:16 ` [PATCH 4/4] ring-buffer: Fix uninitialized read_stamp Steven Rostedt
@ 2012-07-02 16:16   ` Steven Rostedt
  2012-07-06  9:11     ` Ingo Molnar
  0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2012-07-02 16:16 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, David Sharp

On Thu, 2012-06-28 at 19:16 -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> The ring buffer reader page is used to swap a page from the writable
> ring buffer. If the writer happens to be on that page, it ends up on the
> reader page, but will simply move off of it, back into the writable ring
> buffer as writes are added.
> 
> The time stamp passed back to the readers is stored in the cpu_buffer per
> CPU descriptor. This stamp is updated when a swap of the reader page takes
> place, and it reads the current stamp from the page taken from the writable
> ring buffer. Everytime a writer goes to a new page, it updates the time stamp
> of that page.
> 
> The problem happens if a reader reads a page from an empty per CPU ring buffer.
> If the buffer is empty, the swap still takes place, placing the writer at the
> start of the reader page. If at a later time, a write happens, it updates the
> page's time stamp and continues. But the problem is that the read_stamp does
> not get updated, because the page was already swapped.
> 
> The solution to this was to not swap the page if the ring buffer happens to
> be empty. This also removes the side effect that the writes on the reader
> page will not get updated because the writer never gets back on the reader
> page without a swap. That is, if a read happens on an empty buffer, but then
> no reads happen for a while. If a swap took place, and the writer were to start
> writing a lot of data (function tracer), it will start overflowing the ring buffer
> and overwrite the older data. But because the writer never goes back onto the
> reader page, the data left on the reader page never gets overwritten. This
> causes the reader to see really old data, followed by a jump to newer data.
> 
> Link: http://lkml.kernel.org/r/1340060577-9112-1-git-send-email-dhsharp@google.com
> Google-Bug-Id: 6410455
> Reported-by: David Sharp <dhsharp@google.com>
> tested-by: David Sharp <dhsharp@google.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

I'm starting to consider that this patch should be in stable.

Ingo, should I push this to urgent?

-- Steve

> ---
>  kernel/trace/ring_buffer.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 1d0f6a8..82a3e0c 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -3239,6 +3239,10 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	if (cpu_buffer->commit_page == cpu_buffer->reader_page)
>  		goto out;
>  
> +	/* Don't bother swapping if the ring buffer is empty */
> +	if (rb_num_of_entries(cpu_buffer) == 0)
> +		goto out;
> +
>  	/*
>  	 * Reset the reader page to size zero.
>  	 */



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/4] ring-buffer: Fix uninitialized read_stamp
  2012-07-02 16:16   ` Steven Rostedt
@ 2012-07-06  9:11     ` Ingo Molnar
  2012-07-06  9:14       ` Ingo Molnar
  0 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2012-07-06  9:11 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, stable, Ingo Molnar, Andrew Morton,
	Frederic Weisbecker, David Sharp


* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 2012-06-28 at 19:16 -0400, Steven Rostedt wrote:
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > The ring buffer reader page is used to swap a page from the writable
> > ring buffer. If the writer happens to be on that page, it ends up on the
> > reader page, but will simply move off of it, back into the writable ring
> > buffer as writes are added.
> > 
> > The time stamp passed back to the readers is stored in the cpu_buffer per
> > CPU descriptor. This stamp is updated when a swap of the reader page takes
> > place, and it reads the current stamp from the page taken from the writable
> > ring buffer. Everytime a writer goes to a new page, it updates the time stamp
> > of that page.
> > 
> > The problem happens if a reader reads a page from an empty per CPU ring buffer.
> > If the buffer is empty, the swap still takes place, placing the writer at the
> > start of the reader page. If at a later time, a write happens, it updates the
> > page's time stamp and continues. But the problem is that the read_stamp does
> > not get updated, because the page was already swapped.
> > 
> > The solution to this was to not swap the page if the ring buffer happens to
> > be empty. This also removes the side effect that the writes on the reader
> > page will not get updated because the writer never gets back on the reader
> > page without a swap. That is, if a read happens on an empty buffer, but then
> > no reads happen for a while. If a swap took place, and the writer were to start
> > writing a lot of data (function tracer), it will start overflowing the ring buffer
> > and overwrite the older data. But because the writer never goes back onto the
> > reader page, the data left on the reader page never gets overwritten. This
> > causes the reader to see really old data, followed by a jump to newer data.
> > 
> > Link: http://lkml.kernel.org/r/1340060577-9112-1-git-send-email-dhsharp@google.com
> > Google-Bug-Id: 6410455
> > Reported-by: David Sharp <dhsharp@google.com>
> > tested-by: David Sharp <dhsharp@google.com>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> I'm starting to consider that this patch should be in stable.
> 
> Ingo, should I push this to urgent?

Yeah, probably makes sense to do so, especially as it's rather 
small.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/4] ring-buffer: Fix uninitialized read_stamp
  2012-07-06  9:11     ` Ingo Molnar
@ 2012-07-06  9:14       ` Ingo Molnar
  0 siblings, 0 replies; 8+ messages in thread
From: Ingo Molnar @ 2012-07-06  9:14 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, stable, Ingo Molnar, Andrew Morton,
	Frederic Weisbecker, David Sharp


* Ingo Molnar <mingo@kernel.org> wrote:

> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 2012-06-28 at 19:16 -0400, Steven Rostedt wrote:
> > > From: Steven Rostedt <srostedt@redhat.com>
> > > 
> > > The ring buffer reader page is used to swap a page from the writable
> > > ring buffer. If the writer happens to be on that page, it ends up on the
> > > reader page, but will simply move off of it, back into the writable ring
> > > buffer as writes are added.
> > > 
> > > The time stamp passed back to the readers is stored in the cpu_buffer per
> > > CPU descriptor. This stamp is updated when a swap of the reader page takes
> > > place, and it reads the current stamp from the page taken from the writable
> > > ring buffer. Everytime a writer goes to a new page, it updates the time stamp
> > > of that page.
> > > 
> > > The problem happens if a reader reads a page from an empty per CPU ring buffer.
> > > If the buffer is empty, the swap still takes place, placing the writer at the
> > > start of the reader page. If at a later time, a write happens, it updates the
> > > page's time stamp and continues. But the problem is that the read_stamp does
> > > not get updated, because the page was already swapped.
> > > 
> > > The solution to this was to not swap the page if the ring buffer happens to
> > > be empty. This also removes the side effect that the writes on the reader
> > > page will not get updated because the writer never gets back on the reader
> > > page without a swap. That is, if a read happens on an empty buffer, but then
> > > no reads happen for a while. If a swap took place, and the writer were to start
> > > writing a lot of data (function tracer), it will start overflowing the ring buffer
> > > and overwrite the older data. But because the writer never goes back onto the
> > > reader page, the data left on the reader page never gets overwritten. This
> > > causes the reader to see really old data, followed by a jump to newer data.
> > > 
> > > Link: http://lkml.kernel.org/r/1340060577-9112-1-git-send-email-dhsharp@google.com
> > > Google-Bug-Id: 6410455
> > > Reported-by: David Sharp <dhsharp@google.com>
> > > tested-by: David Sharp <dhsharp@google.com>
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > I'm starting to consider that this patch should be in stable.
> > 
> > Ingo, should I push this to urgent?
> 
> Yeah, probably makes sense to do so, especially as it's rather 
> small.

FYI, I have cherry picked it over into perf/urgent:

01c4359c155e ring-buffer: Fix uninitialized read_stamp


Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2012-07-06  9:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-28 23:16 [PATCH 0/4] [GIT PULL][v3.6] tracing: updates Steven Rostedt
2012-06-28 23:16 ` [PATCH 1/4] tracing/selftest: Add a WARN_ON() if a tracer test fails Steven Rostedt
2012-06-28 23:16 ` [PATCH 2/4] tracing/kvm: Use __print_hex() for kvm_emulate_insn tracepoint Steven Rostedt
2012-06-28 23:16 ` [PATCH 3/4] tracing: Remove NR_CPUS array from trace_iterator Steven Rostedt
2012-06-28 23:16 ` [PATCH 4/4] ring-buffer: Fix uninitialized read_stamp Steven Rostedt
2012-07-02 16:16   ` Steven Rostedt
2012-07-06  9:11     ` Ingo Molnar
2012-07-06  9:14       ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).