Skip to contentSkip to conversation

tracing: Review of changes

PATCH In review Steven Rostedt 2 participants 15 messages Jan 21, 2013

Subsystems (6)
x86 Block layer Tracing ftrace +2 more subsystems
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover tracing: Review of changes

I'm posting these as a generic review of the changes I'll be pushing into linux-next and will be pushing this for 3.9.

These patches have passed a series of tests I've run on few machines with several different configs and such.

You can find the patches here. I'll add these to linux-next tomorrow.

-- Steve

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

Head SHA1: a701a1b03bf36d4e31c9fd036a96bde5a11cd5ce

Fengguang Wu (1):
tracing/syscalls: Make local functions static

Hiraku Toyooka (1):

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

Jovi Zhang (1):
tracing: Verify target file before registering a uprobe event

Masami Hiramatsu (3):
ftrace: Move ARCH_SUPPORTS_FTRACE_SAVE_REGS in Kconfig kprobes/x86: Move ftrace-based kprobe code into kprobes-ftrace.c kprobes/x86: Move kprobes stuff under arch/x86/kernel/kprobes/

Shan Wei (1):
tracing: Use this_cpu_ptr per-cpu helper

Steven Rostedt (6):
tracing: Fix sparse warning with is_signed_type() macro
ring-buffer: Remove unnecessary recusive call in rb_advance_iter()
tracing: Remove unneeded check of max_tr->buffer before tracing_reset tracing/lockdep: Disable lockdep first in entering NMI tracing/fgraph: Add max_graph_depth to limit function_graph depth
tracing: Remove the extra 4 bytes of padding in events

----

 arch/Kconfig                         |   12 +
 arch/x86/Kconfig                     |    2 +
 arch/x86/include/asm/ftrace.h        |    1 -
 arch/x86/kernel/Makefile             |    3 +-
 arch/x86/kernel/kprobes-common.h     |  102 ---
 arch/x86/kernel/kprobes-opt.c        |  512 ---------------
 arch/x86/kernel/kprobes.c            | 1130 ----------------------------------
 arch/x86/kernel/kprobes/Makefile     |    7 +
 arch/x86/kernel/kprobes/common.h     |  113 ++++
 arch/x86/kernel/kprobes/core.c       | 1064 ++++++++++++++++++++++++++++++++
 arch/x86/kernel/kprobes/ftrace.c     |   93 +++
 arch/x86/kernel/kprobes/opt.c        |  512 +++++++++++++++
 include/linux/ftrace.h               |    6 +-
 include/linux/ftrace_event.h         |    3 +-
 include/linux/hardirq.h              |    4 +-
 include/linux/kprobes.h              |   12 +-
 kernel/kprobes.c                     |    8 +-
 kernel/trace/Kconfig                 |    8 +
 kernel/trace/blktrace.c              |    2 +-
 kernel/trace/ftrace.c                |    6 +-
 kernel/trace/ring_buffer.c           |    2 +-
 kernel/trace/trace.c                 |   15 +-
 kernel/trace/trace_events.c          |    1 -
 kernel/trace/trace_functions_graph.c |   60 +-
 kernel/trace/trace_selftest.c        |    2 +-
 kernel/trace/trace_syscalls.c        |   18 +-
 kernel/trace/trace_uprobe.c          |    6 +-
 27 files changed, 1909 insertions(+), 1795 deletions(-)
author maintainer
#1 lore raw

PATCH 1/13 tracing: Fix sparse warning with is_signed_type() macro

From: Steven Rostedt <[email protected]>

Sparse complains when is_signed_type() is used on a pointer. This macro is needed for the format output used for ftrace and perf, to know if a binary field is a signed type or not. The is_signed_type() macro is used against all fields that are recorded by events to automate the operation.

The problem sparse has is with the current way is_signed_type() works:

((type)-1 < 0)

If "type" is a poiner, than sparse does not like it being compared to an integer (zero). The simple fix is to just give zero the same type. The runtime result stays the same.

---
 include/linux/ftrace_event.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/ftrace_event.h +1 -1
index a3d4895..43ef8b6 100644
@@ -272,7 +272,7 @@ extern int trace_define_field(struct ftrace_event_call *call, const char *type,
 extern int trace_add_event_call(struct ftrace_event_call *call);
 extern void trace_remove_event_call(struct ftrace_event_call *call);
 
-#define is_signed_type(type)	(((type)(-1)) < 0)
+#define is_signed_type(type)	(((type)(-1)) < (type)0)
 
 int trace_set_clr_event(const char *system, const char *event, int set);
 
Signature · 1 line
1.7.10.4
author maintainer
#2 lore raw

PATCH 2/13 ring-buffer: Remove unnecessary recusive call in rb_advance_iter()

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

The original ring-buffer code had special checks at the start of rb_advance_iter() and instead of repeating them again at the end of the function if a certain condition existed, I just did a recursive call to rb_advance_iter() because the special condition would cause rb_advance_iter() to return early (after the checks).

But as things have changed, the special checks no longer exist and the only thing done for the special_condition is to call rb_inc_iter() and return. Instead of doing a confusing recursive call, just call rb_inc_iter instead.

---
 kernel/trace/ring_buffer.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/ring_buffer.c +1 -1
index ce8514f..6ff9cc4 100644
@@ -3425,7 +3425,7 @@ static void rb_advance_iter(struct ring_buffer_iter *iter)
 	/* check for end of page padding */
 	if ((iter->head >= rb_page_size(iter->head_page)) &&
 	    (iter->head_page != cpu_buffer->commit_page))
-		rb_advance_iter(iter);
+		rb_inc_iter(iter);
 }
 
 static int rb_lost_events(struct ring_buffer_per_cpu *cpu_buffer)
