Skip to contentSkip to conversation

ftrace updates for tip

PATCH In review Steven Rostedt 6 participants 12 messages Dec 2, 2008 – Dec 3, 2008

Subsystems (4)
x86 Tracing ftrace Tracing ring buffer
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover ftrace updates for tip

Ingo,

Here's some more ftrace updates.

The first should help fix the bug in lockdep.

The next are changes to the ring buffer to get ready to have it work with splice.

The last are some clean ups for function graph.

The following patches are in:

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

branch: tip/devel

Steven Rostedt (5):
ftrace: replace raw_local_irq_save with local_irq_save
ring-buffer: move some metadata into buffer page
ring-buffer: read page interface
ftrace: clean up function graph asm
ftrace: have function graph use mcount caller address

----

 arch/x86/kernel/entry_32.S  |    1 +
 arch/x86/kernel/entry_64.S  |    1 +
 arch/x86/kernel/ftrace.c    |   20 +---
 include/linux/ring_buffer.h |    5 +
 kernel/lockdep.c            |    1 +
 kernel/trace/ring_buffer.c  |  229 ++++++++++++++++++++++++++++++++++++++-----
 kernel/trace/trace.c        |   12 +-
 kernel/trace/trace_branch.c |    4 +-
 kernel/trace/trace_stack.c  |    8 +-
 9 files changed, 226 insertions(+), 55 deletions(-)
author maintainer
#10 lore raw

PATCH 5/5 ftrace: have function graph use mcount caller address

From: Steven Rostedt <[email protected]>

Impact: consistency change for function graph

This patch makes function graph record the mcount caller address the same way the function tracer does.

Signed-off-by Steven Rostedt <[email protected]>
---
 arch/x86/kernel/entry_32.S |    1 +
 arch/x86/kernel/entry_64.S |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)
arch/x86/kernel/entry_32.S +1 -0
index f484e9a..3497321 100644
@@ -1027,6 +1027,7 @@ ENTRY(ftrace_graph_caller)
 	pushl %edx
 	movl 0xc(%esp), %edx
 	lea 0x4(%ebp), %eax
+	subl $MCOUNT_INSN_SIZE, %edx
 	call prepare_ftrace_return
 	popl %edx
 	popl %ecx
arch/x86/kernel/entry_64.S +1 -0
index c433d86..47aa5f0 100644
@@ -172,6 +172,7 @@ ENTRY(ftrace_graph_caller)
 
 	leaq 8(%rbp), %rdi
 	movq 0x38(%rsp), %rsi
+	subq $MCOUNT_INSN_SIZE, %rsi
 
 	call	prepare_ftrace_return
Signature · 3 lines
1.5.6.5

--
author maintainer
#1 lore raw

PATCH 1/5 ftrace: replace raw_local_irq_save with local_irq_save

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: fix for lockdep and ftrace

The raw_local_irq_save/restore confuses lockdep. This patch converts them to the local_irq_save/restore variants.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/lockdep.c            |    1 +
 kernel/trace/trace.c        |   12 ++++++------
 kernel/trace/trace_branch.c |    4 ++--
 kernel/trace/trace_stack.c  |    8 ++++----
 4 files changed, 13 insertions(+), 12 deletions(-)
kernel/lockdep.c +1 -0
index 670aba8..1eb7c0b 100644
@@ -25,6 +25,7 @@
  * Thanks to Arjan van de Ven for coming up with the initial idea of
  * mapping lock dependencies runtime.
  */
+#define DISABLE_BRANCH_PROFILING
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/delay.h>
kernel/trace/trace.c +6 -6
index 91887a2..380de63 100644
@@ -1209,7 +1209,7 @@ void trace_graph_entry(struct ftrace_graph_ent *trace)
 	int cpu;
 	int pc;
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	data = tr->data[cpu];
 	disabled = atomic_inc_return(&data->disabled);

@@ -1218,7 +1218,7 @@ void trace_graph_entry(struct ftrace_graph_ent *trace)
 		__trace_graph_entry(tr, data, trace, flags, pc);
 	}
 	atomic_dec(&data->disabled);
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 }
 
 void trace_graph_return(struct ftrace_graph_ret *trace)

@@ -1230,7 +1230,7 @@ void trace_graph_return(struct ftrace_graph_ret *trace)
 	int cpu;
 	int pc;
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	data = tr->data[cpu];
 	disabled = atomic_inc_return(&data->disabled);

@@ -1239,7 +1239,7 @@ void trace_graph_return(struct ftrace_graph_ret *trace)
 		__trace_graph_return(tr, data, trace, flags, pc);
 	}
 	atomic_dec(&data->disabled);
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 

@@ -2645,7 +2645,7 @@ tracing_cpumask_write(struct file *filp, const char __user *ubuf,
 	if (err)
 		goto err_unlock;
 
-	raw_local_irq_disable();
+	local_irq_disable();
 	__raw_spin_lock(&ftrace_max_lock);
 	for_each_tracing_cpu(cpu) {
 		/*

@@ -2662,7 +2662,7 @@ tracing_cpumask_write(struct file *filp, const char __user *ubuf,
 		}
 	}
 	__raw_spin_unlock(&ftrace_max_lock);
-	raw_local_irq_enable();
+	local_irq_enable();
 
 	tracing_cpumask = tracing_cpumask_new;
kernel/trace/trace_branch.c +2 -2
index bc97275..6c00feb 100644
@@ -42,7 +42,7 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect)
 	if (unlikely(!tr))
 		return;
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	if (atomic_inc_return(&tr->data[cpu]->disabled) != 1)
 		goto out;

@@ -74,7 +74,7 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect)
 
  out:
 	atomic_dec(&tr->data[cpu]->disabled);
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 }
 
 static inline
kernel/trace/trace_stack.c +4 -4
index fde3be1..06a1611 100644
@@ -48,7 +48,7 @@ static inline void check_stack(void)
 	if (!object_is_on_stack(&this_size))
 		return;
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	__raw_spin_lock(&max_stack_lock);
 
 	/* a race could have already updated it */

@@ -96,7 +96,7 @@ static inline void check_stack(void)
 
  out:
 	__raw_spin_unlock(&max_stack_lock);
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 }
 
 static void

@@ -162,11 +162,11 @@ stack_max_size_write(struct file *filp, const char __user *ubuf,
 	if (ret < 0)
 		return ret;
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	__raw_spin_lock(&max_stack_lock);
 	*ptr = val;
 	__raw_spin_unlock(&max_stack_lock);
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 
 	return count;
 }
Signature · 3 lines
1.5.6.5

--
author maintainer
#3 lore raw

PATCH 2/5 ring-buffer: move some metadata into buffer page

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: get ready for splice changes

This patch moves the commit and timestamp into the beginning of each data page of the buffer. This change will allow the page to be moved to another location (disk, network, etc) and still have information in the page to be able to read it.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++++-------------------
 1 files changed, 36 insertions(+), 27 deletions(-)
kernel/trace/ring_buffer.c +36 -27
index e206951..8619c53 100644
@@ -195,20 +195,24 @@ void *ring_buffer_event_data(struct ring_buffer_event *event)
 #define TS_MASK		((1ULL << TS_SHIFT) - 1)
 #define TS_DELTA_TEST	(~TS_MASK)
 
-/*
- * This hack stolen from mm/slob.c.
- * We can store per page timing information in the page frame of the page.
- * Thanks to Peter Zijlstra for suggesting this idea.
- */
-struct buffer_page {
+struct buffer_data_page {
 	u64		 time_stamp;	/* page time stamp */
-	local_t		 write;		/* index for next write */
 	local_t		 commit;	/* write commited index */
+	unsigned char	 data[];	/* data of buffer page */
+};
+
+struct buffer_page {
+	local_t		 write;		/* index for next write */
 	unsigned	 read;		/* index for next read */
 	struct list_head list;		/* list of free pages */
-	void *page;			/* Actual data page */
+	struct buffer_data_page *page;	/* Actual data page */
 };
 
+static void rb_init_page(struct buffer_data_page *page)
+{
+	local_set(&page->commit, 0);
+}
+
 /*
  * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing
  * this issue out.

@@ -230,7 +234,7 @@ static inline int test_time_stamp(u64 delta)
 	return 0;
 }
 
-#define BUF_PAGE_SIZE PAGE_SIZE
+#define BUF_PAGE_SIZE (PAGE_SIZE - sizeof(struct buffer_data_page))
 
 /*
  * head_page == tail_page && head == tail then buffer is empty.

@@ -333,6 +337,7 @@ static int rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
 		if (!addr)
 			goto free_pages;
 		page->page = (void *)addr;
+		rb_init_page(page->page);
 	}
 
 	list_splice(&pages, head);

@@ -378,6 +383,7 @@ rb_allocate_cpu_buffer(struct ring_buffer *buffer, int cpu)
 	if (!addr)
 		goto fail_free_reader;
 	page->page = (void *)addr;
+	rb_init_page(page->page);
 
 	INIT_LIST_HEAD(&cpu_buffer->reader_page->list);
 

@@ -647,6 +653,7 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
 			if (!addr)
 				goto free_pages;
 			page->page = (void *)addr;
+			rb_init_page(page->page);
 		}
 	}
 

@@ -682,7 +689,7 @@ static inline int rb_null_event(struct ring_buffer_event *event)
 
 static inline void *__rb_page_index(struct buffer_page *page, unsigned index)
 {
-	return page->page + index;
+	return page->page->data + index;
 }
 
 static inline struct ring_buffer_event *

@@ -712,7 +719,7 @@ static inline unsigned rb_page_write(struct buffer_page *bpage)
 
 static inline unsigned rb_page_commit(struct buffer_page *bpage)
 {
-	return local_read(&bpage->commit);
+	return local_read(&bpage->page->commit);
 }
 
 /* Size is determined by what has been commited */

@@ -804,14 +811,15 @@ rb_set_commit_event(struct ring_buffer_per_cpu *cpu_buffer,
 		if (RB_WARN_ON(cpu_buffer,
 			  cpu_buffer->commit_page == cpu_buffer->tail_page))
 			return;
-		cpu_buffer->commit_page->commit =
+		cpu_buffer->commit_page->page->commit =
 			cpu_buffer->commit_page->write;
 		rb_inc_page(cpu_buffer, &cpu_buffer->commit_page);
-		cpu_buffer->write_stamp = cpu_buffer->commit_page->time_stamp;
+		cpu_buffer->write_stamp =
+			cpu_buffer->commit_page->page->time_stamp;
 	}
 
 	/* Now set the commit to the event's index */
-	local_set(&cpu_buffer->commit_page->commit, index);
+	local_set(&cpu_buffer->commit_page->page->commit, index);
 }
 
 static inline void

@@ -826,16 +834,17 @@ rb_set_commit_to_write(struct ring_buffer_per_cpu *cpu_buffer)
 	 * assign the commit to the tail.
 	 */
 	while (cpu_buffer->commit_page != cpu_buffer->tail_page) {
-		cpu_buffer->commit_page->commit =
+		cpu_buffer->commit_page->page->commit =
 			cpu_buffer->commit_page->write;
 		rb_inc_page(cpu_buffer, &cpu_buffer->commit_page);
-		cpu_buffer->write_stamp = cpu_buffer->commit_page->time_stamp;
+		cpu_buffer->write_stamp =
+			cpu_buffer->commit_page->page->time_stamp;
 		/* add barrier to keep gcc from optimizing too much */
 		barrier();
 	}
 	while (rb_commit_index(cpu_buffer) !=
 	       rb_page_write(cpu_buffer->commit_page)) {
-		cpu_buffer->commit_page->commit =
+		cpu_buffer->commit_page->page->commit =
 			cpu_buffer->commit_page->write;
 		barrier();
 	}

@@ -843,7 +852,7 @@ rb_set_commit_to_write(struct ring_buffer_per_cpu *cpu_buffer)
 
 static void rb_reset_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 {
-	cpu_buffer->read_stamp = cpu_buffer->reader_page->time_stamp;
+	cpu_buffer->read_stamp = cpu_buffer->reader_page->page->time_stamp;
 	cpu_buffer->reader_page->read = 0;
 }
 

@@ -862,7 +871,7 @@ static inline void rb_inc_iter(struct ring_buffer_iter *iter)
 	else
 		rb_inc_page(cpu_buffer, &iter->head_page);
 
-	iter->read_stamp = iter->head_page->time_stamp;
+	iter->read_stamp = iter->head_page->page->time_stamp;
 	iter->head = 0;
 }
 