Signature · 1 line
1.7.10.4
author maintainer
#3 lore raw

PATCH 3/13 tracing: Use this_cpu_ptr per-cpu helper

In reply to Steven Rostedt

From: Shan Wei <[email protected]>

typeof(&buffer) is a pointer to array of 1024 char, or char (*)[1024].
But, typeof(&buffer[0]) is a pointer to char which match the return type of get_trace_buf(). As well-known, the value of &buffer is equal to &buffer[0].
so return this_cpu_ptr(&percpu_buffer->buffer[0]) can avoid type cast.

---
 kernel/trace/blktrace.c |    2 +-
 kernel/trace/trace.c    |    5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)
kernel/trace/blktrace.c +1 -1
index c0bd030..71259e2 100644
@@ -147,7 +147,7 @@ void __trace_note_message(struct blk_trace *bt, const char *fmt, ...)
 		return;
 
 	local_irq_save(flags);
-	buf = per_cpu_ptr(bt->msg_data, smp_processor_id());
+	buf = this_cpu_ptr(bt->msg_data);
 	va_start(args, fmt);
 	n = vscnprintf(buf, BLK_TN_MAX_MSG, fmt, args);
 	va_end(args);
kernel/trace/trace.c +1 -4
index 3c13e46..f8b7c62 100644
@@ -1517,7 +1517,6 @@ static struct trace_buffer_struct *trace_percpu_nmi_buffer;
 static char *get_trace_buf(void)
 {
 	struct trace_buffer_struct *percpu_buffer;
-	struct trace_buffer_struct *buffer;
 
 	/*
 	 * If we have allocated per cpu buffers, then we do not
@@ -1535,9 +1534,7 @@ static char *get_trace_buf(void)
 	if (!percpu_buffer)
 		return NULL;
 
-	buffer = per_cpu_ptr(percpu_buffer, smp_processor_id());
-
-	return buffer->buffer;
+	return this_cpu_ptr(&percpu_buffer->buffer[0]);
 }
 
 static int alloc_percpu_trace_buffer(void)
Signature · 1 line
1.7.10.4
author maintainer
#4 lore raw

PATCH 4/13 tracing: Verify target file before registering a uprobe event

In reply to Steven Rostedt

From: Jovi Zhang <[email protected]>

Without this patch, we can register a uprobe event for a directory. Enabling such a uprobe event would anyway fail.

Example:

$ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events

However dirctories cannot be valid targets for uprobe. Hence verify if the target is a regular file during the probe registration.

Cc Namhyung Kim <[email protected]>
[ cleaned up whitespace and removed redundant IS_DIR() check ]
---
 kernel/trace/trace_uprobe.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
kernel/trace/trace_uprobe.c +5 -1
index c86e6d4..87b6db4 100644
@@ -258,6 +258,10 @@ static int create_trace_uprobe(int argc, char **argv)
 		goto fail_address_parse;
 
 	inode = igrab(path.dentry->d_inode);
+	if (!S_ISREG(inode->i_mode)) {
+		ret = -EINVAL;
+		goto fail_address_parse;
+	}
 
 	argc -= 2;
 	argv += 2;
@@ -356,7 +360,7 @@ fail_address_parse:
 	if (inode)
 		iput(inode);
 
-	pr_info("Failed to parse address.\n");
+	pr_info("Failed to parse address or file.\n");
 
 	return ret;
 }
Signature · 1 line
1.7.10.4
author maintainer
#5 lore raw

PATCH 5/13 tracing/syscalls: Make local functions static

In reply to Steven Rostedt

From: Fengguang Wu <[email protected]>

Some functions in the syscall tracing is used only locally to the file, but they are labeled global. Convert them to static functions.

---
 kernel/trace/trace_syscalls.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
kernel/trace/trace_syscalls.c +9 -9
index 7609dd6..5329e13e 100644
@@ -77,7 +77,7 @@ static struct syscall_metadata *syscall_nr_to_meta(int nr)
 	return syscalls_metadata[nr];
 }
 
-enum print_line_t
+static enum print_line_t
 print_syscall_enter(struct trace_iterator *iter, int flags,
 		    struct trace_event *event)
 {
@@ -130,7 +130,7 @@ end:
 	return TRACE_TYPE_HANDLED;
 }
 
-enum print_line_t
+static enum print_line_t
 print_syscall_exit(struct trace_iterator *iter, int flags,
 		   struct trace_event *event)
 {
@@ -270,7 +270,7 @@ static int syscall_exit_define_fields(struct ftrace_event_call *call)
 	return ret;
 }
 
-void ftrace_syscall_enter(void *ignore, struct pt_regs *regs, long id)
+static void ftrace_syscall_enter(void *ignore, struct pt_regs *regs, long id)
 {
 	struct syscall_trace_enter *entry;
 	struct syscall_metadata *sys_data;
@@ -305,7 +305,7 @@ void ftrace_syscall_enter(void *ignore, struct pt_regs *regs, long id)
 		trace_current_buffer_unlock_commit(buffer, event, 0, 0);
 }
 
-void ftrace_syscall_exit(void *ignore, struct pt_regs *regs, long ret)
+static void ftrace_syscall_exit(void *ignore, struct pt_regs *regs, long ret)
 {
 	struct syscall_trace_exit *entry;
 	struct syscall_metadata *sys_data;
@@ -337,7 +337,7 @@ void ftrace_syscall_exit(void *ignore, struct pt_regs *regs, long ret)
 		trace_current_buffer_unlock_commit(buffer, event, 0, 0);
 }
 
-int reg_event_syscall_enter(struct ftrace_event_call *call)
+static int reg_event_syscall_enter(struct ftrace_event_call *call)
 {
 	int ret = 0;
 	int num;
@@ -356,7 +356,7 @@ int reg_event_syscall_enter(struct ftrace_event_call *call)
 	return ret;
 }
 
-void unreg_event_syscall_enter(struct ftrace_event_call *call)
+static void unreg_event_syscall_enter(struct ftrace_event_call *call)
 {
 	int num;
 
@@ -371,7 +371,7 @@ void unreg_event_syscall_enter(struct ftrace_event_call *call)
 	mutex_unlock(&syscall_trace_lock);
 }
 
-int reg_event_syscall_exit(struct ftrace_event_call *call)
+static int reg_event_syscall_exit(struct ftrace_event_call *call)
 {
 	int ret = 0;
 	int num;
@@ -390,7 +390,7 @@ int reg_event_syscall_exit(struct ftrace_event_call *call)
 	return ret;
 }
 
-void unreg_event_syscall_exit(struct ftrace_event_call *call)
+static void unreg_event_syscall_exit(struct ftrace_event_call *call)
 {
 	int num;
 
@@ -459,7 +459,7 @@ unsigned long __init __weak arch_syscall_addr(int nr)
 	return (unsigned long)sys_call_table[nr];
 }
 
-int __init init_ftrace_syscalls(void)
+static int __init init_ftrace_syscalls(void)
 {
 	struct syscall_metadata *meta;
 	unsigned long addr;
Signature · 1 line
1.7.10.4
author maintainer
#6 lore raw

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

In reply to Steven Rostedt

From: Hiraku Toyooka <[email protected]>

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.

---
 kernel/trace/trace.c |    6 ++++++
 1 file changed, 6 insertions(+)
kernel/trace/trace.c +6 -0
index f8b7c62..72b171b 100644
@@ -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 */
Signature · 1 line
1.7.10.4
author maintainer
#7 lore raw

PATCH 7/13 tracing: Remove unneeded check of max_tr->buffer before tracing_reset

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

There's now a check in tracing_reset_online_cpus() if the buffer is allocated or NULL. No need to do a check before calling it with max_tr.

---
 kernel/trace/trace.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
kernel/trace/trace.c +1 -2
index 72b171b..d62248d 100644
@@ -4040,8 +4040,7 @@ static ssize_t tracing_clock_write(struct file *filp, const char __user *ubuf,
 	 * Reset the buffer so that it doesn't have incomparable timestamps.
 	 */
 	tracing_reset_online_cpus(&global_trace);
-	if (max_tr.buffer)
-		tracing_reset_online_cpus(&max_tr);
+	tracing_reset_online_cpus(&max_tr);
 
 	mutex_unlock(&trace_types_lock);
 
Signature · 1 line
1.7.10.4
author maintainer
#8 lore raw

PATCH 8/13 tracing/lockdep: Disable lockdep first in entering NMI

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

When function tracing with either debug locks enabled or tracing preempt disabled, the add_preempt_count() is traced. This is an issue with lockdep and function tracing. As function tracing can disable interrupts, and lockdep records that change, lockdep may not be able to handle this recursion if it happens from an NMI context.

The first thing that an NMI does is:

 #define nmi_enter()					\
	do {							\
		ftrace_nmi_enter();				\
		BUG_ON(in_nmi());				\
		add_preempt_count(NMI_OFFSET + HARDIRQ_OFFSET);	\
		lockdep_off();					\
		rcu_nmi_enter();				\
		trace_hardirq_enter();				\
	} while (0)

When the add_preempt_count() is traced, and the tracing callback disables interrupts, it will jump into the lockdep code. There's some places in lockdep that can't handle this re-entrance, and causes lockdep to fail.

As the lockdep_off() (and lockdep_on) is a simple:

void lockdep_off(void)

{
	current->lockdep_recursion++;
}

and is never traced, it can be called first in nmi_enter() and lockdep_on() last in nmi_exit().

---
 include/linux/hardirq.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/hardirq.h +2 -2
index 624ef3f..57bfdce 100644
@@ -180,10 +180,10 @@ extern void irq_exit(void);
 
 #define nmi_enter()						\
 	do {							\
+		lockdep_off();					\
 		ftrace_nmi_enter();				\
 		BUG_ON(in_nmi());				\
 		add_preempt_count(NMI_OFFSET + HARDIRQ_OFFSET);	\
-		lockdep_off();					\
 		rcu_nmi_enter();				\
 		trace_hardirq_enter();				\
 	} while (0)
@@ -192,10 +192,10 @@ extern void irq_exit(void);
 	do {							\
 		trace_hardirq_exit();				\
 		rcu_nmi_exit();					\
-		lockdep_on();					\
 		BUG_ON(!in_nmi());				\
 		sub_preempt_count(NMI_OFFSET + HARDIRQ_OFFSET);	\
 		ftrace_nmi_exit();				\
+		lockdep_on();					\
 	} while (0)
 
 #endif /* LINUX_HARDIRQ_H */
Signature · 1 line
1.7.10.4
author maintainer
#9 lore raw

PATCH 9/13 tracing/fgraph: Add max_graph_depth to limit function_graph depth

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Add the file max_graph_depth to the debug tracing directory that lets the user define the depth of the function graph.

A very useful operation is to set the depth to 1. Then it traces only the first function that is called when entering the kernel. This can be used to determine what system operations interrupt a process.

For example, to work on NOHZ processes (single tasks running without a timer tick), if any interrupt goes off and preempts that task, this code will show it happening.

  # cd /sys/kernel/debug/tracing
  # echo 1 > max_graph_depth
  # echo function_graph > current_tracer
  # cat per_cpu/cpu/<cpu-of-process>/trace
Cc Frederic Weisbecker <[email protected]>
---
 kernel/trace/trace_functions_graph.c |   60 ++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 2 deletions(-)
kernel/trace/trace_functions_graph.c +58 -2
index 4edb4b7..7008d2e 100644
@@ -47,6 +47,8 @@ struct fgraph_data {
 #define TRACE_GRAPH_PRINT_ABS_TIME	0x20
 #define TRACE_GRAPH_PRINT_IRQS		0x40
 
+static unsigned int max_depth;
+
 static struct tracer_opt trace_opts[] = {
 	/* Display overruns? (for self-debug purpose) */
 	{ TRACER_OPT(funcgraph-overrun, TRACE_GRAPH_PRINT_OVERRUN) },
@@ -250,8 +252,9 @@ int trace_graph_entry(struct ftrace_graph_ent *trace)
 		return 0;
 
 	/* trace it when it is-nested-in or is a function enabled. */
-	if (!(trace->depth || ftrace_graph_addr(trace->func)) ||
-	      ftrace_graph_ignore_irqs())
+	if ((!(trace->depth || ftrace_graph_addr(trace->func)) ||
+	     ftrace_graph_ignore_irqs()) ||
+	    (max_depth && trace->depth >= max_depth))
 		return 0;
 
 	local_irq_save(flags);
@@ -1457,6 +1460,59 @@ static struct tracer graph_trace __read_mostly = {
 #endif
 };
 
+
+static ssize_t
+graph_depth_write(struct file *filp, const char __user *ubuf, size_t cnt,
+		  loff_t *ppos)
+{
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
+	if (ret)
+		return ret;
+
+	max_depth = val;
+
+	*ppos += cnt;
+
+	return cnt;
+}
+
+static ssize_t
+graph_depth_read(struct file *filp, char __user *ubuf, size_t cnt,
+		 loff_t *ppos)
+{
+	char buf[15]; /* More than enough to hold UINT_MAX + "\n"*/
+	int n;
+
+	n = sprintf(buf, "%d\n", max_depth);
+
+	return simple_read_from_buffer(ubuf, cnt, ppos, buf, n);
+}
+
+static const struct file_operations graph_depth_fops = {
+	.open		= tracing_open_generic,
+	.write		= graph_depth_write,
+	.read		= graph_depth_read,
+	.llseek		= generic_file_llseek,
+};
+
+static __init int init_graph_debugfs(void)
+{
+	struct dentry *d_tracer;
+
+	d_tracer = tracing_init_dentry();
+	if (!d_tracer)
+		return 0;
+
+	trace_create_file("max_graph_depth", 0644, d_tracer,
+			  NULL, &graph_depth_fops);
+
+	return 0;
+}
+fs_initcall(init_graph_debugfs);
+
 static __init int init_graph_trace(void)
 {
 	max_bytes_for_cpu = snprintf(NULL, 0, "%d", nr_cpu_ids - 1);
Signature · 1 line
1.7.10.4
author maintainer
#10 lore raw

PATCH 10/13 ftrace: Move ARCH_SUPPORTS_FTRACE_SAVE_REGS in Kconfig

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Move SAVE_REGS support flag into Kconfig and rename it to CONFIG_DYNAMIC_FTRACE_WITH_REGS. This also introduces CONFIG_HAVE_DYNAMIC_FTRACE_WITH_REGS which indicates the architecture depending part of ftrace has a code that saves full registers.
On the other hand, CONFIG_DYNAMIC_FTRACE_WITH_REGS indicates the code is enabled.

Cc Ingo Molnar <[email protected]>
Cc Ananth N Mavinakayanahalli <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Frederic Weisbecker <[email protected]>
---
 arch/x86/Kconfig              |    1 +
 arch/x86/include/asm/ftrace.h |    1 -
 include/linux/ftrace.h        |    6 +++---
 kernel/trace/Kconfig          |    8 ++++++++
 kernel/trace/ftrace.c         |    6 +++---
 kernel/trace/trace_selftest.c |    2 +-
 6 files changed, 16 insertions(+), 8 deletions(-)
arch/x86/Kconfig +1 -0
index 79795af..996ccec 100644
@@ -44,6 +44,7 @@ config X86
 	select HAVE_FENTRY if X86_64
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_FUNCTION_GRAPH_FP_TEST
arch/x86/include/asm/ftrace.h +0 -1
index 9a25b52..86cb51e 100644
@@ -44,7 +44,6 @@
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 #define ARCH_SUPPORTS_FTRACE_OPS 1
-#define ARCH_SUPPORTS_FTRACE_SAVE_REGS
 #endif
 
 #ifndef __ASSEMBLY__
include/linux/ftrace.h +3 -3
index 92691d8..e5ca8ef 100644
@@ -74,7 +74,7 @@ typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
  * SAVE_REGS - The ftrace_ops wants regs saved at each function called
  *            and passed to the callback. If this flag is set, but the
  *            architecture does not support passing regs
- *            (ARCH_SUPPORTS_FTRACE_SAVE_REGS is not defined), then the
+ *            (CONFIG_DYNAMIC_FTRACE_WITH_REGS is not defined), then the
  *            ftrace_ops will fail to register, unless the next flag
  *            is set.
  * SAVE_REGS_IF_SUPPORTED - This is the same as SAVE_REGS, but if the
@@ -418,7 +418,7 @@ void ftrace_modify_all_code(int command);
 #endif
 
 #ifndef FTRACE_REGS_ADDR
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 # define FTRACE_REGS_ADDR ((unsigned long)ftrace_regs_caller)
 #else
 # define FTRACE_REGS_ADDR FTRACE_ADDR
@@ -480,7 +480,7 @@ extern int ftrace_make_nop(struct module *mod,
  */
 extern int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr);
 
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 /**
  * ftrace_modify_call - convert from one addr to another (no nop)
  * @rec: the mcount call site record
kernel/trace/Kconfig +8 -0
index 5d89335..cdc9d28 100644
@@ -39,6 +39,9 @@ config HAVE_DYNAMIC_FTRACE
 	help
 	  See Documentation/trace/ftrace-design.txt
 
+config HAVE_DYNAMIC_FTRACE_WITH_REGS
+	bool
+
 config HAVE_FTRACE_MCOUNT_RECORD
 	bool
 	help
@@ -434,6 +437,11 @@ config DYNAMIC_FTRACE
 	  were made. If so, it runs stop_machine (stops all CPUS)
 	  and modifies the code to jump over the call to ftrace.
 
+config DYNAMIC_FTRACE_WITH_REGS
+	def_bool y
+	depends on DYNAMIC_FTRACE
+	depends on HAVE_DYNAMIC_FTRACE_WITH_REGS
+
 config FUNCTION_PROFILER
 	bool "Kernel function profiler"
 	depends on FUNCTION_TRACER
kernel/trace/ftrace.c +3 -3
index 41473b4..6e34dc1 100644
@@ -337,7 +337,7 @@ static int __register_ftrace_function(struct ftrace_ops *ops)
 	if ((ops->flags & FL_GLOBAL_CONTROL_MASK) == FL_GLOBAL_CONTROL_MASK)
 		return -EINVAL;
 
-#ifndef ARCH_SUPPORTS_FTRACE_SAVE_REGS
+#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 	/*
 	 * If the ftrace_ops specifies SAVE_REGS, then it only can be used
 	 * if the arch supports it, or SAVE_REGS_IF_SUPPORTED is also set.
@@ -4143,8 +4143,8 @@ __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
  * Archs are to support both the regs and ftrace_ops at the same time.
  * If they support ftrace_ops, it is assumed they support regs.
  * If call backs want to use regs, they must either check for regs
- * being NULL, or ARCH_SUPPORTS_FTRACE_SAVE_REGS.
- * Note, ARCH_SUPPORT_SAVE_REGS expects a full regs to be saved.
+ * being NULL, or CONFIG_DYNAMIC_FTRACE_WITH_REGS.
+ * Note, CONFIG_DYNAMIC_FTRACE_WITH_REGS expects a full regs to be saved.
  * An architecture can pass partial regs with ftrace_ops and still
  * set the ARCH_SUPPORT_FTARCE_OPS.
  */
kernel/trace/trace_selftest.c +1 -1
index 4762316..6c62d58 100644
@@ -568,7 +568,7 @@ trace_selftest_function_regs(void)
 	int ret;
 	int supported = 0;
 
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 	supported = 1;
 #endif
 
Signature · 1 line
1.7.10.4
author maintainer
#11 lore raw

PATCH 11/13 kprobes/x86: Move ftrace-based kprobe code into kprobes-ftrace.c

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Split ftrace-based kprobes code from kprobes, and introduce CONFIG_(HAVE_)KPROBES_ON_FTRACE Kconfig flags.
For the cleanup reason, this also moves kprobe_ftrace check into skip_singlestep.

Cc Ingo Molnar <[email protected]>
Cc Ananth N Mavinakayanahalli <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Frederic Weisbecker <[email protected]>
---
 arch/Kconfig                     |   12 +++++
 arch/x86/Kconfig                 |    1 +
 arch/x86/kernel/Makefile         |    1 +
 arch/x86/kernel/kprobes-common.h |   11 +++++
 arch/x86/kernel/kprobes-ftrace.c |   93 ++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/kprobes.c        |   70 +---------------------------
 include/linux/kprobes.h          |   12 +----
 kernel/kprobes.c                 |    8 ++--
 8 files changed, 125 insertions(+), 83 deletions(-)
 create mode 100644 arch/x86/kernel/kprobes-ftrace.c
arch/Kconfig +12 -0
index 7f8f281..97fb7d0 100644
@@ -76,6 +76,15 @@ config OPTPROBES
 	depends on KPROBES && HAVE_OPTPROBES
 	depends on !PREEMPT
 
+config KPROBES_ON_FTRACE
+	def_bool y
+	depends on KPROBES && HAVE_KPROBES_ON_FTRACE
+	depends on DYNAMIC_FTRACE_WITH_REGS
+	help
+	 If function tracer is enabled and the arch supports full
+	 passing of pt_regs to function tracing, then kprobes can
+	 optimize on top of function tracing.
+
 config UPROBES
 	bool "Transparent user-space probes (EXPERIMENTAL)"
 	depends on UPROBE_EVENT && PERF_EVENTS
@@ -158,6 +167,9 @@ config HAVE_KRETPROBES
 config HAVE_OPTPROBES
 	bool
 
+config HAVE_KPROBES_ON_FTRACE
+	bool
+
 config HAVE_NMI_WATCHDOG
 	bool
 #
arch/x86/Kconfig +1 -0
index 996ccec..be8b2b3 100644
@@ -40,6 +40,7 @@ config X86
 	select HAVE_DMA_CONTIGUOUS if !SWIOTLB
 	select HAVE_KRETPROBES
 	select HAVE_OPTPROBES
+	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_FENTRY if X86_64
 	select HAVE_C_RECORDMCOUNT
arch/x86/kernel/Makefile +1 -0
index 34e923a..cc5d31f 100644
@@ -67,6 +67,7 @@ obj-$(CONFIG_KEXEC)		+= relocate_kernel_$(BITS).o crash.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump_$(BITS).o
 obj-$(CONFIG_KPROBES)		+= kprobes.o
 obj-$(CONFIG_OPTPROBES)		+= kprobes-opt.o
+obj-$(CONFIG_KPROBES_ON_FTRACE)	+= kprobes-ftrace.o
 obj-$(CONFIG_MODULES)		+= module.o
 obj-$(CONFIG_DOUBLEFAULT) 	+= doublefault_32.o
 obj-$(CONFIG_KGDB)		+= kgdb.o
arch/x86/kernel/kprobes-common.h +11 -0
index 3230b68..2e9d4b5 100644
@@ -99,4 +99,15 @@ static inline unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsig
 	return addr;
 }
 #endif
+
+#ifdef CONFIG_KPROBES_ON_FTRACE
+extern int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
+			   struct kprobe_ctlblk *kcb);
+#else
+static inline int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
+				  struct kprobe_ctlblk *kcb)
+{
+	return 0;
+}
+#endif
 #endif
arch/x86/kernel/kprobes-ftrace.c new file +93 -0

93 lines not on this page · raw message

arch/x86/kernel/kprobes.c +2 -68

88 lines not on this page · raw message

include/linux/kprobes.h +1 -11
index 23755ba..4b6ef4d 100644
@@ -49,16 +49,6 @@
 #define KPROBE_REENTER		0x00000004
 #define KPROBE_HIT_SSDONE	0x00000008
 
-/*
- * If function tracer is enabled and the arch supports full
- * passing of pt_regs to function tracing, then kprobes can
- * optimize on top of function tracing.
- */
-#if defined(CONFIG_FUNCTION_TRACER) && defined(ARCH_SUPPORTS_FTRACE_SAVE_REGS) \
-	&& defined(ARCH_SUPPORTS_KPROBES_ON_FTRACE)
-# define KPROBES_CAN_USE_FTRACE
-#endif
-
 /* Attach to insert probes on any functions which should be ignored*/
 #define __kprobes	__attribute__((__section__(".kprobes.text")))
 
@@ -316,7 +306,7 @@ extern int proc_kprobes_optimization_handler(struct ctl_table *table,
 #endif
 
 #endif /* CONFIG_OPTPROBES */
-#ifdef KPROBES_CAN_USE_FTRACE
+#ifdef CONFIG_KPROBES_ON_FTRACE
 extern void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 				  struct ftrace_ops *ops, struct pt_regs *regs);
 extern int arch_prepare_kprobe_ftrace(struct kprobe *p);
kernel/kprobes.c +4 -4
index 098f396..f423c3e 100644
@@ -919,7 +919,7 @@ static __kprobes struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
 }
 #endif /* CONFIG_OPTPROBES */
 
-#ifdef KPROBES_CAN_USE_FTRACE
+#ifdef CONFIG_KPROBES_ON_FTRACE
 static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
 	.func = kprobe_ftrace_handler,
 	.flags = FTRACE_OPS_FL_SAVE_REGS,
@@ -964,7 +964,7 @@ static void __kprobes disarm_kprobe_ftrace(struct kprobe *p)
 			   (unsigned long)p->addr, 1, 0);
 	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
 }