@@ -998,12 +1007,12 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,
 		 */
 		if (tail_page == cpu_buffer->tail_page) {
 			local_set(&next_page->write, 0);
-			local_set(&next_page->commit, 0);
+			local_set(&next_page->page->commit, 0);
 			cpu_buffer->tail_page = next_page;
 
 			/* reread the time stamp */
 			*ts = ring_buffer_time_stamp(cpu_buffer->cpu);
-			cpu_buffer->tail_page->time_stamp = *ts;
+			cpu_buffer->tail_page->page->time_stamp = *ts;
 		}
 
 		/*

@@ -1048,7 +1057,7 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,
 	 * this page's time stamp.
 	 */
 	if (!tail && rb_is_commit(cpu_buffer, event))
-		cpu_buffer->commit_page->time_stamp = *ts;
+		cpu_buffer->commit_page->page->time_stamp = *ts;
 
 	return event;
 

@@ -1099,7 +1108,7 @@ rb_add_time_stamp(struct ring_buffer_per_cpu *cpu_buffer,
 			event->time_delta = *delta & TS_MASK;
 			event->array[0] = *delta >> TS_SHIFT;
 		} else {
-			cpu_buffer->commit_page->time_stamp = *ts;
+			cpu_buffer->commit_page->page->time_stamp = *ts;
 			event->time_delta = 0;
 			event->array[0] = 0;
 		}