-#else	/* !KPROBES_CAN_USE_FTRACE */
+#else	/* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)	arch_prepare_kprobe(p)
 #define arm_kprobe_ftrace(p)	do {} while (0)
 #define disarm_kprobe_ftrace(p)	do {} while (0)
@@ -1414,12 +1414,12 @@ static __kprobes int check_kprobe_address_safe(struct kprobe *p,
 	 */
 	ftrace_addr = ftrace_location((unsigned long)p->addr);
 	if (ftrace_addr) {
-#ifdef KPROBES_CAN_USE_FTRACE
+#ifdef CONFIG_KPROBES_ON_FTRACE
 		/* Given address is not on the instruction boundary */
 		if ((unsigned long)p->addr != ftrace_addr)
 			return -EILSEQ;
 		p->flags |= KPROBE_FLAG_FTRACE;
-#else	/* !KPROBES_CAN_USE_FTRACE */
+#else	/* !CONFIG_KPROBES_ON_FTRACE */
 		return -EINVAL;
 #endif
 	}
Signature · 1 line
1.7.10.4
author maintainer
#12 lore raw

PATCH 12/13 kprobes/x86: Move kprobes stuff under arch/x86/kernel/kprobes/

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Move arch-dep kprobes stuff under arch/x86/kernel/kprobes.

Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ananth N Mavinakayanahalli <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
[ fixed whitespace and s/__attribute__((packed))/__packed/ ]
---
 arch/x86/kernel/Makefile         |    4 +-
 arch/x86/kernel/kprobes-common.h |  113 ----
 arch/x86/kernel/kprobes-ftrace.c |   93 ----
 arch/x86/kernel/kprobes-opt.c    |  512 ------------------
 arch/x86/kernel/kprobes.c        | 1064 --------------------------------------
 arch/x86/kernel/kprobes/Makefile |    7 +
 arch/x86/kernel/kprobes/common.h |  113 ++++
 arch/x86/kernel/kprobes/core.c   | 1064 ++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/kprobes/ftrace.c |   93 ++++
 arch/x86/kernel/kprobes/opt.c    |  512 ++++++++++++++++++
 10 files changed, 1790 insertions(+), 1785 deletions(-)
 delete mode 100644 arch/x86/kernel/kprobes-common.h
 delete mode 100644 arch/x86/kernel/kprobes-ftrace.c
 delete mode 100644 arch/x86/kernel/kprobes-opt.c
 delete mode 100644 arch/x86/kernel/kprobes.c
 create mode 100644 arch/x86/kernel/kprobes/Makefile
 create mode 100644 arch/x86/kernel/kprobes/common.h
 create mode 100644 arch/x86/kernel/kprobes/core.c
 create mode 100644 arch/x86/kernel/kprobes/ftrace.c
 create mode 100644 arch/x86/kernel/kprobes/opt.c
arch/x86/kernel/Makefile +1 -3
index cc5d31f..ac3b3d0 100644
@@ -65,9 +65,7 @@ obj-$(CONFIG_X86_TSC)		+= trace_clock.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec_$(BITS).o
 obj-$(CONFIG_KEXEC)		+= relocate_kernel_$(BITS).o crash.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump_$(BITS).o
-obj-$(CONFIG_KPROBES)		+= kprobes.o
-obj-$(CONFIG_OPTPROBES)		+= kprobes-opt.o
-obj-$(CONFIG_KPROBES_ON_FTRACE)	+= kprobes-ftrace.o
+obj-y				+= kprobes/
 obj-$(CONFIG_MODULES)		+= module.o
 obj-$(CONFIG_DOUBLEFAULT) 	+= doublefault_32.o
 obj-$(CONFIG_KGDB)		+= kgdb.o
arch/x86/kernel/kprobes-common.h deleted +0 -113

113 lines not on this page · raw message

arch/x86/kernel/kprobes-ftrace.c deleted +0 -93

93 lines not on this page · raw message

arch/x86/kernel/kprobes-opt.c deleted +0 -512

512 lines not on this page · raw message

arch/x86/kernel/kprobes.c deleted +0 -1064

1,064 lines not on this page · raw message

arch/x86/kernel/kprobes/Makefile new file +7 -0
new file mode 100644
index 0000000..0d33169
@@ -0,0 +1,7 @@
+#
+# Makefile for kernel probes
+#
+
+obj-$(CONFIG_KPROBES)		+= core.o
+obj-$(CONFIG_OPTPROBES)		+= opt.o
+obj-$(CONFIG_KPROBES_ON_FTRACE)	+= ftrace.o
arch/x86/kernel/kprobes/common.h new file +113 -0

113 lines not on this page · raw message

arch/x86/kernel/kprobes/core.c new file +1064 -0

1,064 lines not on this page · raw message

arch/x86/kernel/kprobes/ftrace.c new file +93 -0

24 lines not on this page · raw message

arch/x86/kernel/kprobes/opt.c new file +512 -0

512 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line
1.7.10.4

Part of the diff is not shown here. View the raw message.

author maintainer
#13 lore raw

PATCH 13/13 tracing: Remove the extra 4 bytes of padding in events

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Due to a userspace issue with PowerTop v1, which hardcoded the offset of event fields that it was using, it broke when we removed the Big Kernel Lock counter from the event header.

(commit e6e1e2593 "tracing: Remove lock_depth from event entry")

Because this broke userspace, it was determined that we must keep those 4 bytes around.

(commit a3a4a5acd "Regression: partial revert "tracing: Remove lock_depth from event entry"")

This unfortunately wastes space in the ring buffer. 4 bytes per event, where a lot of events are just 24 bytes. That's 16% of the buffer wasted. A million events will add 4 megs of white space into the buffer.

It was later noticed that PowerTop v1 could not work on systems where the kernel was 64 bit but the userspace was 32 bits. The reason was because the offsets are different between the two and the hard coded offset of one would not work with the other.

With PowerTop v2, it implemented the same interface that both perf and trace-cmd use. That is, it reads the format file of the event to find the offsets of the fields it needs. This fixes the problem with running powertop on a 32 bit userspace running on a 64 bit kernel. It also no longer requires the 4 byte padding.

As PowerTop v2 has been out for a while, and is included in all major distributions, it is time that we can safely remove the 4 bytes of padding. Users of PowerTop v1 should upgrade to PowerTop v2.

Cc Arjan van de Ven <[email protected]>
Cc Linus Torvalds <[email protected]>
---
 include/linux/ftrace_event.h |    1 -
 kernel/trace/trace.c         |    1 -
 kernel/trace/trace_events.c  |    1 -
 3 files changed, 3 deletions(-)
include/linux/ftrace_event.h +0 -1
index 43ef8b6..6f8d0b7 100644
@@ -49,7 +49,6 @@ struct trace_entry {
 	unsigned char		flags;
 	unsigned char		preempt_count;
 	int			pid;
-	int			padding;
 };
 
 #define FTRACE_MAX_EVENT						\
kernel/trace/trace.c +0 -1
index d62248d..a387bd2 100644
@@ -1173,7 +1173,6 @@ tracing_generic_entry_update(struct trace_entry *entry, unsigned long flags,
 
 	entry->preempt_count		= pc & 0xff;
 	entry->pid			= (tsk) ? tsk->pid : 0;
-	entry->padding			= 0;
 	entry->flags =
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
 		(irqs_disabled_flags(flags) ? TRACE_FLAG_IRQS_OFF : 0) |
kernel/trace/trace_events.c +0 -1
index 880073d..57e9b28 100644
@@ -116,7 +116,6 @@ static int trace_define_common_fields(void)
 	__common_field(unsigned char, flags);
 	__common_field(unsigned char, preempt_count);
 	__common_field(int, pid);
-	__common_field(int, padding);
 
 	return ret;
 }
Signature · 1 line
1.7.10.4
Acked-by on v1 13/13
#14 lore raw

Re: [PATCH 13/13 -next] tracing: Remove the extra 4 bytes of padding in events

Quoted 29 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Due to a userspace issue with PowerTop v1, which hardcoded the offset of event fields that it was using, it broke when we removed the Big Kernel Lock counter from the event header.

(commit e6e1e2593 "tracing: Remove lock_depth from event entry")

Because this broke userspace, it was determined that we must keep those 4 bytes around.

(commit a3a4a5acd "Regression: partial revert "tracing: Remove lock_depth from event entry"")

This unfortunately wastes space in the ring buffer. 4 bytes per event, where a lot of events are just 24 bytes. That's 16% of the buffer wasted. A million events will add 4 megs of white space into the buffer.

It was later noticed that PowerTop v1 could not work on systems where the kernel was 64 bit but the userspace was 32 bits. The reason was because the offsets are different between the two and the hard coded offset of one would not work with the other.

With PowerTop v2, it implemented the same interface that both perf and trace-cmd use. That is, it reads the format file of the event to find the offsets of the fields it needs. This fixes the problem with running powertop on a 32 bit userspace running on a 64 bit kernel. It also no longer requires the 4 byte padding.

Quoted 6 lines

As PowerTop v2 has been out for a while, and is included in all major distributions, it is time that we can safely remove the 4 bytes of padding. Users of PowerTop v1 should upgrade to PowerTop v2.

Cc Arjan van de Ven <[email protected]>

this only impacts the users of the 2.0beta series (which, while arguably staying around for too long, is now very very obsolete and replaced by the final in all distros that I'm aware of)