@@ -1552,7 +1561,7 @@ static void rb_iter_reset(struct ring_buffer_iter *iter)
 	if (iter->head)
 		iter->read_stamp = cpu_buffer->read_stamp;
 	else
-		iter->read_stamp = iter->head_page->time_stamp;
+		iter->read_stamp = iter->head_page->page->time_stamp;
 }
 
 /**

@@ -1696,7 +1705,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	cpu_buffer->reader_page->list.prev = reader->list.prev;
 
 	local_set(&cpu_buffer->reader_page->write, 0);
-	local_set(&cpu_buffer->reader_page->commit, 0);
+	local_set(&cpu_buffer->reader_page->page->commit, 0);
 
 	/* Make the reader page now replace the head */
 	reader->list.prev->next = &cpu_buffer->reader_page->list;

@@ -2088,7 +2097,7 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer)
 	cpu_buffer->head_page
 		= list_entry(cpu_buffer->pages.next, struct buffer_page, list);
 	local_set(&cpu_buffer->head_page->write, 0);
-	local_set(&cpu_buffer->head_page->commit, 0);
+	local_set(&cpu_buffer->head_page->page->commit, 0);
 
 	cpu_buffer->head_page->read = 0;
 

@@ -2097,7 +2106,7 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer)
 
 	INIT_LIST_HEAD(&cpu_buffer->reader_page->list);
 	local_set(&cpu_buffer->reader_page->write, 0);
-	local_set(&cpu_buffer->reader_page->commit, 0);
+	local_set(&cpu_buffer->reader_page->page->commit, 0);
 	cpu_buffer->reader_page->read = 0;
 
 	cpu_buffer->overrun = 0;
Signature · 3 lines
1.5.6.5

--
author maintainer
#4 lore raw

PATCH 3/5 ring-buffer: read page interface

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: new API to ring buffer

This patch adds a new interface into the ring buffer that allows a page to be read from the ring buffer on a given CPU. For every page read, one must also be given to allow for a "swap" of the pages.

rpage = ring_buffer_alloc_read_page(buffer);
if (!rpage)

	goto err;
 ret = ring_buffer_read_page(buffer, &rpage, cpu, full);
 if (!ret)
	goto empty;

process_page(rpage);
ring_buffer_free_read_page(rpage);

The caller of these functions must handle any waits that are needed to wait for new data. The ring_buffer_read_page will simply return 0 if there is no data, or if "full" is set and the writer is still on the current page.

Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/ring_buffer.h |    5 ++
 kernel/trace/ring_buffer.c  |  166 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 171 insertions(+), 0 deletions(-)
include/linux/ring_buffer.h +5 -0
index 3bb87a7..1a350a8 100644
@@ -124,6 +124,11 @@ void tracing_on(void);
 void tracing_off(void);
 void tracing_off_permanent(void);
 
+void *ring_buffer_alloc_read_page(struct ring_buffer *buffer);
+void ring_buffer_free_read_page(struct ring_buffer *buffer, void *data);
+int ring_buffer_read_page(struct ring_buffer *buffer,
+			  void **data_page, int cpu, int full);
+
 enum ring_buffer_flags {
 	RB_FL_OVERWRITE		= 1 << 0,
 };
kernel/trace/ring_buffer.c +166 -0

178 lines not on this page · raw message

Signature · 3 lines
1.5.6.5

--
author maintainer
#8 lore raw

PATCH 4/5 ftrace: clean up function graph asm

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: clean up

There exists macros for x86 asm to handle x86_64 and i386. This patch updates function graph asm to use them.

Signed-off-by Steven Rostedt <[email protected]>
---
 arch/x86/kernel/ftrace.c |   20 ++++----------------
 1 files changed, 4 insertions(+), 16 deletions(-)
arch/x86/kernel/ftrace.c +4 -16
index 5883247..1a5b8f8 100644
@@ -467,28 +467,16 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 	 * ignore such a protection.
 	 */
 	asm volatile(
-#ifdef CONFIG_X86_64
-		"1: movq (%[parent_old]), %[old]\n"
-		"2: movq %[return_hooker], (%[parent_replaced])\n"
-#else
-		"1: movl (%[parent_old]), %[old]\n"
-		"2: movl %[return_hooker], (%[parent_replaced])\n"
-#endif
+		"1: " _ASM_MOV " (%[parent_old]), %[old]\n"
+		"2: " _ASM_MOV " %[return_hooker], (%[parent_replaced])\n"
 		"   movl $0, %[faulted]\n"
 
 		".section .fixup, \"ax\"\n"
 		"3: movl $1, %[faulted]\n"
 		".previous\n"
 
-		".section __ex_table, \"a\"\n"
-#ifdef CONFIG_X86_64
-		"   .quad 1b, 3b\n"
-		"   .quad 2b, 3b\n"
-#else
-		"   .long 1b, 3b\n"
-		"   .long 2b, 3b\n"
-#endif
-		".previous\n"
+		_ASM_EXTABLE(1b, 3b)
+		_ASM_EXTABLE(2b, 3b)
 
 		: [parent_replaced] "=r" (parent), [old] "=r" (old),
 		  [faulted] "=r" (faulted)
Signature · 3 lines
1.5.6.5

--
#9 lore raw

Re: [PATCH 4/5] ftrace: clean up function graph asm

2008/12/2 Steven Rostedt <[email protected]>:

Quoted 40 lines

From: Steven Rostedt <[email protected]>

Impact: clean up

There exists macros for x86 asm to handle x86_64 and i386. This patch updates function graph asm to use them.

Signed-off-by Steven Rostedt <[email protected]>
---
 arch/x86/kernel/ftrace.c |   20 ++++----------------
 1 files changed, 4 insertions(+), 16 deletions(-)
arch/x86/kernel/ftrace.c +2 -12
index 5883247..1a5b8f8 100644
@@ -467,28 +467,16 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
        * ignore such a protection.
        */
       asm volatile(
-#ifdef CONFIG_X86_64
-               "1: movq (%[parent_old]), %[old]\n"
-               "2: movq %[return_hooker], (%[parent_replaced])\n"
-#else
-               "1: movl (%[parent_old]), %[old]\n"
-               "2: movl %[return_hooker], (%[parent_replaced])\n"
-#endif
+               "1: " _ASM_MOV " (%[parent_old]), %[old]\n"
+               "2: " _ASM_MOV " %[return_hooker], (%[parent_replaced])\n"
               "   movl $0, %[faulted]\n"
 
               ".section .fixup, \"ax\"\n"
               "3: movl $1, %[faulted]\n"
               ".previous\n"
 
-               ".section __ex_table, \"a\"\n"
-#ifdef CONFIG_X86_64
-               "   .quad 1b, 3b\n"
-               "   .quad 2b, 3b\n"
-#else

arch/x86/kernel/ftrace.c void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)

Quoted 10 lines
arch/x86/kernel/ftrace.c +2 -5
-               "   .long 1b, 3b\n"
-               "   .long 2b, 3b\n"
-#endif
-               ".previous\n"
+               _ASM_EXTABLE(1b, 3b)
+               _ASM_EXTABLE(2b, 3b)
 
               : [parent_replaced] "=r" (parent), [old] "=r" (old),
                 [faulted] "=r" (faulted)
--

Thanks Steven. I wasn't aware of _ASM_MOV.

#5 lore raw

Re: [PATCH 3/5] ring-buffer: read page interface

In reply to Steven Rostedt
Quoted 24 lines · On Tue, 02 Dec 2008 15:34:07 -0500 Steven Rostedt <[email protected]> wrote:

From: Steven Rostedt <[email protected]>

Impact: new API to ring buffer

This patch adds a new interface into the ring buffer that allows a page to be read from the ring buffer on a given CPU. For every page read, one must also be given to allow for a "swap" of the pages.

rpage = ring_buffer_alloc_read_page(buffer);
if (!rpage)

	goto err;
 ret = ring_buffer_read_page(buffer, &rpage, cpu, full);
 if (!ret)
	goto empty;

process_page(rpage);
ring_buffer_free_read_page(rpage);

The caller of these functions must handle any waits that are needed to wait for new data. The ring_buffer_read_page will simply return 0 if there is no data, or if "full" is set and the writer is still on the current page.

...

kernel/trace/ring_buffer.c static inline int rb_null_event(struct ring_buffer_event *event) · line 694

Quoted 5 lines
kernel/trace/ring_buffer.c +5 -0
+static inline void *
+__rb_data_page_index(struct buffer_data_page *page, unsigned index)
+{
+	return page->data + index;
+}

Many kernel developers will fall over stunned and confused when they see a variable called page' which does not have type struct page *'. This is a very strong kernel convention and this code tromps all over it :(

How's about renaming all of these everywhere to `bpage' or whatever?

kernel/trace/ring_buffer.c int ring_buffer_swap_cpu(struct ring_buffer *buffer_a, · line 2362

Quoted 5 lines

...

kernel/trace/ring_buffer.c +1 -0
+	if (!local_read(&cpu_buffer->reader_page->page->commit))

Rusty (I think it was Rusty) has convincingly argued that local_t is irredeemably busted. I forget the details?

author maintainer
#6 lore raw

Re: [PATCH 3/5] ring-buffer: read page interface

Quoted 38 lines · Andrew Morton
Quoted 24 lines · On Tue, 02 Dec 2008 15:34:07 -0500 Steven Rostedt <[email protected]> wrote:

From: Steven Rostedt <[email protected]>

Impact: new API to ring buffer

This patch adds a new interface into the ring buffer that allows a page to be read from the ring buffer on a given CPU. For every page read, one must also be given to allow for a "swap" of the pages.

rpage = ring_buffer_alloc_read_page(buffer);
if (!rpage)

	goto err;
 ret = ring_buffer_read_page(buffer, &rpage, cpu, full);
 if (!ret)
	goto empty;

process_page(rpage);
ring_buffer_free_read_page(rpage);

The caller of these functions must handle any waits that are needed to wait for new data. The ring_buffer_read_page will simply return 0 if there is no data, or if "full" is set and the writer is still on the current page.

...

kernel/trace/ring_buffer.c static inline int rb_null_event(struct ring_buffer_event *event) · line 694

Quoted 5 lines
kernel/trace/ring_buffer.c +5 -0
+static inline void *
+__rb_data_page_index(struct buffer_data_page *page, unsigned index)
+{
+	return page->data + index;
+}

Many kernel developers will fall over stunned and confused when they see a variable called page' which does not have type struct page *'. This is a very strong kernel convention and this code tromps all over it :(

Quoted 1 line

How's about renaming all of these everywhere to `bpage' or whatever?

OK, I'll send a patch to Ingo to make this update.

Quoted 9 lines

kernel/trace/ring_buffer.c int ring_buffer_swap_cpu(struct ring_buffer *buffer_a, · line 2362

Quoted 6 lines

...

kernel/trace/ring_buffer.c +1 -0
+	if (!local_read(&cpu_buffer->reader_page->page->commit))

Rusty (I think it was Rusty) has convincingly argued that local_t is irredeemably busted. I forget the details?

Really?? I've been using local for quite a bit. Using atomic seems overkill, since I only want to be able to protect against updates by interrupts. Perhaps what needs to be done is to fix local?

-- Steve

#7 lore raw

Re: [PATCH 3/5] ring-buffer: read page interface

In reply to Andrew Morton
Quoted 5 lines · On Wednesday 03 December 2008 08:06:54 Andrew Morton wrote:

kernel/trace/ring_buffer.c int ring_buffer_swap_cpu(struct ring_buffer *buffer_a, · line 2362

Quoted 1 line · Steven Rostedt
kernel/trace/ring_buffer.c +1 -0
+	if (!local_read(&cpu_buffer->reader_page->page->commit))

Rusty (I think it was Rusty) has convincingly argued that local_t is irredeemably busted. I forget the details?

That was probably Christoph Lameter. The x86 implementation needs some love, but the API seems reasonable.

On x86-32, the cpu_local_inc should be (as intended) turned into one insn. That would be possible on 64 bit once we get zero-based percpu (as will World Peace).

Cheers,
Rusty.

#11 lore raw
In reply to Steven Rostedt
Quoted 5 lines · Steven Rostedt

Ingo,

Here's some more ftrace updates.

The first should help fix the bug in lockdep.

ok.

Quoted 2 lines

The next are changes to the ring buffer to get ready to have it work with splice.

great!

Quoted 1 line

The last are some clean ups for function graph.

ok. I'll also pull the followup cleanups that you did based on Andrew's suggestions.

Quoted 15 lines

The following patches are in:

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

branch: tip/devel

Steven Rostedt (5):
ftrace: replace raw_local_irq_save with local_irq_save
ring-buffer: move some metadata into buffer page
ring-buffer: read page interface
ftrace: clean up function graph asm
ftrace: have function graph use mcount caller address

----

Quoted 10 lines
 arch/x86/kernel/entry_32.S  |    1 +
 arch/x86/kernel/entry_64.S  |    1 +
 arch/x86/kernel/ftrace.c    |   20 +---
 include/linux/ring_buffer.h |    5 +
 kernel/lockdep.c            |    1 +
 kernel/trace/ring_buffer.c  |  229 ++++++++++++++++++++++++++++++++++++++-----
 kernel/trace/trace.c        |   12 +-
 kernel/trace/trace_branch.c |    4 +-
 kernel/trace/trace_stack.c  |    8 +-
 9 files changed, 226 insertions(+), 55 deletions(-)

pulled into tip/tracing/ftrace, thanks Steve!

Ingo

#2 lore raw

Re: [PATCH 1/5] ftrace: replace raw_local_irq_save with local_irq_save

In reply to Steven Rostedt

hi Ingo,

Where could I find define of "local_irq_save" for x86 if CONFIG_TRACE_IRQFLAGS_SUPPORT is not defined?

Liming Wang