[for-next][PATCH 00/20] tracing: linux-next updates

[for-next][PATCH 00/20] tracing: linux-next updates

[Steven Rostedt]

This is mostly clean ups. There's code to shrink the foot print of
tracepoints quite a bit. Fixes for hard to hit error paths. Fixes
to TRACE_EVENT dynamic array handling for newer events coming.
Handling of trace module taint failure.


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

Head SHA1: 7f11f5ecf4ae09815dc2de267c5e04d1de01d862


Filipe Brandenburger (2):
      tracing: Correctly expand len expressions from __dynamic_array macro
      tracing: Evaluate len expression only once in __dynamic_array macro

Jiri Slaby (5):
      ftrace: Cleanup of global variables ftrace_new_pgs and ftrace_update_cnt
      ftrace: Inline the code from ftrace_dyn_table_alloc()
      ftrace: Pass retval through return in ftrace_dyn_arch_init()
      ftrace: Do not pass data to ftrace_dyn_arch_init
      ftrace: Remove freelist from struct dyn_ftrace

Petr Mladek (3):
      ftrace/x86: One more missing sync after fixup of function modification failure
      ftrace: Warn on error when modifying ftrace function
      ftrace/x86: BUG when ftrace recovery fails

Steven Rostedt (4):
      tracing: Move raw output code from macro to standalone function
      tracing: Move event storage for array from macro to standalone function
      tracing: Use helper functions in event assignment to shrink macro size
      tracing: Warn if a tracepoint is not set via debugfs

Steven Rostedt (Red Hat) (6):
      ftrace/x86: Run a sync after fixup on failure
      tracepoint: Do not waste memory on mods with no tracepoints
      ftrace/x86: Have ftrace_write() return -EPERM and clean up callers
      tracing: Fix event header writeback.h to include tracepoint.h
      tracing: Fix event header migrate.h to include tracepoint.h
      tracing/module: Replace include of tracepoint.h with jump_label.h in module.h

----
 Documentation/trace/ftrace-design.txt |  5 +--
 arch/arm/kernel/ftrace.c              |  4 +-
 arch/blackfin/kernel/ftrace.c         |  5 +--
 arch/ia64/kernel/ftrace.c             |  4 +-
 arch/metag/kernel/ftrace.c            |  5 +--
 arch/microblaze/kernel/ftrace.c       |  5 +--
 arch/mips/kernel/ftrace.c             |  5 +--
 arch/powerpc/kernel/ftrace.c          |  7 +---
 arch/s390/kernel/ftrace.c             |  3 +-
 arch/sh/kernel/ftrace.c               |  5 +--
 arch/sparc/kernel/ftrace.c            |  6 +--
 arch/tile/kernel/ftrace.c             |  4 +-
 arch/x86/kernel/ftrace.c              | 55 ++++++++++++------------
 include/linux/ftrace.h                |  9 ++--
 include/linux/ftrace_event.h          | 28 +++++++++++--
 include/linux/module.h                |  2 +-
 include/trace/events/migrate.h        |  2 +
 include/trace/events/writeback.h      |  1 +
 include/trace/ftrace.h                | 50 +++++++---------------
 kernel/trace/ftrace.c                 | 78 +++++++++++++----------------------
 kernel/trace/trace_events.c           | 36 +++++++++++++---
 kernel/trace/trace_export.c           | 12 ++----
 kernel/trace/trace_output.c           | 52 +++++++++++++++++++++++
 kernel/tracepoint.c                   | 17 +++++++-
 24 files changed, 216 insertions(+), 184 deletions(-)

[for-next][PATCH 01/20] ftrace/x86: Run a sync after fixup on failure

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-x86-Run-a-sync-after-fixup-on-failure.patch --]
[-- Type: text/plain, Size: 2741 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

If a failure occurs while enabling a trace, it bails out and will remove
the tracepoints to be back to what the code originally was. But the fix
up had some bugs in it. By injecting a failure in the code, the fix up
ran to completion, but shortly afterward the system rebooted.

There was two bugs here.

The first was that there was no final sync run across the CPUs after the
fix up was done, and before the ftrace int3 handler flag was reset. That
means that other CPUs could still see the breakpoint and trigger on it
long after the flag was cleared, and the int3 handler would think it was
a spurious interrupt. Worse yet, the int3 handler could hit other breakpoints
because the ftrace int3 handler flag would have prevented the int3 handler
from going further.

Here's a description of the issue:

	CPU0				CPU1
	----				----
  remove_breakpoint();
  modifying_ftrace_code = 0;

				[still sees breakpoint]
				<takes trap>
				[sees modifying_ftrace_code as zero]
				[no breakpoint handler]
				[goto failed case]
				[trap exception - kernel breakpoint, no
				 handler]
				BUG()

The second bug was that the removal of the breakpoints required the
"within()" logic updates instead of accessing the ip address directly.
As the kernel text is mapped read-only when CONFIG_DEBUG_RODATA is set, and
the removal of the breakpoint is a modification of the kernel text.
The ftrace_write() includes the "within()" logic, where as, the
probe_kernel_write() does not. This prevented the breakpoint from being
removed at all.

Link: http://lkml.kernel.org/r/1392650573-3390-1-git-send-email-pmladek@suse.cz

Reported-by: Petr Mladek <pmladek@suse.cz>
Tested-by: Petr Mladek <pmladek@suse.cz>
Acked-by: H. Peter Anvin <hpa@linux.intel.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index e625319..6b566c8 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -455,7 +455,7 @@ static int remove_breakpoint(struct dyn_ftrace *rec)
 	}
 
  update:
-	return probe_kernel_write((void *)ip, &nop[0], 1);
+	return ftrace_write(ip, nop, 1);
 }
 
 static int add_update_code(unsigned long ip, unsigned const char *new)
@@ -634,6 +634,7 @@ void ftrace_replace_code(int enable)
 		rec = ftrace_rec_iter_record(iter);
 		remove_breakpoint(rec);
 	}
+	run_sync();
 }
 
 static int
@@ -664,7 +665,7 @@ ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 	return ret;
 
  fail_update:
-	probe_kernel_write((void *)ip, &old_code[0], 1);
+	ftrace_write(ip, old_code, 1);
 	goto out;
 }
 
-- 
1.8.5.3

[for-next][PATCH 02/20] ftrace/x86: One more missing sync after fixup of function modification failure

[Steven Rostedt]

[-- Attachment #1: 0002-ftrace-x86-One-more-missing-sync-after-fixup-of-func.patch --]
[-- Type: text/plain, Size: 1431 bytes --]

From: Petr Mladek <pmladek@suse.cz>

If a failure occurs while modifying ftrace function, it bails out and will
remove the tracepoints to be back to what the code originally was.

There is missing the final sync run across the CPUs after the fix up is done
and before the ftrace int3 handler flag is reset.

Here's the description of the problem:

	CPU0				CPU1
	----				----
  remove_breakpoint();
  modifying_ftrace_code = 0;

				[still sees breakpoint]
				<takes trap>
				[sees modifying_ftrace_code as zero]
				[no breakpoint handler]
				[goto failed case]
				[trap exception - kernel breakpoint, no
				 handler]
				BUG()

Link: http://lkml.kernel.org/r/1393258342-29978-2-git-send-email-pmladek@suse.cz

Fixes: 8a4d0a687a5 "ftrace: Use breakpoint method to update ftrace caller"
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: H. Peter Anvin <hpa@linux.intel.com>
Signed-off-by: Petr Mladek <pmladek@suse.cz>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 6b566c8..69885e2 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -660,8 +660,8 @@ ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 		ret = -EPERM;
 		goto out;
 	}
-	run_sync();
  out:
+	run_sync();
 	return ret;
 
  fail_update:
-- 
1.8.5.3

[for-next][PATCH 03/20] tracepoint: Do not waste memory on mods with no tracepoints

[Steven Rostedt]

[-- Attachment #1: 0003-tracepoint-Do-not-waste-memory-on-mods-with-no-trace.patch --]
[-- Type: text/plain, Size: 1177 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

No reason to allocate tp_module structures for modules that have no
tracepoints. This just wastes memory.

Fixes: b75ef8b44b1c "Tracepoint: Dissociate from module mutex"
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/tracepoint.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 29f2654..0d4ef26 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -636,6 +636,9 @@ static int tracepoint_module_coming(struct module *mod)
 	struct tp_module *tp_mod, *iter;
 	int ret = 0;
 
+	if (!mod->num_tracepoints)
+		return 0;
+
 	/*
 	 * We skip modules that taint the kernel, especially those with different
 	 * module headers (for forced load), to make sure we don't cause a crash.
@@ -679,6 +682,9 @@ static int tracepoint_module_going(struct module *mod)
 {
 	struct tp_module *pos;
 
+	if (!mod->num_tracepoints)
+		return 0;
+
 	mutex_lock(&tracepoints_mutex);
 	tracepoint_update_probe_range(mod->tracepoints_ptrs,
 		mod->tracepoints_ptrs + mod->num_tracepoints);
-- 
1.8.5.3

[for-next][PATCH 04/20] ftrace/x86: Have ftrace_write() return -EPERM and clean up callers

[Steven Rostedt]

[-- Attachment #1: 0004-ftrace-x86-Have-ftrace_write-return-EPERM-and-clean-.patch --]
[-- Type: text/plain, Size: 2632 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Having ftrace_write() return -EPERM on failure, as that's what the callers
return, then we can clean up the code a bit. That is, instead of:

  if (ftrace_write(...))
     return -EPERM;
  return 0;

or

  if (ftrace_write(...)) {
     ret = -EPERM;
     goto_out;
  }

We can instead have:

  return ftrace_write(...);

or

  ret = ftrace_write(...);
  if (ret)
    goto out;

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 69885e2..8cabf63 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -308,7 +308,10 @@ static int ftrace_write(unsigned long ip, const char *val, int size)
 	if (within(ip, (unsigned long)_text, (unsigned long)_etext))
 		ip = (unsigned long)__va(__pa_symbol(ip));
 
-	return probe_kernel_write((void *)ip, val, size);
+	if (probe_kernel_write((void *)ip, val, size))
+		return -EPERM;
+
+	return 0;
 }
 
 static int add_break(unsigned long ip, const char *old)
@@ -323,10 +326,7 @@ static int add_break(unsigned long ip, const char *old)
 	if (memcmp(replaced, old, MCOUNT_INSN_SIZE) != 0)
 		return -EINVAL;
 
-	if (ftrace_write(ip, &brk, 1))
-		return -EPERM;
-
-	return 0;
+	return ftrace_write(ip, &brk, 1);
 }
 
 static int add_brk_on_call(struct dyn_ftrace *rec, unsigned long addr)
@@ -463,9 +463,7 @@ static int add_update_code(unsigned long ip, unsigned const char *new)
 	/* skip breakpoint */
 	ip++;
 	new++;
-	if (ftrace_write(ip, new, MCOUNT_INSN_SIZE - 1))
-		return -EPERM;
-	return 0;
+	return ftrace_write(ip, new, MCOUNT_INSN_SIZE - 1);
 }
 
 static int add_update_call(struct dyn_ftrace *rec, unsigned long addr)
@@ -520,10 +518,7 @@ static int finish_update_call(struct dyn_ftrace *rec, unsigned long addr)
 
 	new = ftrace_call_replace(ip, addr);
 
-	if (ftrace_write(ip, new, 1))
-		return -EPERM;
-
-	return 0;
+	return ftrace_write(ip, new, 1);
 }
 
 static int finish_update_nop(struct dyn_ftrace *rec)
@@ -533,9 +528,7 @@ static int finish_update_nop(struct dyn_ftrace *rec)
 
 	new = ftrace_nop_replace();
 
-	if (ftrace_write(ip, new, 1))
-		return -EPERM;
-	return 0;
+	return ftrace_write(ip, new, 1);
 }
 
 static int finish_update(struct dyn_ftrace *rec, int enable)
@@ -656,10 +649,6 @@ ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 	run_sync();
 
 	ret = ftrace_write(ip, new_code, 1);
-	if (ret) {
-		ret = -EPERM;
-		goto out;
-	}
  out:
 	run_sync();
 	return ret;
-- 
1.8.5.3

[for-next][PATCH 05/20] tracing: Move raw output code from macro to standalone function

[Steven Rostedt]

[-- Attachment #1: 0005-tracing-Move-raw-output-code-from-macro-to-standalon.patch --]
[-- Type: text/plain, Size: 4286 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The code for trace events to format the raw recorded event data
into human readable format in the 'trace' file is repeated for every
event in the system. When you have over 500 events, this can add up
quite a bit.

By making helper functions in the core kernel to do the work
instead, we can shrink the size of the kernel down a bit.

With a kernel configured with 502 events, the change in size was:

   text    data     bss     dec     hex filename
12991007        1913568 9785344 24689919        178bcff /tmp/vmlinux.orig
12990946        1913568 9785344 24689858        178bcc2 /tmp/vmlinux.patched

Note, this version does not save as much as the version of this patch
I had a few years ago. That is because in the mean time, commit
f71130de5c7f ("tracing: Add a helper function for event print functions")
did a lot of the work my original patch did. But this change helps
slightly, and is part of a larger clean up to reduce the size much further.

Link: http://lkml.kernel.org/r/20120810034707.378538034@goodmis.org

Cc: Li Zefan <lizefan@huawei.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace_event.h |  5 +++++
 include/trace/ftrace.h       | 10 +---------
 kernel/trace/trace_output.c  | 31 +++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 4e4cc28..a91ab93 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -163,6 +163,8 @@ void trace_current_buffer_discard_commit(struct ring_buffer *buffer,
 
 void tracing_record_cmdline(struct task_struct *tsk);
 
+int ftrace_output_call(struct trace_iterator *iter, char *name, char *fmt, ...);
+
 struct event_filter;
 
 enum trace_reg {
@@ -197,6 +199,9 @@ struct ftrace_event_class {
 extern int ftrace_event_reg(struct ftrace_event_call *event,
 			    enum trace_reg type, void *data);
 
+int ftrace_output_event(struct trace_iterator *iter, struct ftrace_event_call *event,
+			char *fmt, ...);
+
 enum {
 	TRACE_EVENT_FL_FILTERED_BIT,
 	TRACE_EVENT_FL_CAP_ANY_BIT,
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 1a8b28d..3873d6e 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -265,11 +265,9 @@ static notrace enum print_line_t					\
 ftrace_raw_output_##call(struct trace_iterator *iter, int flags,	\
 			 struct trace_event *event)			\
 {									\
-	struct trace_seq *s = &iter->seq;				\
 	struct ftrace_raw_##template *field;				\
 	struct trace_entry *entry;					\
 	struct trace_seq *p = &iter->tmp_seq;				\
-	int ret;							\
 									\
 	entry = iter->ent;						\
 									\
@@ -281,13 +279,7 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags,	\
 	field = (typeof(field))entry;					\
 									\
 	trace_seq_init(p);						\
-	ret = trace_seq_printf(s, "%s: ", #call);			\
-	if (ret)							\
-		ret = trace_seq_printf(s, print);			\
-	if (!ret)							\
-		return TRACE_TYPE_PARTIAL_LINE;				\
-									\
-	return TRACE_TYPE_HANDLED;					\
+	return ftrace_output_call(iter, #call, print);			\
 }									\
 static struct trace_event_functions ftrace_event_type_funcs_##call = {	\
 	.trace			= ftrace_raw_output_##call,		\
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index ed32284..ca0e79e2 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -439,6 +439,37 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
 }
 EXPORT_SYMBOL(ftrace_raw_output_prep);
 
+static int ftrace_output_raw(struct trace_iterator *iter, char *name,
+			     char *fmt, va_list ap)
+{
+	struct trace_seq *s = &iter->seq;
+	int ret;
+
+	ret = trace_seq_printf(s, "%s: ", name);
+	if (!ret)
+		return TRACE_TYPE_PARTIAL_LINE;
+
+	ret = trace_seq_vprintf(s, fmt, ap);
+
+	if (!ret)
+		return TRACE_TYPE_PARTIAL_LINE;
+
+	return TRACE_TYPE_HANDLED;
+}
+
+int ftrace_output_call(struct trace_iterator *iter, char *name, char *fmt, ...)
+{
+	va_list ap;
+	int ret;
+
+	va_start(ap, fmt);
+	ret = ftrace_output_raw(iter, name, fmt, ap);
+	va_end(ap);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ftrace_output_call);
+
 #ifdef CONFIG_KRETPROBES
 static inline const char *kretprobed(const char *name)
 {
-- 
1.8.5.3

[for-next][PATCH 06/20] tracing: Move event storage for array from macro to standalone function

[Steven Rostedt]

[-- Attachment #1: 0006-tracing-Move-event-storage-for-array-from-macro-to-s.patch --]
[-- Type: text/plain, Size: 5739 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The code that shows array fields for events is defined for all events.
This can add up quite a bit when you have over 500 events.

By making helper functions in the core kernel to do the work
instead, we can shrink the size of the kernel down a bit.

With a kernel configured with 502 events, the change in size was:

   text    data     bss     dec     hex filename
12990946        1913568 9785344 24689858        178bcc2 /tmp/vmlinux
12987390        1913504 9785344 24686238        178ae9e /tmp/vmlinux.patched

That's a total of 3556 bytes, which comes down to 7 bytes per event.
Although it's not much, this code is just called at initialization of
the events.

Link: http://lkml.kernel.org/r/20120810034708.084036335@goodmis.org

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace_event.h |  8 ++++----
 include/trace/ftrace.h       | 12 ++++--------
 kernel/trace/trace_events.c  |  6 ------
 kernel/trace/trace_export.c  | 12 ++++--------
 kernel/trace/trace_output.c  | 21 +++++++++++++++++++++
 5 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index a91ab93..ffe642e 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -202,6 +202,10 @@ extern int ftrace_event_reg(struct ftrace_event_call *event,
 int ftrace_output_event(struct trace_iterator *iter, struct ftrace_event_call *event,
 			char *fmt, ...);
 
+int ftrace_event_define_field(struct ftrace_event_call *call,
+			      char *type, int len, char *item, int offset,
+			      int field_size, int sign, int filter);
+
 enum {
 	TRACE_EVENT_FL_FILTERED_BIT,
 	TRACE_EVENT_FL_CAP_ANY_BIT,
@@ -500,10 +504,6 @@ enum {
 	FILTER_TRACE_FN,
 };
 
-#define EVENT_STORAGE_SIZE 128
-extern struct mutex event_storage_mutex;
-extern char event_storage[EVENT_STORAGE_SIZE];
-
 extern int trace_event_raw_init(struct ftrace_event_call *call);
 extern int trace_define_field(struct ftrace_event_call *call, const char *type,
 			      const char *name, int offset, int size,
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 3873d6e..54928fa 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -302,15 +302,11 @@ static struct trace_event_functions ftrace_event_type_funcs_##call = {	\
 #undef __array
 #define __array(type, item, len)					\
 	do {								\
-		mutex_lock(&event_storage_mutex);			\
 		BUILD_BUG_ON(len > MAX_FILTER_STR_VAL);			\
-		snprintf(event_storage, sizeof(event_storage),		\
-			 "%s[%d]", #type, len);				\
-		ret = trace_define_field(event_call, event_storage, #item, \
-				 offsetof(typeof(field), item),		\
-				 sizeof(field.item),			\
-				 is_signed_type(type), FILTER_OTHER);	\
-		mutex_unlock(&event_storage_mutex);			\
+		ret = ftrace_event_define_field(event_call, #type, len,	\
+				#item, offsetof(typeof(field), item),   \
+				sizeof(field.item),			\
+			 	is_signed_type(type), FILTER_OTHER); \
 		if (ret)						\
 			return ret;					\
 	} while (0);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index e71ffd4..22826c7 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -27,12 +27,6 @@
 
 DEFINE_MUTEX(event_mutex);
 
-DEFINE_MUTEX(event_storage_mutex);
-EXPORT_SYMBOL_GPL(event_storage_mutex);
-
-char event_storage[EVENT_STORAGE_SIZE];
-EXPORT_SYMBOL_GPL(event_storage);
-
 LIST_HEAD(ftrace_events);
 static LIST_HEAD(ftrace_common_fields);
 
diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c
index 7c3e3e7..39c746c 100644
--- a/kernel/trace/trace_export.c
+++ b/kernel/trace/trace_export.c
@@ -96,14 +96,10 @@ static void __always_unused ____ftrace_check_##name(void)		\
 #define __array(type, item, len)					\
 	do {								\
 		BUILD_BUG_ON(len > MAX_FILTER_STR_VAL);			\
-		mutex_lock(&event_storage_mutex);			\
-		snprintf(event_storage, sizeof(event_storage),		\
-			 "%s[%d]", #type, len);				\
-		ret = trace_define_field(event_call, event_storage, #item, \
-				 offsetof(typeof(field), item),		\
-				 sizeof(field.item),			\
-				 is_signed_type(type), filter_type);	\
-		mutex_unlock(&event_storage_mutex);			\
+		ret = ftrace_event_define_field(event_call, #type, len,	\
+				#item, offsetof(typeof(field), item),   \
+				sizeof(field.item),			\
+			 	is_signed_type(type), filter_type);	\
 		if (ret)						\
 			return ret;					\
 	} while (0);
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index ca0e79e2..ee8d748 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -20,6 +20,10 @@ static struct hlist_head event_hash[EVENT_HASHSIZE] __read_mostly;
 
 static int next_event_type = __TRACE_LAST_TYPE + 1;
 
+#define EVENT_STORAGE_SIZE 128
+static DEFINE_MUTEX(event_storage_mutex);
+static char event_storage[EVENT_STORAGE_SIZE];
+
 int trace_print_seq(struct seq_file *m, struct trace_seq *s)
 {
 	int len = s->len >= PAGE_SIZE ? PAGE_SIZE - 1 : s->len;
@@ -470,6 +474,23 @@ int ftrace_output_call(struct trace_iterator *iter, char *name, char *fmt, ...)
 }
 EXPORT_SYMBOL_GPL(ftrace_output_call);
 
+int ftrace_event_define_field(struct ftrace_event_call *call,
+			      char *type, int len, char *item, int offset,
+			      int field_size, int sign, int filter)
+{
+	int ret;
+
+	mutex_lock(&event_storage_mutex);
+	snprintf(event_storage, sizeof(event_storage),
+		 "%s[%d]", type, len);
+	ret = trace_define_field(call, event_storage, item, offset,
+				 field_size, sign, filter);
+	mutex_unlock(&event_storage_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ftrace_event_define_field);
+
 #ifdef CONFIG_KRETPROBES
 static inline const char *kretprobed(const char *name)
 {
-- 
1.8.5.3

[for-next][PATCH 07/20] tracing: Use helper functions in event assignment to shrink macro size

[Steven Rostedt]

[-- Attachment #1: 0007-tracing-Use-helper-functions-in-event-assignment-to-.patch --]
[-- Type: text/plain, Size: 5079 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The functions that assign the contents for the ftrace events are
defined by the TRACE_EVENT() macros. Each event has its own unique
way to assign data to its buffer. When you have over 500 events,
that means there's 500 functions assigning data uniquely for each
event (not really that many, as DECLARE_EVENT_CLASS() and multiple
DEFINE_EVENT()s will only need a single function).

By making helper functions in the core kernel to do some of the work
instead, we can shrink the size of the kernel down a bit.

With a kernel configured with 502 events, the change in size was:

   text    data     bss     dec     hex filename
12987390        1913504 9785344 24686238        178ae9e /tmp/vmlinux
12959102        1913504 9785344 24657950        178401e /tmp/vmlinux.patched

That's a total of 28288 bytes, which comes down to 56 bytes per event.

Link: http://lkml.kernel.org/r/20120810034708.370808175@goodmis.org

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace_event.h | 15 +++++++++++++++
 include/trace/ftrace.h       | 22 ++++++----------------
 kernel/trace/trace_events.c  | 30 ++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index ffe642e..9d3fe06 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -206,6 +206,21 @@ int ftrace_event_define_field(struct ftrace_event_call *call,
 			      char *type, int len, char *item, int offset,
 			      int field_size, int sign, int filter);
 
+struct ftrace_event_buffer {
+	struct ring_buffer		*buffer;
+	struct ring_buffer_event	*event;
+	struct ftrace_event_file	*ftrace_file;
+	void				*entry;
+	unsigned long			flags;
+	int				pc;
+};
+
+void *ftrace_event_buffer_reserve(struct ftrace_event_buffer *fbuffer,
+				  struct ftrace_event_file *ftrace_file,
+				  unsigned long len);
+
+void ftrace_event_buffer_commit(struct ftrace_event_buffer *fbuffer);
+
 enum {
 	TRACE_EVENT_FL_FILTERED_BIT,
 	TRACE_EVENT_FL_CAP_ANY_BIT,
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 54928fa..1cc2265 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -532,37 +532,27 @@ static notrace void							\
 ftrace_raw_event_##call(void *__data, proto)				\
 {									\
 	struct ftrace_event_file *ftrace_file = __data;			\
-	struct ftrace_event_call *event_call = ftrace_file->event_call;	\
 	struct ftrace_data_offsets_##call __maybe_unused __data_offsets;\
-	struct ring_buffer_event *event;				\
+	struct ftrace_event_buffer fbuffer;				\
 	struct ftrace_raw_##call *entry;				\
-	struct ring_buffer *buffer;					\
-	unsigned long irq_flags;					\
 	int __data_size;						\
-	int pc;								\
 									\
 	if (ftrace_trigger_soft_disabled(ftrace_file))			\
 		return;							\
 									\
-	local_save_flags(irq_flags);					\
-	pc = preempt_count();						\
-									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
 									\
-	event = trace_event_buffer_lock_reserve(&buffer, ftrace_file,	\
-				 event_call->event.type,		\
-				 sizeof(*entry) + __data_size,		\
-				 irq_flags, pc);			\
-	if (!event)							\
+	entry = ftrace_event_buffer_reserve(&fbuffer, ftrace_file,	\
+				 sizeof(*entry) + __data_size);		\
+									\
+	if (!entry)							\
 		return;							\
-	entry	= ring_buffer_event_data(event);			\
 									\
 	tstruct								\
 									\
 	{ assign; }							\
 									\
-	event_trigger_unlock_commit(ftrace_file, buffer, event, entry, \
-				    irq_flags, pc);		       \
+	ftrace_event_buffer_commit(&fbuffer);				\
 }
 /*
  * The ftrace_test_probe is compiled out, it is only here as a build time check
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 22826c7..b8f73b3 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -188,6 +188,36 @@ int trace_event_raw_init(struct ftrace_event_call *call)
 }
 EXPORT_SYMBOL_GPL(trace_event_raw_init);
 
+void *ftrace_event_buffer_reserve(struct ftrace_event_buffer *fbuffer,
+				  struct ftrace_event_file *ftrace_file,
+				  unsigned long len)
+{
+	struct ftrace_event_call *event_call = ftrace_file->event_call;
+
+	local_save_flags(fbuffer->flags);
+	fbuffer->pc = preempt_count();
+	fbuffer->ftrace_file = ftrace_file;
+
+	fbuffer->event =
+		trace_event_buffer_lock_reserve(&fbuffer->buffer, ftrace_file,
+						event_call->event.type, len,
+						fbuffer->flags, fbuffer->pc);
+	if (!fbuffer->event)
+		return NULL;
+
+	fbuffer->entry = ring_buffer_event_data(fbuffer->event);
+	return fbuffer->entry;
+}
+EXPORT_SYMBOL_GPL(ftrace_event_buffer_reserve);
+
+void ftrace_event_buffer_commit(struct ftrace_event_buffer *fbuffer)
+{
+	event_trigger_unlock_commit(fbuffer->ftrace_file, fbuffer->buffer,
+				    fbuffer->event, fbuffer->entry,
+				    fbuffer->flags, fbuffer->pc);
+}
+EXPORT_SYMBOL_GPL(ftrace_event_buffer_commit);
+
 int ftrace_event_reg(struct ftrace_event_call *call,
 		     enum trace_reg type, void *data)
 {
-- 
1.8.5.3

[for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

[-- Attachment #1: 0008-tracing-Warn-if-a-tracepoint-is-not-set-via-debugfs.patch --]
[-- Type: text/plain, Size: 2980 bytes --]

From: Steven Rostedt <rostedt@goodmis.org>

Tracepoints were made to allow enabling a tracepoint in a module before that
module was loaded. When a tracepoint is enabled and it does not exist, the
name is stored and will be enabled when the tracepoint is created.

The problem with this approach is that when a tracepoint is enabled when
it expects to be there, it gives no warning that it does not exist.

To add salt to the wound, if a module is added and sets the FORCED flag, which
can happen if it isn't signed properly, the tracepoint code will not enabled
the tracepoints, but they will be created in the debugfs system! When a user
goes to enable the tracepoint, the tracepoint code will not see it existing
and will think it is to be enabled later AND WILL NOT GIVE A WARNING.

The tracing will look like it succeeded but will actually be doing nothing.
This will cause lots of confusion and headaches for developers trying to
figure out why they are not seeing their tracepoints.

Link: http://lkml.kernel.org/r/20140213154507.4040fb06@gandalf.local.home

Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Reported-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/tracepoint.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 0d4ef26..0058f33 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -62,6 +62,7 @@ struct tracepoint_entry {
 	struct hlist_node hlist;
 	struct tracepoint_func *funcs;
 	int refcount;	/* Number of times armed. 0 if disarmed. */
+	int enabled;	/* Tracepoint enabled */
 	char name[0];
 };
 
@@ -237,6 +238,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
 	memcpy(&e->name[0], name, name_len);
 	e->funcs = NULL;
 	e->refcount = 0;
+	e->enabled = 0;
 	hlist_add_head(&e->hlist, head);
 	return e;
 }
@@ -316,6 +318,7 @@ static void tracepoint_update_probe_range(struct tracepoint * const *begin,
 		if (mark_entry) {
 			set_tracepoint(&mark_entry, *iter,
 					!!mark_entry->refcount);
+			mark_entry->enabled = !!mark_entry->refcount;
 		} else {
 			disable_tracepoint(*iter);
 		}
@@ -380,6 +383,8 @@ tracepoint_add_probe(const char *name, void *probe, void *data)
 int tracepoint_probe_register(const char *name, void *probe, void *data)
 {
 	struct tracepoint_func *old;
+	struct tracepoint_entry *entry;
+	int ret = 0;
 
 	mutex_lock(&tracepoints_mutex);
 	old = tracepoint_add_probe(name, probe, data);
@@ -388,9 +393,13 @@ int tracepoint_probe_register(const char *name, void *probe, void *data)
 		return PTR_ERR(old);
 	}
 	tracepoint_update_probes();		/* may update entry */
+	entry = get_tracepoint(name);
+	/* Make sure the entry was enabled */
+	if (!entry || !entry->enabled)
+		ret = -ENODEV;
 	mutex_unlock(&tracepoints_mutex);
 	release_probes(old);
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(tracepoint_probe_register);
 
-- 
1.8.5.3

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: linux-kernel@vger.kernel.org
> Cc: "Ingo Molnar" <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew Morton"
> <akpm@linux-foundation.org>, "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, "Johannes Berg"
> <johannes.berg@intel.com>
> Sent: Friday, March 7, 2014 10:09:28 AM
> Subject: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> From: Steven Rostedt <rostedt@goodmis.org>
> 
> Tracepoints were made to allow enabling a tracepoint in a module before that
> module was loaded. When a tracepoint is enabled and it does not exist, the
> name is stored and will be enabled when the tracepoint is created.
> 
> The problem with this approach is that when a tracepoint is enabled when
> it expects to be there, it gives no warning that it does not exist.
> 
> To add salt to the wound, if a module is added and sets the FORCED flag,
> which
> can happen if it isn't signed properly, the tracepoint code will not enabled
> the tracepoints, but they will be created in the debugfs system! When a user
> goes to enable the tracepoint, the tracepoint code will not see it existing
> and will think it is to be enabled later AND WILL NOT GIVE A WARNING.
> 
> The tracing will look like it succeeded but will actually be doing nothing.
> This will cause lots of confusion and headaches for developers trying to
> figure out why they are not seeing their tracepoints.
> 
> Link: http://lkml.kernel.org/r/20140213154507.4040fb06@gandalf.local.home
> 
> Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Reported-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/tracepoint.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 0d4ef26..0058f33 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -62,6 +62,7 @@ struct tracepoint_entry {
>  	struct hlist_node hlist;
>  	struct tracepoint_func *funcs;
>  	int refcount;	/* Number of times armed. 0 if disarmed. */
> +	int enabled;	/* Tracepoint enabled */
>  	char name[0];
>  };
>  
> @@ -237,6 +238,7 @@ static struct tracepoint_entry *add_tracepoint(const char
> *name)
>  	memcpy(&e->name[0], name, name_len);
>  	e->funcs = NULL;
>  	e->refcount = 0;
> +	e->enabled = 0;
>  	hlist_add_head(&e->hlist, head);
>  	return e;
>  }
> @@ -316,6 +318,7 @@ static void tracepoint_update_probe_range(struct
> tracepoint * const *begin,
>  		if (mark_entry) {
>  			set_tracepoint(&mark_entry, *iter,
>  					!!mark_entry->refcount);
> +			mark_entry->enabled = !!mark_entry->refcount;
>  		} else {
>  			disable_tracepoint(*iter);
>  		}
> @@ -380,6 +383,8 @@ tracepoint_add_probe(const char *name, void *probe, void
> *data)
>  int tracepoint_probe_register(const char *name, void *probe, void *data)
>  {
>  	struct tracepoint_func *old;
> +	struct tracepoint_entry *entry;
> +	int ret = 0;
>  
>  	mutex_lock(&tracepoints_mutex);
>  	old = tracepoint_add_probe(name, probe, data);
> @@ -388,9 +393,13 @@ int tracepoint_probe_register(const char *name, void
> *probe, void *data)
>  		return PTR_ERR(old);
>  	}
>  	tracepoint_update_probes();		/* may update entry */
> +	entry = get_tracepoint(name);
> +	/* Make sure the entry was enabled */
> +	if (!entry || !entry->enabled)
> +		ret = -ENODEV;

Hi Steven,

Returning -ENODEV when the probe is still registered might come as a
surprise to the caller. For instance, a caller may dynamically allocate
name, probe, and/or data, it may want to free them when
tracepoint_probe_register returns an error. But this "-ENODEV" return value
is not really an error, and the parameters passed are still used.

If we go down this route, we might want at the very least to add documentation
of tracepoint_probe_register() return values and their meaning
in a comment on top of this function (perhaps also in the header). But
even if we do so, this weird return value semantic with respect to use of the
received parameters will likely cause memory corruption at some point.

Thoughts ?

Thanks,

Mathieu

>  	mutex_unlock(&tracepoints_mutex);
>  	release_probes(old);
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(tracepoint_probe_register);
>  
> --
> 1.8.5.3
> 
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Mon, 10 Mar 2014 20:01:34 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
 
> >  	mutex_lock(&tracepoints_mutex);
> >  	old = tracepoint_add_probe(name, probe, data);
> > @@ -388,9 +393,13 @@ int tracepoint_probe_register(const char *name, void
> > *probe, void *data)
> >  		return PTR_ERR(old);
> >  	}
> >  	tracepoint_update_probes();		/* may update entry */
> > +	entry = get_tracepoint(name);
> > +	/* Make sure the entry was enabled */
> > +	if (!entry || !entry->enabled)
> > +		ret = -ENODEV;
> 
> Hi Steven,
> 
> Returning -ENODEV when the probe is still registered might come as a
> surprise to the caller. For instance, a caller may dynamically allocate
> name, probe, and/or data, it may want to free them when
> tracepoint_probe_register returns an error. But this "-ENODEV" return value
> is not really an error, and the parameters passed are still used.

It's an error when you wanted to enable a probe and the probe doesn't
exist. There are no in tree users of this call that expect it to work
when the probe does not exist.

> 
> If we go down this route, we might want at the very least to add documentation
> of tracepoint_probe_register() return values and their meaning
> in a comment on top of this function (perhaps also in the header). But
> even if we do so, this weird return value semantic with respect to use of the
> received parameters will likely cause memory corruption at some point.
> 
> Thoughts ?

Send a patch to document the return values. Your module can expect this
return value when it doesn't expect the probe to exist.

Again, it's really strange when you go to enable a probe, and there is
no probe to enable.

Note, I was nice. I removed the logic to unregister the probe in this
case.

Anyway, this should even help you. Before there was no way to enable a
probe and know if it was enabled or not. That is, if it didn't exist,
there was no feedback letting you know that.

If you expect to enable a probe that doesn't exist, then you can expect
this return value.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>,
> "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg" <johannes.berg@intel.com>
> Sent: Monday, March 10, 2014 4:19:27 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Mon, 10 Mar 2014 20:01:34 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>  
> > >  	mutex_lock(&tracepoints_mutex);
> > >  	old = tracepoint_add_probe(name, probe, data);
> > > @@ -388,9 +393,13 @@ int tracepoint_probe_register(const char *name, void
> > > *probe, void *data)
> > >  		return PTR_ERR(old);
> > >  	}
> > >  	tracepoint_update_probes();		/* may update entry */
> > > +	entry = get_tracepoint(name);
> > > +	/* Make sure the entry was enabled */
> > > +	if (!entry || !entry->enabled)
> > > +		ret = -ENODEV;
> > 
> > Hi Steven,
> > 
> > Returning -ENODEV when the probe is still registered might come as a
> > surprise to the caller. For instance, a caller may dynamically allocate
> > name, probe, and/or data, it may want to free them when
> > tracepoint_probe_register returns an error. But this "-ENODEV" return value
> > is not really an error, and the parameters passed are still used.
> 
> It's an error when you wanted to enable a probe and the probe doesn't
> exist. There are no in tree users of this call that expect it to work
> when the probe does not exist.
> 
> > 
> > If we go down this route, we might want at the very least to add
> > documentation
> > of tracepoint_probe_register() return values and their meaning
> > in a comment on top of this function (perhaps also in the header). But
> > even if we do so, this weird return value semantic with respect to use of
> > the
> > received parameters will likely cause memory corruption at some point.
> > 
> > Thoughts ?
> 
> Send a patch to document the return values. Your module can expect this
> return value when it doesn't expect the probe to exist.
> 
> Again, it's really strange when you go to enable a probe, and there is
> no probe to enable.
> 
> Note, I was nice. I removed the logic to unregister the probe in this
> case.
> 
> Anyway, this should even help you. Before there was no way to enable a
> probe and know if it was enabled or not. That is, if it didn't exist,
> there was no feedback letting you know that.
> 
> If you expect to enable a probe that doesn't exist, then you can expect
> this return value.

I'm OK with this change. I was merely pointing out that we need to document
this return value, since its semantic differs from what is usually expected.

I'll prepare a patch soon to document the return value.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Frank Ch. Eigler]

Hi -


>> From: Steven Rostedt <rostedt@goodmis.org>
>> 
>> Tracepoints were made to allow enabling a tracepoint in a module before that
>> module was loaded. When a tracepoint is enabled and it does not exist, the
>> name is stored and will be enabled when the tracepoint is created.
>> 
>> The problem with this approach is that when a tracepoint is enabled when
>> it expects to be there, it gives no warning that it does not exist.

So it is a deferred-activation kind of call, with no way of knowing
when or if the tracepoints will start coming in.  Why is that
supported at all, considering that clients could monitor modules
coming & going via the module_notifier chain, and update registration
at that time?


>> +	entry = get_tracepoint(name);
>> +	/* Make sure the entry was enabled */
>> +	if (!entry || !entry->enabled)
>> +		ret = -ENODEV;

For what it's worth, I agree with Mathieu that this sort of successful
failure result code is a problem for tracking what needs cleanup and
what doesn't.  (In systemtap's case, if this change gets merged, we'll
have to treat -ENODEV as if it were 0.)


- FChE

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Mon, 10 Mar 2014 22:41:31 -0400
fche@redhat.com (Frank Ch. Eigler) wrote:

> 
> Hi -
> 
> 
> >> From: Steven Rostedt <rostedt@goodmis.org>
> >> 
> >> Tracepoints were made to allow enabling a tracepoint in a module before that
> >> module was loaded. When a tracepoint is enabled and it does not exist, the
> >> name is stored and will be enabled when the tracepoint is created.
> >> 
> >> The problem with this approach is that when a tracepoint is enabled when
> >> it expects to be there, it gives no warning that it does not exist.
> 
> So it is a deferred-activation kind of call, with no way of knowing
> when or if the tracepoints will start coming in.  Why is that
> supported at all, considering that clients could monitor modules
> coming & going via the module_notifier chain, and update registration
> at that time?

That's my argument.

> 
> 
> >> +	entry = get_tracepoint(name);
> >> +	/* Make sure the entry was enabled */
> >> +	if (!entry || !entry->enabled)
> >> +		ret = -ENODEV;
> 
> For what it's worth, I agree with Mathieu that this sort of successful
> failure result code is a problem for tracking what needs cleanup and
> what doesn't.  (In systemtap's case, if this change gets merged, we'll
> have to treat -ENODEV as if it were 0.)

Does systemtap enable tracepoints before they are created? That is, do
you allow enabling of a tracepoint in a module that is not loaded yet?

If not, than you want this as an error.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Frank Ch. Eigler" <fche@redhat.com>
> Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, linux-kernel@vger.kernel.org, "Ingo Molnar"
> <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>,
> "Johannes Berg" <johannes.berg@intel.com>
> Sent: Monday, March 10, 2014 10:58:20 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Mon, 10 Mar 2014 22:41:31 -0400
> fche@redhat.com (Frank Ch. Eigler) wrote:
> 
> > 
> > Hi -
> > 
> > 
> > >> From: Steven Rostedt <rostedt@goodmis.org>
> > >> 
> > >> Tracepoints were made to allow enabling a tracepoint in a module before
> > >> that
> > >> module was loaded. When a tracepoint is enabled and it does not exist,
> > >> the
> > >> name is stored and will be enabled when the tracepoint is created.
> > >> 
> > >> The problem with this approach is that when a tracepoint is enabled when
> > >> it expects to be there, it gives no warning that it does not exist.
> > 
> > So it is a deferred-activation kind of call, with no way of knowing
> > when or if the tracepoints will start coming in.  Why is that
> > supported at all, considering that clients could monitor modules
> > coming & going via the module_notifier chain, and update registration
> > at that time?
> 
> That's my argument.

So basically, all we'd have to do in LTTng is to add a hash table tracking the
tracepoint probes which are registered, but for which there are no
tracepoint call sites. Whenever registration of a probe would fail due to
-ENODEV (assuming we unregister the probe within tracepoint.c when we return
-ENODEV, as you initially proposed), we would put this probe in the hash table.
Upon module coming, we would iterate on the module's tracepoints and check
if any of those match the content of the hash table, and then register the
probe.

I guess I'd prefer that to the weird successful failure return value in
tracepoint.c.

Thanks,

Mathieu

> 
> > 
> > 
> > >> +	entry = get_tracepoint(name);
> > >> +	/* Make sure the entry was enabled */
> > >> +	if (!entry || !entry->enabled)
> > >> +		ret = -ENODEV;
> > 
> > For what it's worth, I agree with Mathieu that this sort of successful
> > failure result code is a problem for tracking what needs cleanup and
> > what doesn't.  (In systemtap's case, if this change gets merged, we'll
> > have to treat -ENODEV as if it were 0.)
> 
> Does systemtap enable tracepoints before they are created? That is, do
> you allow enabling of a tracepoint in a module that is not loaded yet?
> 
> If not, than you want this as an error.
> 
> -- Steve
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Tue, 11 Mar 2014 04:08:27 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

 
> > That's my argument.
> 
> So basically, all we'd have to do in LTTng is to add a hash table tracking the
> tracepoint probes which are registered, but for which there are no
> tracepoint call sites. Whenever registration of a probe would fail due to
> -ENODEV (assuming we unregister the probe within tracepoint.c when we return
> -ENODEV, as you initially proposed), we would put this probe in the hash table.
> Upon module coming, we would iterate on the module's tracepoints and check
> if any of those match the content of the hash table, and then register the
> probe.
> 
> I guess I'd prefer that to the weird successful failure return value in
> tracepoint.c.
> 

OK, then I'll add back in the removal of the tracepoint on this error.
Then your LTTng module can handle the tracepoints that don't exist yet.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Frank Ch. Eigler]

Hi, Steven -

> > So it is a deferred-activation kind of call, with no way of knowing
> > when or if the tracepoints will start coming in.  Why is that
> > supported at all, considering that clients could monitor modules
> > coming & going via the module_notifier chain, and update registration
> > at that time?
> 
> That's my argument.

Was there an answer?


> > >> +	entry = get_tracepoint(name);
> > >> +	/* Make sure the entry was enabled */
> > >> +	if (!entry || !entry->enabled)
> > >> +		ret = -ENODEV;
> > 
> > For what it's worth, I agree with Mathieu that this sort of successful
> > failure result code is a problem for tracking what needs cleanup and
> > what doesn't.  (In systemtap's case, if this change gets merged, we'll
> > have to treat -ENODEV as if it were 0.)
> 
> Does systemtap enable tracepoints before they are created? That is, do
> you allow enabling of a tracepoint in a module that is not loaded yet?

We have no formal opinion on whether or not this makes sense.  If the
kernel permits it, fine.

> If not, than you want this as an error.

But it's not exactly an error!  It's a success of sorts, and means
that later on we have to unregister the callback, just as if it were
successful.


- FChE

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Frank Ch. Eigler" <fche@redhat.com>
> To: "Steven Rostedt" <rostedt@goodmis.org>
> Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, linux-kernel@vger.kernel.org, "Ingo Molnar"
> <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>,
> "Johannes Berg" <johannes.berg@intel.com>
> Sent: Tuesday, March 11, 2014 10:26:50 AM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> Hi, Steven -
> 
> > > So it is a deferred-activation kind of call, with no way of knowing
> > > when or if the tracepoints will start coming in.  Why is that
> > > supported at all, considering that clients could monitor modules
> > > coming & going via the module_notifier chain, and update registration
> > > at that time?
> > 
> > That's my argument.
> 
> Was there an answer?

Let's step back and look at the overall picture here, along with the
possible solutions that are available so far.

My intent is to let end users specify "I want to trace this specific
tracepoint" from the tracer interface as long as the tracepoint probe
provider module is loaded (the file with CREATE_TRACE_POINTS defined).
For instance, the tracepoint call site can be within a driver loaded by
the USB stack when a USB device is plugged in. While tracing is enabled,
the user may want to plug in the said USB device to investigate what is
the culprit of an issue he would be facing when the the device is plugged
in.

There seems to be 2 elegant ways to achieve this while giving feedback to
tracers about whether there are active tracepoint callsites or not:

1) We add a trace_has_callsites_enabled() (or any better name) function to
   tracepoint.h, which allows tracers to query whether a given tracepoint
   name has any callsites enabled,
2) We change tracepoint_probe_register() so it unregisters the tracepoint
   and return -ENODEV if there are no callsites enabled, and deal with
   the use-case I explain above with module coming and going within the
   tracer. This duplicates part of the tracepoint infrastructure into the
   tracer though, which is why I was not so keen on going for this
   solution.

The other solution proposed by Steven (returning -ENODEV without
unregistering the tracepoint) does not appear to be an elegant solution,
as we discussed earlier in this thread. It kind of weird to have a negative
value treated as an OK special case.

The other solution proposed by Steven in an earlier thread was to tie
tracepoints very deeply with module loading infrastructure and add module
parameters specifically to specify if tracepoint callsites need to be
enabled on module load. This approach unfortunately expect that everyone
interacts with module loading, as root, in a system-wide (no multi-session)
fashion and is not suitable for the user-base we are targeting. I seems to
be a user experience disaster IMHO.

I'm OK as long as we have an elegant way forward. Ideally I would have
prefered (1) to eliminate code duplication between tracers and tracepoint
infrastructure (we have to reimplement a hash table similar to tracepoints
within the tracer with solution (2)), but (2) technically works too.

Thanks,

Mathieu

> 
> 
> > > >> +	entry = get_tracepoint(name);
> > > >> +	/* Make sure the entry was enabled */
> > > >> +	if (!entry || !entry->enabled)
> > > >> +		ret = -ENODEV;
> > > 
> > > For what it's worth, I agree with Mathieu that this sort of successful
> > > failure result code is a problem for tracking what needs cleanup and
> > > what doesn't.  (In systemtap's case, if this change gets merged, we'll
> > > have to treat -ENODEV as if it were 0.)
> > 
> > Does systemtap enable tracepoints before they are created? That is, do
> > you allow enabling of a tracepoint in a module that is not loaded yet?
> 
> We have no formal opinion on whether or not this makes sense.  If the
> kernel permits it, fine.
> 
> > If not, than you want this as an error.
> 
> But it's not exactly an error!  It's a success of sorts, and means
> that later on we have to unregister the callback, just as if it were
> successful.
> 
> 
> - FChE
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Tue, 11 Mar 2014 15:06:22 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:


> Let's step back and look at the overall picture here, along with the
> possible solutions that are available so far.
> 
> My intent is to let end users specify "I want to trace this specific
> tracepoint" from the tracer interface as long as the tracepoint probe
> provider module is loaded (the file with CREATE_TRACE_POINTS defined).
> For instance, the tracepoint call site can be within a driver loaded by
> the USB stack when a USB device is plugged in. While tracing is enabled,
> the user may want to plug in the said USB device to investigate what is
> the culprit of an issue he would be facing when the the device is plugged
> in.
> 
> There seems to be 2 elegant ways to achieve this while giving feedback to
> tracers about whether there are active tracepoint callsites or not:
> 
> 1) We add a trace_has_callsites_enabled() (or any better name) function to
>    tracepoint.h, which allows tracers to query whether a given tracepoint
>    name has any callsites enabled,

I don't find that one very elegant at all.

> 2) We change tracepoint_probe_register() so it unregisters the tracepoint
>    and return -ENODEV if there are no callsites enabled, and deal with
>    the use-case I explain above with module coming and going within the
>    tracer. This duplicates part of the tracepoint infrastructure into the
>    tracer though, which is why I was not so keen on going for this
>    solution.

I don't mind the above.

> 
> The other solution proposed by Steven (returning -ENODEV without
> unregistering the tracepoint) does not appear to be an elegant solution,
> as we discussed earlier in this thread. It kind of weird to have a negative
> value treated as an OK special case.

Right, but it was a compromise as you didn't like #2 earlier.

> 
> The other solution proposed by Steven in an earlier thread was to tie
> tracepoints very deeply with module loading infrastructure and add module
> parameters specifically to specify if tracepoint callsites need to be
> enabled on module load. This approach unfortunately expect that everyone
> interacts with module loading, as root, in a system-wide (no multi-session)

Who can load modules not as root?? That is utterly broken. As once you
can load a module, YOU ARE ROOT.
 
> fashion and is not suitable for the user-base we are targeting. I seems to
> be a user experience disaster IMHO.

For your case only. But it is normal operation for normal uses of Linux.

> 
> I'm OK as long as we have an elegant way forward. Ideally I would have
> prefered (1) to eliminate code duplication between tracers and tracepoint
> infrastructure (we have to reimplement a hash table similar to tracepoints
> within the tracer with solution (2)), but (2) technically works too.

Here's what I propose then. We implement 2 for now. You can "duplicate"
the code into your own work. Then we should be able to simplify the
tracepoint code as it no longer will have the requirement to enable
tracepoints that do not exist.

Also we can remove that tracepoint_probe_unregister_noupdate() as it
has no in-tree users. Which, you have been lucky that no one noticed
that yet, as that is a legitimate excuse to remove a function.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>
> Sent: Tuesday, March 11, 2014 11:40:23 AM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Tue, 11 Mar 2014 15:06:22 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> 
> > Let's step back and look at the overall picture here, along with the
> > possible solutions that are available so far.
> > 
> > My intent is to let end users specify "I want to trace this specific
> > tracepoint" from the tracer interface as long as the tracepoint probe
> > provider module is loaded (the file with CREATE_TRACE_POINTS defined).
> > For instance, the tracepoint call site can be within a driver loaded by
> > the USB stack when a USB device is plugged in. While tracing is enabled,
> > the user may want to plug in the said USB device to investigate what is
> > the culprit of an issue he would be facing when the the device is plugged
> > in.
> > 
> > There seems to be 2 elegant ways to achieve this while giving feedback to
> > tracers about whether there are active tracepoint callsites or not:
> > 
> > 1) We add a trace_has_callsites_enabled() (or any better name) function to
> >    tracepoint.h, which allows tracers to query whether a given tracepoint
> >    name has any callsites enabled,
> 
> I don't find that one very elegant at all.
> 
> > 2) We change tracepoint_probe_register() so it unregisters the tracepoint
> >    and return -ENODEV if there are no callsites enabled, and deal with
> >    the use-case I explain above with module coming and going within the
> >    tracer. This duplicates part of the tracepoint infrastructure into the
> >    tracer though, which is why I was not so keen on going for this
> >    solution.
> 
> I don't mind the above.
> 
> > 
> > The other solution proposed by Steven (returning -ENODEV without
> > unregistering the tracepoint) does not appear to be an elegant solution,
> > as we discussed earlier in this thread. It kind of weird to have a negative
> > value treated as an OK special case.
> 
> Right, but it was a compromise as you didn't like #2 earlier.
> 
> > 
> > The other solution proposed by Steven in an earlier thread was to tie
> > tracepoints very deeply with module loading infrastructure and add module
> > parameters specifically to specify if tracepoint callsites need to be
> > enabled on module load. This approach unfortunately expect that everyone
> > interacts with module loading, as root, in a system-wide (no multi-session)
> 
> Who can load modules not as root?? That is utterly broken. As once you
> can load a module, YOU ARE ROOT.

udevd runs as root, and listens to events such as USB hotplug, and loads modules
in the back of users. The users don't need to be root for this to happen.

>  
> > fashion and is not suitable for the user-base we are targeting. I seems to
> > be a user experience disaster IMHO.
> 
> For your case only. But it is normal operation for normal uses of Linux.

AFAIK pretty much all distros use udev nowadays. Are you suggesting that all
users using udev and distribution kernels are not "normal uses of Linux" ?

> 
> > 
> > I'm OK as long as we have an elegant way forward. Ideally I would have
> > prefered (1) to eliminate code duplication between tracers and tracepoint
> > infrastructure (we have to reimplement a hash table similar to tracepoints
> > within the tracer with solution (2)), but (2) technically works too.
> 
> Here's what I propose then. We implement 2 for now. You can "duplicate"
> the code into your own work.

Works for me.

> Then we should be able to simplify the
> tracepoint code as it no longer will have the requirement to enable
> tracepoints that do not exist.

What happens for the case where we enable a tracepoint, and then the
only module containing a callsite of that tracepoint is unloaded, and
then reloaded ?

> 
> Also we can remove that tracepoint_probe_unregister_noupdate() as it
> has no in-tree users. Which, you have been lucky that no one noticed
> that yet, as that is a legitimate excuse to remove a function.

LTTng 2.x does not use it at all, so I have no issue with the removal.
I think since this commit it was not required anymore:

commit b75ef8b44b1cb95f5a26484b0e2fe37a63b12b44
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Wed Aug 10 15:18:39 2011 -0400

    Tracepoint: Dissociate from module mutex

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Tue, 11 Mar 2014 17:34:23 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
 
> > Who can load modules not as root?? That is utterly broken. As once you
> > can load a module, YOU ARE ROOT.
> 
> udevd runs as root, and listens to events such as USB hotplug, and loads modules
> in the back of users. The users don't need to be root for this to happen.

udevd may run as a proxy for users. But it is still a root user.
Sysadmins are the ones that set up udevd. If you own your own box, you
are technically the sysadmin for it. Modifications to udev require sudo
privileges.

Tracepoints should not be something a non-sysadmin can modify or
enable. If you want non root users to do so, create a proxy daemon like
udevd to do it for them. But the kernel isn't going to allow that
directly.

> 
> >  
> > > fashion and is not suitable for the user-base we are targeting. I seems to
> > > be a user experience disaster IMHO.
> > 
> > For your case only. But it is normal operation for normal uses of Linux.
> 
> AFAIK pretty much all distros use udev nowadays. Are you suggesting that all
> users using udev and distribution kernels are not "normal uses of Linux" ?

udev is root, and is modified by root users. A normal user can not just
interact with udev. And sticking in a usb stick into a computer counts
as a sysadmin operation, even if the person doesn't official have the
title.

> 
> > 
> > > 
> > > I'm OK as long as we have an elegant way forward. Ideally I would have
> > > prefered (1) to eliminate code duplication between tracers and tracepoint
> > > infrastructure (we have to reimplement a hash table similar to tracepoints
> > > within the tracer with solution (2)), but (2) technically works too.
> > 
> > Here's what I propose then. We implement 2 for now. You can "duplicate"
> > the code into your own work.
> 
> Works for me.
> 
> > Then we should be able to simplify the
> > tracepoint code as it no longer will have the requirement to enable
> > tracepoints that do not exist.
> 
> What happens for the case where we enable a tracepoint, and then the
> only module containing a callsite of that tracepoint is unloaded, and
> then reloaded ?

When a module is unloaded, it usually loses state. Is there any state
that is maintained for a module being unloaded and reloaded again?
Besides tracepoints? If not, then the module should lose its state for
tracepoints as well.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>
> Sent: Tuesday, March 11, 2014 3:13:57 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 

For those just added in CC, https://lkml.org/lkml/2014/3/11/431 provides the
missing context. The discussion is about adding module load parameters as ABI
for end users to enable tracepoints in modules (Steven has proposed the idea
a couple of times in the past, I am against for the reasons I am exposing
below). This discussion will likely have deep impact on other changes Steven
is proposing for tracepoint.c (such as making tracepoint_probe_register()
return -ENODEV when no tracepoint call site is currently loaded for a given
tracepoint), so I think the audience should be broadened.

> On Tue, 11 Mar 2014 17:34:23 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>  
> > > Who can load modules not as root?? That is utterly broken. As once you
> > > can load a module, YOU ARE ROOT.
> > 
> > udevd runs as root, and listens to events such as USB hotplug, and loads
> > modules
> > in the back of users. The users don't need to be root for this to happen.
> 
> udevd may run as a proxy for users. But it is still a root user.
> Sysadmins are the ones that set up udevd.

Uh ? There are tons of distributions that setup udevd for their users, and
no sysadmin has to interact with it.

> If you own your own box, you are technically the sysadmin for it.

If you use a box owned by your company, running Linux, but enforcing module
signature, you might be physically allowed to connect USB devices, and let
udevd load the appropriate modules for the connected devices, but that does
not mean you have root access at all. But having the ability to trace the
said computer from a user having "tracing" privileges is very valuable to
help debugging that machine.

Even without going that far, if you want tracing to be widely usable to
pinpoint issues within Linux and/or its applications or libraries, you
basically have to remove most user interactions from the picture, and let
automated tools gather tracing data to perform analysis. As soon as you
require users to fiddle with udev configuration, or pass special parameters
to modprobe, or if you start coupling the tracing infrastructure with the
module configuration, you just made tracing unusable for a vast majority of
Linux distribution users out there.

I've been trying to make my point that tracing should be first of all
_usable_ by non-expert Linux end users, but it seems that I'm completely
failing at getting the message through somehow. And believe me I've spent
years trying to voice this message. Perhaps I'm not cursing enough, perhaps
I'm not yelling enough. Rather than arguing, the path I'm currently taking
is to prove my point by putting the tools needed by my user community in
their hands.

If to you the majority of Linux users means "kernel developers", then
I can see how our point of views cannot be reconciled easily. From my
point of view, there are far more users who need those tracing tools that
are not kernel developers.

> Modifications to udev require sudo
> privileges.
> 
> Tracepoints should not be something a non-sysadmin can modify or
> enable. If you want non root users to do so, create a proxy daemon like
> udevd to do it for them. But the kernel isn't going to allow that
> directly.

I never said the kernel should directly expose ABIs accessible by others
than root. Actually, this is the very reason we have a "lttng-sessiond"
daemon running as root to control kernel tracing. However, it can be
controlled by a "lttng" command line tool over unix sockets if the user
is part of the "tracing" group. Right there, this is the concept of your
proxy daemon. However, you seem to imply that this proxy daemon should
somehow fiddle with udevd and/or modprobe configuration to add temporary
system-wide parameters to how modules are loaded, depending on tracing
needs. Trying to couple something usually configured initially for a
large portion of a system's life (and even with tripwire validation that
/etc does not change over time) with something as transient as tracing
simply cannot work without causing havoc in the system configuration. So
no, having a tracing proxy daemon changing configuration of module
arguments cannot fly.

> 
> > 
> > >  
> > > > fashion and is not suitable for the user-base we are targeting. I seems
> > > > to
> > > > be a user experience disaster IMHO.
> > > 
> > > For your case only. But it is normal operation for normal uses of Linux.
> > 
> > AFAIK pretty much all distros use udev nowadays. Are you suggesting that
> > all
> > users using udev and distribution kernels are not "normal uses of Linux" ?
> 
> udev is root, and is modified by root users. A normal user can not just
> interact with udev. And sticking in a usb stick into a computer counts
> as a sysadmin operation, even if the person doesn't official have the
> title.

So if I understand your line of thought, you imply that anyone who can
plug a USB stick of any kind (memory stick, USB wifi card, etc) into a
computer should automatically have root access ? Is it just me, or it
makes no sense ?

> 
> > 
> > > 
> > > > 
> > > > I'm OK as long as we have an elegant way forward. Ideally I would have
> > > > prefered (1) to eliminate code duplication between tracers and
> > > > tracepoint
> > > > infrastructure (we have to reimplement a hash table similar to
> > > > tracepoints
> > > > within the tracer with solution (2)), but (2) technically works too.
> > > 
> > > Here's what I propose then. We implement 2 for now. You can "duplicate"
> > > the code into your own work.
> > 
> > Works for me.
> > 
> > > Then we should be able to simplify the
> > > tracepoint code as it no longer will have the requirement to enable
> > > tracepoints that do not exist.
> > 
> > What happens for the case where we enable a tracepoint, and then the
> > only module containing a callsite of that tracepoint is unloaded, and
> > then reloaded ?
> 
> When a module is unloaded, it usually loses state. Is there any state
> that is maintained for a module being unloaded and reloaded again?
> Besides tracepoints? If not, then the module should lose its state for
> tracepoints as well.

What you're missing here is that the same tracepoint can appear within
more than one module.

I think there is a core, fundamental difference between the ways we see
tracing. You seem to see tracing as being solely part of the modules
that contain the caller tracing code. From my perspective, tracing fits
much better in the "aspect oriented programming" paradigm, since it spreads
the same "crosscutting concerns" across multiple modules. So yes, in AOP,
it makes perfect sense that tracing activation state goes beyond the
lifetime of a single loaded module: it spawns across core kernel, and zero,
one, or more loaded modules, independently of whether they are unloaded and
reloaded again.


So going back on the changes you proposed to the tracepoint infrastructure:

A) Making tracepoint_probe_register() unregister the probe and fail when
   there is no tracepoint call site present for a probe at that time:

With this approach, if the probe is registered when the module containing
the call site is loaded, and then that module is unloaded, it will leave
the probe in a registered state, leaving the tracer lying to the user,
letting them think there are currently callsites for this tracepoint when
there are none.

B) Making tracepoint_probe_register() just return an error (-ENOEDV), but
   leaving the probe registered.

I think we have already agreed that this compromise solution is a weird API
choice, since an error condition must be treated as a success by the caller.
Moreover, it has the same issue as (A) above: if the module is unloaded
after probe registration, the tracer is lying to the user, making them
think there is a call site loaded when there are none.

C) Adding module load arguments to specify which tracepoint should be
   enabled.

Please refer to my response above in this email to understand why I object.

Back to your original requirement: you want to let the tracer communicate
to end users whether a specific tracepoint has call sites or not. I'm
currently preparing a patch implementing a "tracepoint_callsite_count()"
API that will allow this, and it won't lie to the tracer like solutions
(A) and (B) above. Whenever the tracer queries the state, it will get the
current information about the number of enabled call sites, not some stale
data based on a prior registration error.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Wed, 12 Mar 2014 14:24:54 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> ----- Original Message -----
> > From: "Steven Rostedt" <rostedt@goodmis.org>
> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> > Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> > Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> > <johannes.berg@intel.com>
> > Sent: Tuesday, March 11, 2014 3:13:57 PM
> > Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> > 
> 
> For those just added in CC, https://lkml.org/lkml/2014/3/11/431 provides the
> missing context. The discussion is about adding module load parameters as ABI
> for end users to enable tracepoints in modules (Steven has proposed the idea
> a couple of times in the past, I am against for the reasons I am exposing
> below). This discussion will likely have deep impact on other changes Steven
> is proposing for tracepoint.c (such as making tracepoint_probe_register()
> return -ENODEV when no tracepoint call site is currently loaded for a given
> tracepoint), so I think the audience should be broadened.

Please tell me one in-tree user that will be impacted?

> 
> > On Tue, 11 Mar 2014 17:34:23 +0000 (UTC)
> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> >  
> > > > Who can load modules not as root?? That is utterly broken. As once you
> > > > can load a module, YOU ARE ROOT.
> > > 
> > > udevd runs as root, and listens to events such as USB hotplug, and loads
> > > modules
> > > in the back of users. The users don't need to be root for this to happen.
> > 
> > udevd may run as a proxy for users. But it is still a root user.
> > Sysadmins are the ones that set up udevd.
> 
> Uh ? There are tons of distributions that setup udevd for their users, and
> no sysadmin has to interact with it.

And the distribution just magically installs itself onto a computer? No,
a user does, and in doing so, they set up udev. A person installing a
distribution is a sysadmin. Even if it's grandma installing it herself.
She owns the box, no one else is doing anything for her (unless it's
your grandma).

> 
> > If you own your own box, you are technically the sysadmin for it.
> 
> If you use a box owned by your company, running Linux, but enforcing module
> signature, you might be physically allowed to connect USB devices, and let
> udevd load the appropriate modules for the connected devices, but that does
> not mean you have root access at all. But having the ability to trace the
> said computer from a user having "tracing" privileges is very valuable to
> help debugging that machine.

Tracing is a root privilege.

> 
> Even without going that far, if you want tracing to be widely usable to
> pinpoint issues within Linux and/or its applications or libraries, you
> basically have to remove most user interactions from the picture, and let
> automated tools gather tracing data to perform analysis. As soon as you
> require users to fiddle with udev configuration, or pass special parameters
> to modprobe, or if you start coupling the tracing infrastructure with the
> module configuration, you just made tracing unusable for a vast majority of
> Linux distribution users out there.

This seems very specific to a single tracing tool that happens not to
be in the kernel tree.

> 
> I've been trying to make my point that tracing should be first of all
> _usable_ by non-expert Linux end users, but it seems that I'm completely
> failing at getting the message through somehow. And believe me I've spent
> years trying to voice this message. Perhaps I'm not cursing enough, perhaps
> I'm not yelling enough. Rather than arguing, the path I'm currently taking
> is to prove my point by putting the tools needed by my user community in
> their hands.

I don't know why you are complaining. Think of this as your job
security.

> 
> If to you the majority of Linux users means "kernel developers", then
> I can see how our point of views cannot be reconciled easily. From my
> point of view, there are far more users who need those tracing tools that
> are not kernel developers.

No, but the majority of tracing users *are* kernel developers, or those
that are experts in the system.

Why would grandma want to trace the kernel?

> 
> > Modifications to udev require sudo
> > privileges.
> > 
> > Tracepoints should not be something a non-sysadmin can modify or
> > enable. If you want non root users to do so, create a proxy daemon like
> > udevd to do it for them. But the kernel isn't going to allow that
> > directly.
> 
> I never said the kernel should directly expose ABIs accessible by others
> than root. Actually, this is the very reason we have a "lttng-sessiond"
> daemon running as root to control kernel tracing. However, it can be
> controlled by a "lttng" command line tool over unix sockets if the user
> is part of the "tracing" group. Right there, this is the concept of your
> proxy daemon. However, you seem to imply that this proxy daemon should
> somehow fiddle with udevd and/or modprobe configuration to add temporary
> system-wide parameters to how modules are loaded, depending on tracing
> needs. Trying to couple something usually configured initially for a
> large portion of a system's life (and even with tripwire validation that
> /etc does not change over time) with something as transient as tracing
> simply cannot work without causing havoc in the system configuration. So
> no, having a tracing proxy daemon changing configuration of module
> arguments cannot fly.

I actually never said that. I said that you could have a proxy to
enable or disable the tracepoint. I don't know where you're getting
this crap from. Yes, I have suggested in the past that the ideal
approach is to have modprobe handle it, just like it handles other
module changes for one time deals. And yes, tracing a module is a one
time deal. But you seem to be getting in a tizzy thinking that's what I
recommended yesterday.

> 
> > 
> > > 
> > > >  
> > > > > fashion and is not suitable for the user-base we are targeting. I seems
> > > > > to
> > > > > be a user experience disaster IMHO.
> > > > 
> > > > For your case only. But it is normal operation for normal uses of Linux.
> > > 
> > > AFAIK pretty much all distros use udev nowadays. Are you suggesting that
> > > all
> > > users using udev and distribution kernels are not "normal uses of Linux" ?
> > 
> > udev is root, and is modified by root users. A normal user can not just
> > interact with udev. And sticking in a usb stick into a computer counts
> > as a sysadmin operation, even if the person doesn't official have the
> > title.
> 
> So if I understand your line of thought, you imply that anyone who can
> plug a USB stick of any kind (memory stick, USB wifi card, etc) into a
> computer should automatically have root access ? Is it just me, or it
> makes no sense ?

I wouldn't want people to have access to any box I own to be able to
just stick any usb device into the computer. Look at all the USB drivers
we have. You think they are all perfect. I'm sure it wouldn't be too
hard to find a bad USB device driver and make your own dongle to stick
in, that can root the box.

> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > I'm OK as long as we have an elegant way forward. Ideally I would have
> > > > > prefered (1) to eliminate code duplication between tracers and
> > > > > tracepoint
> > > > > infrastructure (we have to reimplement a hash table similar to
> > > > > tracepoints
> > > > > within the tracer with solution (2)), but (2) technically works too.
> > > > 
> > > > Here's what I propose then. We implement 2 for now. You can "duplicate"
> > > > the code into your own work.
> > > 
> > > Works for me.
> > > 
> > > > Then we should be able to simplify the
> > > > tracepoint code as it no longer will have the requirement to enable
> > > > tracepoints that do not exist.
> > > 
> > > What happens for the case where we enable a tracepoint, and then the
> > > only module containing a callsite of that tracepoint is unloaded, and
> > > then reloaded ?
> > 
> > When a module is unloaded, it usually loses state. Is there any state
> > that is maintained for a module being unloaded and reloaded again?
> > Besides tracepoints? If not, then the module should lose its state for
> > tracepoints as well.
> 
> What you're missing here is that the same tracepoint can appear within
> more than one module.

So?

> 
> I think there is a core, fundamental difference between the ways we see
> tracing. You seem to see tracing as being solely part of the modules
> that contain the caller tracing code. From my perspective, tracing fits
> much better in the "aspect oriented programming" paradigm, since it spreads
> the same "crosscutting concerns" across multiple modules. So yes, in AOP,
> it makes perfect sense that tracing activation state goes beyond the
> lifetime of a single loaded module: it spawns across core kernel, and zero,
> one, or more loaded modules, independently of whether they are unloaded and
> reloaded again.

Have your LTTNg module dictate that policy.

> 
> 
> So going back on the changes you proposed to the tracepoint infrastructure:
> 
> A) Making tracepoint_probe_register() unregister the probe and fail when
>    there is no tracepoint call site present for a probe at that time:
> 
> With this approach, if the probe is registered when the module containing
> the call site is loaded, and then that module is unloaded, it will leave
> the probe in a registered state, leaving the tracer lying to the user,
> letting them think there are currently callsites for this tracepoint when
> there are none.

Hmm, currently the probe lies to the user when the tracepoint doesn't
exist. We enable it and nothing happens. This BROKE IN TREE USERS!!!!

I suggest that we remove the probe when the module is unloaded. No
lying. Either a tracepoint exists when you enable it, or it doesn't.


> 
> B) Making tracepoint_probe_register() just return an error (-ENOEDV), but
>    leaving the probe registered.
> 
> I think we have already agreed that this compromise solution is a weird API
> choice, since an error condition must be treated as a success by the caller.
> Moreover, it has the same issue as (A) above: if the module is unloaded
> after probe registration, the tracer is lying to the user, making them
> think there is a call site loaded when there are none.

But isn't that what we do today?

> 
> C) Adding module load arguments to specify which tracepoint should be
>    enabled.
> 
> Please refer to my response above in this email to understand why I object.

I didn't suggest this in this thread. I only pointed out that is the
most sane situation. But I've been trying to work with you, and you
don't seem to see that.

> 
> Back to your original requirement: you want to let the tracer communicate
> to end users whether a specific tracepoint has call sites or not. I'm
> currently preparing a patch implementing a "tracepoint_callsite_count()"
> API that will allow this, and it won't lie to the tracer like solutions
> (A) and (B) above. Whenever the tracer queries the state, it will get the
> current information about the number of enabled call sites, not some stale
> data based on a prior registration error.

No, I don't want to know if this call site exists or not. This stupid
"tracepoint_probe_register()" that activates a tracepoint by name. But
if that tracepoint doesn't exist, it just returns normally as if
everything is fine and dandy. WTF!

Look Mathieu, I've been trying to be nice here. This crap has broken
in tree kernel users, and this magical "enable a non-existent
tracepoint, so when one is created, it will be enabled" is only used by
LTTng! NOBODY ELSE USES IT. This means that I can strip this usage out
of the kernel, as there are no uses of it in the kernel.

What you don't seem to realize, is that your methods broke the stuff
that is in the kernel, and you don't seem to give a shit about that.

The solution I like the most that I believe will work for both of us,
is to to move this magic "enable tracepoint in the future" to your
LTTng module. Have your module register a module load and unload handler
to be able to see the tracepoints that exist in the module, and you can
enable them then. When a module is unloaded, your module can do the
accounting to record that, and the state of its tracepoints. 

Looks like we can have it both ways. A way that works well for the
kernel, and a way that works well for you. But your module will need to
do the heavy work for what you want.

To me, a tracepoint should only be enabled when it exists. If it is
enabled in module when the module is unloaded, then it should be
removed after the module has left. If the module is loaded again, it is
up to the user (or your module) to enable that tracepoint again.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

To sum up this thread, and get the signal vs noise ratio up.

On Wed, 12 Mar 2014 11:11:00 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:


> The solution I like the most that I believe will work for both of us,
> is to to move this magic "enable tracepoint in the future" to your
> LTTng module. Have your module register a module load and unload handler
> to be able to see the tracepoints that exist in the module, and you can
> enable them then. When a module is unloaded, your module can do the
> accounting to record that, and the state of its tracepoints. 

This is my final proposal.

I'll add the patch that removes the tracepoint on failure along with
returning -ENODEV. That way, there will be no registered tracepoints
that do not exist.

I'll also make sure that on module unload, the tracepoints are disabled
for the module as well.

Then, you can simply add a module notifier that does the work that you
like, and save and restore the state of named tracepoints and enabled
them on module load. Just set the priority of the notifier to 1
so that it runs after the tracepoint notifier that adds the new
tracepoints to the system.

> 
> Looks like we can have it both ways. A way that works well for the
> kernel, and a way that works well for you. But your module will need to
> do the heavy work for what you want.
> 
> To me, a tracepoint should only be enabled when it exists. If it is
> enabled in module when the module is unloaded, then it should be
> removed after the module has left. If the module is loaded again, it is
> up to the user (or your module) to enable that tracepoint again.

I want to point out that LTTng should not be dictating the way the
kernel works, but it should be the other way around.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>
> Sent: Wednesday, March 12, 2014 11:46:11 AM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> 
> To sum up this thread, and get the signal vs noise ratio up.
> 
> On Wed, 12 Mar 2014 11:11:00 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> 
> > The solution I like the most that I believe will work for both of us,
> > is to to move this magic "enable tracepoint in the future" to your
> > LTTng module. Have your module register a module load and unload handler
> > to be able to see the tracepoints that exist in the module, and you can
> > enable them then. When a module is unloaded, your module can do the
> > accounting to record that, and the state of its tracepoints.
> 
> This is my final proposal.
> 
> I'll add the patch that removes the tracepoint on failure along with
> returning -ENODEV. That way, there will be no registered tracepoints
> that do not exist.
> 
> I'll also make sure that on module unload, the tracepoints are disabled
> for the module as well.

Do you mean that the tracepoint probe will be unregistered from within
tracepoint.c whenever all modules containing tracepoint call sites are
unloaded ? If so, how do you plan to handle ownership of the "name",
"probe" and "data" pointer ? They belong to the tracer. Would they simply
leak ?

> 
> Then, you can simply add a module notifier that does the work that you
> like, and save and restore the state of named tracepoints and enabled
> them on module load. Just set the priority of the notifier to 1
> so that it runs after the tracepoint notifier that adds the new
> tracepoints to the system.

I don't mind the extra work on the LTTng side at all. What I am concerned
about are changes that would make the tracepoint API sloppy.

> 
> > 
> > Looks like we can have it both ways. A way that works well for the
> > kernel, and a way that works well for you. But your module will need to
> > do the heavy work for what you want.
> > 
> > To me, a tracepoint should only be enabled when it exists. If it is
> > enabled in module when the module is unloaded, then it should be
> > removed after the module has left. If the module is loaded again, it is
> > up to the user (or your module) to enable that tracepoint again.
> 
> I want to point out that LTTng should not be dictating the way the
> kernel works, but it should be the other way around.

I don't care about doing extra work in LTTng, no worries about that.
I'm just trying to ensure all the corner cases are thought through
when a change such as this is proposed in a core infrastructure like
tracepoints.

Thanks,

Mathieu

> 
> -- Steve
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Wed, 12 Mar 2014 16:05:36 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> ----- Original Message -----
> > From: "Steven Rostedt" <rostedt@goodmis.org>
> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> > Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> > Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> > <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> > <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> > "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>
> > Sent: Wednesday, March 12, 2014 11:46:11 AM
> > Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> > 
> > 
> > To sum up this thread, and get the signal vs noise ratio up.
> > 
> > On Wed, 12 Mar 2014 11:11:00 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > 
> > > The solution I like the most that I believe will work for both of us,
> > > is to to move this magic "enable tracepoint in the future" to your
> > > LTTng module. Have your module register a module load and unload handler
> > > to be able to see the tracepoints that exist in the module, and you can
> > > enable them then. When a module is unloaded, your module can do the
> > > accounting to record that, and the state of its tracepoints.
> > 
> > This is my final proposal.
> > 
> > I'll add the patch that removes the tracepoint on failure along with
> > returning -ENODEV. That way, there will be no registered tracepoints
> > that do not exist.
> > 
> > I'll also make sure that on module unload, the tracepoints are disabled
> > for the module as well.
> 
> Do you mean that the tracepoint probe will be unregistered from within
> tracepoint.c whenever all modules containing tracepoint call sites are
> unloaded ? If so, how do you plan to handle ownership of the "name",
> "probe" and "data" pointer ? They belong to the tracer. Would they simply
> leak ?

Are you telling me it's not possible to delete the entire probe?

What I'm proposing is to do what the trace events do. Delete everything
associated to the tracepoints associated to the module.

> 
> > 
> > Then, you can simply add a module notifier that does the work that you
> > like, and save and restore the state of named tracepoints and enabled
> > them on module load. Just set the priority of the notifier to 1
> > so that it runs after the tracepoint notifier that adds the new
> > tracepoints to the system.
> 
> I don't mind the extra work on the LTTng side at all. What I am concerned
> about are changes that would make the tracepoint API sloppy.

I believe they are currently sloppy.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>
> Sent: Wednesday, March 12, 2014 12:18:13 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 16:05:36 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> > ----- Original Message -----
> > > From: "Steven Rostedt" <rostedt@goodmis.org>
> > > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> > > Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org,
> > > "Ingo Molnar" <mingo@kernel.org>, "Frederic
> > > Weisbecker" <fweisbec@gmail.com>, "Andrew Morton"
> > > <akpm@linux-foundation.org>, "Johannes Berg"
> > > <johannes.berg@intel.com>, "Linus Torvalds"
> > > <torvalds@linux-foundation.org>, "Peter Zijlstra"
> > > <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg
> > > Kroah-Hartman" <gregkh@linuxfoundation.org>,
> > > "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell"
> > > <rusty@rustcorp.com.au>
> > > Sent: Wednesday, March 12, 2014 11:46:11 AM
> > > Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not
> > > set via debugfs
> > > 
> > > 
> > > To sum up this thread, and get the signal vs noise ratio up.
> > > 
> > > On Wed, 12 Mar 2014 11:11:00 -0400
> > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > > 
> > > 
> > > > The solution I like the most that I believe will work for both of us,
> > > > is to to move this magic "enable tracepoint in the future" to your
> > > > LTTng module. Have your module register a module load and unload
> > > > handler
> > > > to be able to see the tracepoints that exist in the module, and you can
> > > > enable them then. When a module is unloaded, your module can do the
> > > > accounting to record that, and the state of its tracepoints.
> > > 
> > > This is my final proposal.
> > > 
> > > I'll add the patch that removes the tracepoint on failure along with
> > > returning -ENODEV. That way, there will be no registered tracepoints
> > > that do not exist.
> > > 
> > > I'll also make sure that on module unload, the tracepoints are disabled
> > > for the module as well.
> > 
> > Do you mean that the tracepoint probe will be unregistered from within
> > tracepoint.c whenever all modules containing tracepoint call sites are
> > unloaded ? If so, how do you plan to handle ownership of the "name",
> > "probe" and "data" pointer ? They belong to the tracer. Would they simply
> > leak ?
> 
> Are you telling me it's not possible to delete the entire probe?

The ownership flow is the following:

1) Tracer creates name, probe, data objects. The probe can be typically
   code within a probe provider module, which needs to have a reference
   count incremented. The name and data objects can be dynamically allocated,
   or in some special cases part of a probe provider module (again with
   refcount incremented).

2) The tracer registers the tracepoint probe. If registration returns 0,
   the tracer should not free those elements until it calls tracepoint
   probe unregister for that (name, probe, data) tuple.

3) Tracer calls tracepoint probe unregister for the (name, probe, data) tuple.

4) Tracer calls tracepoint_synchronize_unregister() to ensure quiescence
   of tracepoint call sites wrt the probe that has just been unregistered.

5) Tracer can free/unref the probe provider module.

> 
> What I'm proposing is to do what the trace events do. Delete everything
> associated to the tracepoints associated to the module.

Is your intent to have a module "going" notifier in tracepoint.c managing
ownership of objects it does not own ? If not, I guess I'm not understanding
your proposal fully.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Wed, 12 Mar 2014 16:39:55 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:


> > Are you telling me it's not possible to delete the entire probe?
> 
> The ownership flow is the following:
> 
> 1) Tracer creates name, probe, data objects. The probe can be typically
>    code within a probe provider module, which needs to have a reference
>    count incremented. The name and data objects can be dynamically allocated,
>    or in some special cases part of a probe provider module (again with
>    refcount incremented).
> 
> 2) The tracer registers the tracepoint probe. If registration returns 0,
>    the tracer should not free those elements until it calls tracepoint
>    probe unregister for that (name, probe, data) tuple.
> 
> 3) Tracer calls tracepoint probe unregister for the (name, probe, data) tuple.

We can make the registered tracepoint up the mod count to prevent it
from unloading. But that probably defeats the purpose.

> 
> 4) Tracer calls tracepoint_synchronize_unregister() to ensure quiescence
>    of tracepoint call sites wrt the probe that has just been unregistered.

Tracepoints should all be disabled when the module is unloaded. If you
have a tracepoint callback still being called when the module is
unloaded then something is seriously wrong. That means the callback
will go back to the trace_foo() call which is in the module code and
will no longer exist. Kernel panic is the result.


> 
> 5) Tracer can free/unref the probe provider module.

I'm a bit confused at what you are doing. As this is totally unrelated
to anything that happens in the kernel.

> 
> > 
> > What I'm proposing is to do what the trace events do. Delete everything
> > associated to the tracepoints associated to the module.
> 
> Is your intent to have a module "going" notifier in tracepoint.c managing
> ownership of objects it does not own ? If not, I guess I'm not understanding
> your proposal fully.

Well, actually that's exactly what the trace_event code does. It
disables any event of the module that happens to be enabled.

Look at event_remove() in kernel/trace/trace_events.c

On module unload, the events are destroyed.

Thus, what your module should do, is exactly what event_remove() does.
On module unload, you unregister any of the tracepoints that were
registered. Just like any other module resource. If you request a
resource on the behalf of a module, it is up to you to free it when the
module is unloaded.

The tracepoint code will just destroy what it set up when the module
was loaded. It's up to your module to clean up the allocations that you
made when the module was loaded on unload. Just like we do for all
other resources.

Mathieu, stop thinking that tracepoints are special. They are not.


-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>
> Sent: Wednesday, March 12, 2014 1:50:59 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 16:39:55 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> 
> > > Are you telling me it's not possible to delete the entire probe?
> > 
> > The ownership flow is the following:
> > 
> > 1) Tracer creates name, probe, data objects. The probe can be typically
> >    code within a probe provider module, which needs to have a reference
> >    count incremented. The name and data objects can be dynamically
> >    allocated,
> >    or in some special cases part of a probe provider module (again with
> >    refcount incremented).
> > 
> > 2) The tracer registers the tracepoint probe. If registration returns 0,
> >    the tracer should not free those elements until it calls tracepoint
> >    probe unregister for that (name, probe, data) tuple.
> > 
> > 3) Tracer calls tracepoint probe unregister for the (name, probe, data)
> > tuple.
> 
> We can make the registered tracepoint up the mod count to prevent it
> from unloading. But that probably defeats the purpose.

It seems more flexible to me to let this be handled by the tracer rather than
tracepoint.c.

> 
> > 
> > 4) Tracer calls tracepoint_synchronize_unregister() to ensure quiescence
> >    of tracepoint call sites wrt the probe that has just been unregistered.
> 
> Tracepoints should all be disabled when the module is unloaded.

This is only true for the tracepoint call sites located within this module.

> If you
> have a tracepoint callback still being called when the module is
> unloaded then something is seriously wrong. That means the callback
> will go back to the trace_foo() call which is in the module code and
> will no longer exist. Kernel panic is the result.

I agree that the specific call sites within an unloaded module need to be
already quiescent by the time the module is unmapped from memory, no argument
there.

The scenario I'm painting here (ownership of name, probe and data through the
5 steps) is the common "enable tracing, disable tracing" scenario, where the
module is not necessarily unloaded.

> 
> 
> > 
> > 5) Tracer can free/unref the probe provider module.
> 
> I'm a bit confused at what you are doing. As this is totally unrelated
> to anything that happens in the kernel.

The objects "name" and "data" can be dynamically allocated, and need to be
freed at some point. The probe provider module need to have its reference
count released after its probe function is unregistered. However, there
may still be tracepoint call sites in flights (again, this is a normal
enable tracing/disable tracing scenario, no module unload).

I detailed this scenario to answer your question "Are you telling me it's
not possible to delete the entire probe?", thus showing what the ownership
of objects passed to register/unregister is, to shed some light on why
it's currently not possible to unregister a tracepoint probe from
tracepoint.c, because it does not have ownership of those objects.

> 
> > 
> > > 
> > > What I'm proposing is to do what the trace events do. Delete everything
> > > associated to the tracepoints associated to the module.
> > 
> > Is your intent to have a module "going" notifier in tracepoint.c managing
> > ownership of objects it does not own ? If not, I guess I'm not
> > understanding
> > your proposal fully.
> 
> Well, actually that's exactly what the trace_event code does. It
> disables any event of the module that happens to be enabled.
> 
> Look at event_remove() in kernel/trace/trace_events.c
> 
> On module unload, the events are destroyed.

Isn't trace_event.c responsible for dealing with tracepoint probes rather
than call sites ? This is quite different. A tracepoint probe "foo" is only
located within a single module (the one you are unloading here). However,
if you try to unload a module that contains the callsite "foo", you have no
guarantee that no other modules also contain this callsite, and therefore
you cannot destroy the associated name, probe, nor data objects.

> 
> Thus, what your module should do, is exactly what event_remove() does.
> On module unload, you unregister any of the tracepoints that were
> registered. Just like any other module resource. If you request a
> resource on the behalf of a module, it is up to you to free it when the
> module is unloaded.

You seem to try to apply a logic that works in the case of the probes
defined by trace event to tracepoint call sites, but the fact is that
they are very different. Or again perhaps I'm just on the wrong track.

> 
> The tracepoint code will just destroy what it set up when the module
> was loaded. It's up to your module to clean up the allocations that you
> made when the module was loaded on unload. Just like we do for all
> other resources.
> 
> Mathieu, stop thinking that tracepoints are special. They are not.

I'm trying to understand how module going of tracepoint probes and
call sites can be considered the same. What am I missing ?

Thanks,

Mathieu

> 
> 
> -- Steve
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Wed, 12 Mar 2014 18:47:15 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
 
> > On module unload, the events are destroyed.
> 
> Isn't trace_event.c responsible for dealing with tracepoint probes rather
> than call sites ? This is quite different. A tracepoint probe "foo" is only
> located within a single module (the one you are unloading here). However,
> if you try to unload a module that contains the callsite "foo", you have no
> guarantee that no other modules also contain this callsite, and therefore
> you cannot destroy the associated name, probe, nor data objects.

No module should have the same tracepoint name as another module.
That's just broken. Although we do not technically enforce this, in
practice that has been the case.


> 
> > 
> > Thus, what your module should do, is exactly what event_remove() does.
> > On module unload, you unregister any of the tracepoints that were
> > registered. Just like any other module resource. If you request a
> > resource on the behalf of a module, it is up to you to free it when the
> > module is unloaded.
> 
> You seem to try to apply a logic that works in the case of the probes
> defined by trace event to tracepoint call sites, but the fact is that
> they are very different. Or again perhaps I'm just on the wrong track.
> 
> > 
> > The tracepoint code will just destroy what it set up when the module
> > was loaded. It's up to your module to clean up the allocations that you
> > made when the module was loaded on unload. Just like we do for all
> > other resources.
> > 
> > Mathieu, stop thinking that tracepoints are special. They are not.
> 
> I'm trying to understand how module going of tracepoint probes and
> call sites can be considered the same. What am I missing ?

What creates the tracepoint probe? For all purposes, it should be
either created on boot up (on core tracepoints), or when a module is
loaded.

Two modules should not have the same name. Is there any duplicate
tracepoints you are aware of. Namespace collisions in tracepoints
should be avoided, as that would cause people to trace things they did
not intend on tracing.

That should be a new patch as well. Enforce unique tracepoint names.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Wed, 12 Mar 2014 14:58:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Two modules should not have the same name. Is there any duplicate
> tracepoints you are aware of. Namespace collisions in tracepoints
> should be avoided, as that would cause people to trace things they did
> not intend on tracing.
> 
> That should be a new patch as well. Enforce unique tracepoint names.

This may be why you are not understanding what I want. It's the way
things are implemented today, which I believe are wrong. I see what you
did. You have probes that are registered, and tracepoints that are
where the code lies. You just add and remove probes from a hash list,
and then you loop through all the tracepoints seeing if the iter->name
matches a probe->name.

I'm fine with keeping the probe separate, but there really should be
no more than just a one to one mapping between probes and tracepoints.
Have the probe point to the matching tracepoint. The probe is
registered, it enables the tracepoint static key, when it's ref count
goes to zero, it disables the tracepoint static key. We can get rid of
that loop then, as well as the duplicate names between probes and
tracepoints.

Here's the steps we should take:

1) Prevent duplicate tracepoints. They are just namespace collisions
that we already try to avoid. How to do this? We may need to add a
hlist_node to the tracepoint structure, and keep them in a hash by name.
Check for collisions when the name is added to the hash.

2) Change the way tracepoints are enabled. Do not do a loop of all
tracepoints, but instead have the first probe of a tracepoint enable
it, and the last one to disable it. This would require a pointer from
the probe to the tracepoint it represents. Again, it should not
represent more than one.

3) On module unload, it would be the responsibility of the user to
unload all the tracepoints that may have been enabled for a module. We
can add a mod pointer in the probe to make this easier, as well as to
the tp_module structure.

The way tracepoints are today are to handle two completely different
tracepoints with the same name. That should be avoided, and will make
things much less complex.

Then you can easily handle the accounting of modules loading and
unloading in your module, and the tracepoint code will match what the
rest of the kernel does for resource management.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>
> Sent: Wednesday, March 12, 2014 3:30:06 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 14:58:02 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > Two modules should not have the same name. Is there any duplicate
> > tracepoints you are aware of. Namespace collisions in tracepoints
> > should be avoided, as that would cause people to trace things they did
> > not intend on tracing.
> > 
> > That should be a new patch as well. Enforce unique tracepoint names.
> 
> This may be why you are not understanding what I want. It's the way
> things are implemented today, which I believe are wrong. I see what you
> did. You have probes that are registered, and tracepoints that are
> where the code lies. You just add and remove probes from a hash list,
> and then you loop through all the tracepoints seeing if the iter->name
> matches a probe->name.
> 
> I'm fine with keeping the probe separate, but there really should be
> no more than just a one to one mapping between probes and tracepoints.
> Have the probe point to the matching tracepoint. The probe is
> registered, it enables the tracepoint static key, when it's ref count
> goes to zero, it disables the tracepoint static key. We can get rid of
> that loop then, as well as the duplicate names between probes and
> tracepoints.

Right there, this is not possible for a few reasons, namely:

- loop unrolling performed by the compiler can duplicate a tracepoint,
  even if it is only there once in the source code,
- inlining performed by the compiler may do the same,
- LTO, whenever it will start being used for the kernel, may do the same,
  and also spread call sites across modules.

There can be no 1 to 1 mapping between a probe function and a callsite
due to those compilers optimisations, even if we enforce the strictest
coding style rules possible on their use.

Thanks,

Mathieu


> 
> Here's the steps we should take:
> 
> 1) Prevent duplicate tracepoints. They are just namespace collisions
> that we already try to avoid. How to do this? We may need to add a
> hlist_node to the tracepoint structure, and keep them in a hash by name.
> Check for collisions when the name is added to the hash.
> 
> 2) Change the way tracepoints are enabled. Do not do a loop of all
> tracepoints, but instead have the first probe of a tracepoint enable
> it, and the last one to disable it. This would require a pointer from
> the probe to the tracepoint it represents. Again, it should not
> represent more than one.
> 
> 3) On module unload, it would be the responsibility of the user to
> unload all the tracepoints that may have been enabled for a module. We
> can add a mod pointer in the probe to make this easier, as well as to
> the tp_module structure.
> 
> The way tracepoints are today are to handle two completely different
> tracepoints with the same name. That should be avoided, and will make
> things much less complex.
> 
> Then you can easily handle the accounting of modules loading and
> unloading in your module, and the tracepoint code will match what the
> rest of the kernel does for resource management.
> 
> -- Steve
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>
> Sent: Wednesday, March 12, 2014 2:58:02 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 18:47:15 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>  
> > > On module unload, the events are destroyed.
> > 
> > Isn't trace_event.c responsible for dealing with tracepoint probes rather
> > than call sites ? This is quite different. A tracepoint probe "foo" is only
> > located within a single module (the one you are unloading here). However,
> > if you try to unload a module that contains the callsite "foo", you have no
> > guarantee that no other modules also contain this callsite, and therefore
> > you cannot destroy the associated name, probe, nor data objects.
> 
> No module should have the same tracepoint name as another module.
> That's just broken. Although we do not technically enforce this, in
> practice that has been the case.

I agree with you on this.

I now notice that all the tracepoints within kernel headers have been removed,
this is why we're talking past each other. As soon as there is a tracepoint
within a static inline function in a header shared between modules, you end up
having instances of tracepoint call sites with the same names within different
modules.

So I understand that you wish to banish tracepoints from static inline
functions within headers to ensure they only appear within a single module.
This seems to be a step backward, but let's assume we stick to that rule.
Then how do you envision dealing with Link-Time Optimisations (LTO) ?

> > 
> > > 
> > > Thus, what your module should do, is exactly what event_remove() does.
> > > On module unload, you unregister any of the tracepoints that were
> > > registered. Just like any other module resource. If you request a
> > > resource on the behalf of a module, it is up to you to free it when the
> > > module is unloaded.
> > 
> > You seem to try to apply a logic that works in the case of the probes
> > defined by trace event to tracepoint call sites, but the fact is that
> > they are very different. Or again perhaps I'm just on the wrong track.
> > 
> > > 
> > > The tracepoint code will just destroy what it set up when the module
> > > was loaded. It's up to your module to clean up the allocations that you
> > > made when the module was loaded on unload. Just like we do for all
> > > other resources.
> > > 
> > > Mathieu, stop thinking that tracepoints are special. They are not.
> > 
> > I'm trying to understand how module going of tracepoint probes and
> > call sites can be considered the same. What am I missing ?
> 
> What creates the tracepoint probe? For all purposes, it should be
> either created on boot up (on core tracepoints), or when a module is
> loaded.

Yep, agreed.

> 
> Two modules should not have the same name. Is there any duplicate
> tracepoints you are aware of. Namespace collisions in tracepoints
> should be avoided, as that would cause people to trace things they did
> not intend on tracing.
> 
> That should be a new patch as well. Enforce unique tracepoint names.

There are no duplicate tracepoint that I'm aware of in the kernel (or
very few, and they are in similar functions in the same module).

This only leaves tracepoints in header files and the impact of LTO as
requirements for having tracepoint callsites with the same name across
modules.

Thoughts ?

Thanks,

Mathieu

> 
> -- Steve
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Andi Kleen]

> So I understand that you wish to banish tracepoints from static inline
> functions within headers to ensure they only appear within a single module.
> This seems to be a step backward, but let's assume we stick to that rule.
> Then how do you envision dealing with Link-Time Optimisations (LTO) ?

I assume it uses the file name defines set by Kbuild? These don't change with
LTO. It's whatever was specified at compile time. Also LTO doesn't
inline over module boundaries (if the module is not built in)

-Andi

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Andi Kleen" <andi@firstfloor.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Steven Rostedt" <rostedt@goodmis.org>, "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo
> Molnar" <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>,
> "Johannes Berg" <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>, "Andi Kleen" <andi@firstfloor.org>
> Sent: Wednesday, March 12, 2014 4:35:15 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> > So I understand that you wish to banish tracepoints from static inline
> > functions within headers to ensure they only appear within a single module.
> > This seems to be a step backward, but let's assume we stick to that rule.
> > Then how do you envision dealing with Link-Time Optimisations (LTO) ?
> 
> I assume it uses the file name defines set by Kbuild?

Just to make sure I understand your question: I understand that you are asking
whether tracepoints use file name defines at all in the naming of a tracepoint.
The answer to this question is: No, they do not.

> These don't change with
> LTO. It's whatever was specified at compile time. Also LTO doesn't
> inline over module boundaries (if the module is not built in)

Good to know. Can it inline core kernel functions into a module ?

Thanks,

Mathieu

> 
> -Andi
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Andi Kleen]

On Wed, Mar 12, 2014 at 08:47:07PM +0000, Mathieu Desnoyers wrote:
> ----- Original Message -----
> > From: "Andi Kleen" <andi@firstfloor.org>
> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> > Cc: "Steven Rostedt" <rostedt@goodmis.org>, "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo
> > Molnar" <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>,
> > "Johannes Berg" <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> > <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> > "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>, "Andi Kleen" <andi@firstfloor.org>
> > Sent: Wednesday, March 12, 2014 4:35:15 PM
> > Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> > 
> > > So I understand that you wish to banish tracepoints from static inline
> > > functions within headers to ensure they only appear within a single module.
> > > This seems to be a step backward, but let's assume we stick to that rule.
> > > Then how do you envision dealing with Link-Time Optimisations (LTO) ?
> > 
> > I assume it uses the file name defines set by Kbuild?
> 
> Just to make sure I understand your question: I understand that you are asking
> whether tracepoints use file name defines at all in the naming of a tracepoint.
> The answer to this question is: No, they do not.

Ok. It uses kallsyms? That can change of course.
> 
> > These don't change with
> > LTO. It's whatever was specified at compile time. Also LTO doesn't
> > inline over module boundaries (if the module is not built in)
> 
> Good to know. Can it inline core kernel functions into a module ?

Each module and the main kernel are currently LTO'ed separately.

In theory it would be possible to change this, but likely at some
compile time cost.

-Andi

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Andi Kleen" <andi@firstfloor.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Andi Kleen" <andi@firstfloor.org>, "Steven Rostedt" <rostedt@goodmis.org>, "Frank Ch. Eigler" <fche@redhat.com>,
> linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Andrew
> Morton" <akpm@linux-foundation.org>, "Johannes Berg" <johannes.berg@intel.com>, "Linus Torvalds"
> <torvalds@linux-foundation.org>, "Peter Zijlstra" <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>,
> "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell"
> <rusty@rustcorp.com.au>
> Sent: Wednesday, March 12, 2014 11:15:01 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, Mar 12, 2014 at 08:47:07PM +0000, Mathieu Desnoyers wrote:
> > ----- Original Message -----
> > > From: "Andi Kleen" <andi@firstfloor.org>
> > > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> > > Cc: "Steven Rostedt" <rostedt@goodmis.org>, "Frank Ch. Eigler"
> > > <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo
> > > Molnar" <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>,
> > > "Andrew Morton" <akpm@linux-foundation.org>,
> > > "Johannes Berg" <johannes.berg@intel.com>, "Linus Torvalds"
> > > <torvalds@linux-foundation.org>, "Peter Zijlstra"
> > > <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg
> > > Kroah-Hartman" <gregkh@linuxfoundation.org>,
> > > "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell"
> > > <rusty@rustcorp.com.au>, "Andi Kleen" <andi@firstfloor.org>
> > > Sent: Wednesday, March 12, 2014 4:35:15 PM
> > > Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not
> > > set via debugfs
> > > 
> > > > So I understand that you wish to banish tracepoints from static inline
> > > > functions within headers to ensure they only appear within a single
> > > > module.
> > > > This seems to be a step backward, but let's assume we stick to that
> > > > rule.
> > > > Then how do you envision dealing with Link-Time Optimisations (LTO) ?
> > > 
> > > I assume it uses the file name defines set by Kbuild?
> > 
> > Just to make sure I understand your question: I understand that you are
> > asking
> > whether tracepoints use file name defines at all in the naming of a
> > tracepoint.
> > The answer to this question is: No, they do not.
> 
> Ok. It uses kallsyms? That can change of course.

As I just replied to Steven, I now see that I mixed up concerns about
static keys, and the prior kernel markers, with tracepoint concerns.
The way they are implemented are very much different (Hey! I should know,
I wrote that code some 6 years ago!) ;)

> > 
> > > These don't change with
> > > LTO. It's whatever was specified at compile time. Also LTO doesn't
> > > inline over module boundaries (if the module is not built in)
> > 
> > Good to know. Can it inline core kernel functions into a module ?
> 
> Each module and the main kernel are currently LTO'ed separately.
> 
> In theory it would be possible to change this, but likely at some
> compile time cost.

OK, thanks for the explanations!

Mathieu

> 
> -Andi
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Wed, 12 Mar 2014 19:51:01 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> This only leaves tracepoints in header files and the impact of LTO as
> requirements for having tracepoint callsites with the same name across
> modules.

The only thing that needs to be unique is the struct tracepoint
__tracepoint_##name. There should not be any duplicates of those. I
can't see how the LTO would duplicate a data structure without screwing
everything (not just tracepoints) up.

We can still have more than one trace_##name() called, as that is
handled by the static key.

Note, I'm scrambling to get ready for my trip tomorrow. Thus, I'm not
as much at the computer. I may work on some patches in my 6 hour
layover though.

-- Steve

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>, "Andi Kleen" <andi@firstfloor.org>
> Sent: Wednesday, March 12, 2014 8:49:07 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 19:51:01 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> > This only leaves tracepoints in header files and the impact of LTO as
> > requirements for having tracepoint callsites with the same name across
> > modules.
> 
> The only thing that needs to be unique is the struct tracepoint
> __tracepoint_##name. There should not be any duplicates of those. I
> can't see how the LTO would duplicate a data structure without screwing
> everything (not just tracepoints) up.
> 
> We can still have more than one trace_##name() called, as that is
> handled by the static key.

Hrm, I seem to have mixed up the concerns regarding compiler
optimisations between the static keys with those related to tracepoints
(including that their predecessors "kernel markers" worked more like
static keys than tracepoint). I did work in both area pretty much at the
same time back in 2007-2008.

Having gotten my head back up straight, I now see the point in your
proposal. Thanks for bearing with me.

Even if we ever want to have tracepoints within header files, as long
as we have the DECLARE_TRACE() within the header, and a DEFINE_TRACE()
in one location in the loaded kernel image (or a loaded module we depend
on), it should be possible too. All tracepoint.c cares about is the site
defined by DEFINE_TRACE().

> 
> Note, I'm scrambling to get ready for my trip tomorrow. Thus, I'm not
> as much at the computer. I may work on some patches in my 6 hour
> layover though.

Allright. Have a good trip!

Thanks,

Mathieu

> 
> -- Steve
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> To: "Steven Rostedt" <rostedt@goodmis.org>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>, "Andi Kleen" <andi@firstfloor.org>
> Sent: Wednesday, March 12, 2014 11:10:53 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> ----- Original Message -----
> > From: "Steven Rostedt" <rostedt@goodmis.org>
> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> > Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org,
> > "Ingo Molnar" <mingo@kernel.org>, "Frederic
> > Weisbecker" <fweisbec@gmail.com>, "Andrew Morton"
> > <akpm@linux-foundation.org>, "Johannes Berg"
> > <johannes.berg@intel.com>, "Linus Torvalds"
> > <torvalds@linux-foundation.org>, "Peter Zijlstra"
> > <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg
> > Kroah-Hartman" <gregkh@linuxfoundation.org>,
> > "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell"
> > <rusty@rustcorp.com.au>, "Andi Kleen" <andi@firstfloor.org>
> > Sent: Wednesday, March 12, 2014 8:49:07 PM
> > Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not
> > set via debugfs
> > 
> > On Wed, 12 Mar 2014 19:51:01 +0000 (UTC)
> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > 
> > > This only leaves tracepoints in header files and the impact of LTO as
> > > requirements for having tracepoint callsites with the same name across
> > > modules.
> > 
> > The only thing that needs to be unique is the struct tracepoint
> > __tracepoint_##name. There should not be any duplicates of those. I
> > can't see how the LTO would duplicate a data structure without screwing
> > everything (not just tracepoints) up.
> > 
> > We can still have more than one trace_##name() called, as that is
> > handled by the static key.
> 
> Hrm, I seem to have mixed up the concerns regarding compiler
> optimisations between the static keys with those related to tracepoints
> (including that their predecessors "kernel markers" worked more like
> static keys than tracepoint). I did work in both area pretty much at the
> same time back in 2007-2008.
> 
> Having gotten my head back up straight, I now see the point in your
> proposal. Thanks for bearing with me.
> 
> Even if we ever want to have tracepoints within header files, as long
> as we have the DECLARE_TRACE() within the header, and a DEFINE_TRACE()
> in one location in the loaded kernel image (or a loaded module we depend
> on), it should be possible too. All tracepoint.c cares about is the site
> defined by DEFINE_TRACE().

I just prepared a patch implementing your idea. I will post it as RFC very
soon. The callers will still need to be adapted, so this is just to show
the tracepoint.c/h modifications.

Thanks!

Mathieu

> 
> > 
> > Note, I'm scrambling to get ready for my trip tomorrow. Thus, I'm not
> > as much at the computer. I may work on some patches in my 6 hour
> > layover though.
> 
> Allright. Have a good trip!
> 
> Thanks,
> 
> Mathieu
> 
> > 
> > -- Steve
> > 
> 
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

(FYI, this is the leg of the thread that can be considered lower signal/noise
ratio if the underlying arguments are not interesting to you. If they are, please
read on!) :-)

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>
> Sent: Wednesday, March 12, 2014 11:11:00 AM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 14:24:54 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> > ----- Original Message -----
> > > From: "Steven Rostedt" <rostedt@goodmis.org>
> > > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> > > Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org,
> > > "Ingo Molnar" <mingo@kernel.org>, "Frederic
> > > Weisbecker" <fweisbec@gmail.com>, "Andrew Morton"
> > > <akpm@linux-foundation.org>, "Johannes Berg"
> > > <johannes.berg@intel.com>
> > > Sent: Tuesday, March 11, 2014 3:13:57 PM
> > > Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not
> > > set via debugfs
> > > 
> > 
> > For those just added in CC, https://lkml.org/lkml/2014/3/11/431 provides
> > the
> > missing context. The discussion is about adding module load parameters as
> > ABI
> > for end users to enable tracepoints in modules (Steven has proposed the
> > idea
> > a couple of times in the past, I am against for the reasons I am exposing
> > below). This discussion will likely have deep impact on other changes
> > Steven
> > is proposing for tracepoint.c (such as making tracepoint_probe_register()
> > return -ENODEV when no tracepoint call site is currently loaded for a given
> > tracepoint), so I think the audience should be broadened.
> 
> Please tell me one in-tree user that will be impacted?

I'm talking about all users of tracepoints, all in tree and out of tree
users. We need the design and API for tracepoint to stay sound, and for that
we need to study carefully the impact of the changes you propose on module
load/unload scenarios.

> 
> > 
> > > On Tue, 11 Mar 2014 17:34:23 +0000 (UTC)
> > > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > >  
> > > > > Who can load modules not as root?? That is utterly broken. As once
> > > > > you
> > > > > can load a module, YOU ARE ROOT.
> > > > 
> > > > udevd runs as root, and listens to events such as USB hotplug, and
> > > > loads
> > > > modules
> > > > in the back of users. The users don't need to be root for this to
> > > > happen.
> > > 
> > > udevd may run as a proxy for users. But it is still a root user.
> > > Sysadmins are the ones that set up udevd.
> > 
> > Uh ? There are tons of distributions that setup udevd for their users, and
> > no sysadmin has to interact with it.
> 
> And the distribution just magically installs itself onto a computer? No,
> a user does, and in doing so, they set up udev. A person installing a
> distribution is a sysadmin. Even if it's grandma installing it herself.
> She owns the box, no one else is doing anything for her (unless it's
> your grandma).

In a corporate environment, it's pretty much never the end user who ends
up being the system administrator of his machine.

> 
> > 
> > > If you own your own box, you are technically the sysadmin for it.
> > 
> > If you use a box owned by your company, running Linux, but enforcing module
> > signature, you might be physically allowed to connect USB devices, and let
> > udevd load the appropriate modules for the connected devices, but that does
> > not mean you have root access at all. But having the ability to trace the
> > said computer from a user having "tracing" privileges is very valuable to
> > help debugging that machine.
> 
> Tracing is a root privilege.

Tracing the kernel should require root privilege for the process interacting
with the kernel ABI, I agree with that part. However, the end user of the
machine should not need to be in a root shell to interact with tracing. For
instance, if the tracer only provides summarized information pinpointing
issues, the system administrator could very well allow the end user to use
tracing without being root.

An example of this is the way laptops nowadays handle wifi connexions through
a root daemon setting up the network (interfacing with kernel ABI), but which
gets the user interaction though a user interface running in user context.

> 
> > 
> > Even without going that far, if you want tracing to be widely usable to
> > pinpoint issues within Linux and/or its applications or libraries, you
> > basically have to remove most user interactions from the picture, and let
> > automated tools gather tracing data to perform analysis. As soon as you
> > require users to fiddle with udev configuration, or pass special parameters
> > to modprobe, or if you start coupling the tracing infrastructure with the
> > module configuration, you just made tracing unusable for a vast majority of
> > Linux distribution users out there.
> 
> This seems very specific to a single tracing tool that happens not to
> be in the kernel tree.

And the reason for that is because the said kernel community has been deaf to
the requirements of the user-base we target. Unlike Ftrace, we are not targeting
kernel developers, and unlike perf, we are not targeting sampling use-cases. Yes,
there is a partial infrastructure overlap, but LTTng has been rejected on the
ground of "fear of work duplication" which, frankly, makes no sense, since we are
targeting different use cases. Some of the infrastructure work needs to be done
in common, and some of the work needs to go in different directions.

> 
> > 
> > I've been trying to make my point that tracing should be first of all
> > _usable_ by non-expert Linux end users, but it seems that I'm completely
> > failing at getting the message through somehow. And believe me I've spent
> > years trying to voice this message. Perhaps I'm not cursing enough, perhaps
> > I'm not yelling enough. Rather than arguing, the path I'm currently taking
> > is to prove my point by putting the tools needed by my user community in
> > their hands.
> 
> I don't know why you are complaining. Think of this as your job
> security.

I think the tension that exists in the Linux tracing community is not helping
progress at all in this area. This is why I'm trying to re-open the dialogue.

> 
> > 
> > If to you the majority of Linux users means "kernel developers", then
> > I can see how our point of views cannot be reconciled easily. From my
> > point of view, there are far more users who need those tracing tools that
> > are not kernel developers.
> 
> No, but the majority of tracing users *are* kernel developers, or those
> that are experts in the system.

Is it that way because the current tracing tools are so unusable by
non kernel-developers that those are the only ones able to interact with
those tools ?

You seem to mix "knowledge needed to make sense of a trace" with "knowledge
needed to gather a trace". The first kind of knowledge can be put into
analysis tools, but they cannot be used widely if gathering a trace requires
people to be kernel experts too.

> 
> Why would grandma want to trace the kernel?

Perhaps because her kernel crashes and she hates to lose the emails she is
writing to her grandchildren when the crash happens, and because there is a
nice tool that allow her to click "yes, I want to report issues to my
favorite Linux distribution", which will then analyze the trace report and
come up with an automated kernel upgrade a few weeks later.

> 
> > 
> > > Modifications to udev require sudo
> > > privileges.
> > > 
> > > Tracepoints should not be something a non-sysadmin can modify or
> > > enable. If you want non root users to do so, create a proxy daemon like
> > > udevd to do it for them. But the kernel isn't going to allow that
> > > directly.
> > 
> > I never said the kernel should directly expose ABIs accessible by others
> > than root. Actually, this is the very reason we have a "lttng-sessiond"
> > daemon running as root to control kernel tracing. However, it can be
> > controlled by a "lttng" command line tool over unix sockets if the user
> > is part of the "tracing" group. Right there, this is the concept of your
> > proxy daemon. However, you seem to imply that this proxy daemon should
> > somehow fiddle with udevd and/or modprobe configuration to add temporary
> > system-wide parameters to how modules are loaded, depending on tracing
> > needs. Trying to couple something usually configured initially for a
> > large portion of a system's life (and even with tripwire validation that
> > /etc does not change over time) with something as transient as tracing
> > simply cannot work without causing havoc in the system configuration. So
> > no, having a tracing proxy daemon changing configuration of module
> > arguments cannot fly.
> 
> I actually never said that. I said that you could have a proxy to
> enable or disable the tracepoint. I don't know where you're getting
> this crap from. Yes, I have suggested in the past that the ideal
> approach is to have modprobe handle it, just like it handles other
> module changes for one time deals. And yes, tracing a module is a one
> time deal. But you seem to be getting in a tizzy thinking that's what I
> recommended yesterday.

This was the logical consequence of your recommendation of having a root proxy
daemon to handle this.


> 
> > 
> > > 
> > > > 
> > > > >  
> > > > > > fashion and is not suitable for the user-base we are targeting. I
> > > > > > seems
> > > > > > to
> > > > > > be a user experience disaster IMHO.
> > > > > 
> > > > > For your case only. But it is normal operation for normal uses of
> > > > > Linux.
> > > > 
> > > > AFAIK pretty much all distros use udev nowadays. Are you suggesting
> > > > that
> > > > all
> > > > users using udev and distribution kernels are not "normal uses of
> > > > Linux" ?
> > > 
> > > udev is root, and is modified by root users. A normal user can not just
> > > interact with udev. And sticking in a usb stick into a computer counts
> > > as a sysadmin operation, even if the person doesn't official have the
> > > title.
> > 
> > So if I understand your line of thought, you imply that anyone who can
> > plug a USB stick of any kind (memory stick, USB wifi card, etc) into a
> > computer should automatically have root access ? Is it just me, or it
> > makes no sense ?
> 
> I wouldn't want people to have access to any box I own to be able to
> just stick any usb device into the computer. Look at all the USB drivers
> we have. You think they are all perfect. I'm sure it wouldn't be too
> hard to find a bad USB device driver and make your own dongle to stick
> in, that can root the box.

Yes, no code is perfect, and that includes USB drivers. But in that line of
reasoning, why bother about security at all ? Since the ssh server code is
imperfect, you might as well publish your private key. Sorry, this line of
reasoning makes no sense.

> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > I'm OK as long as we have an elegant way forward. Ideally I would
> > > > > > have
> > > > > > prefered (1) to eliminate code duplication between tracers and
> > > > > > tracepoint
> > > > > > infrastructure (we have to reimplement a hash table similar to
> > > > > > tracepoints
> > > > > > within the tracer with solution (2)), but (2) technically works
> > > > > > too.
> > > > > 
> > > > > Here's what I propose then. We implement 2 for now. You can
> > > > > "duplicate"
> > > > > the code into your own work.
> > > > 
> > > > Works for me.
> > > > 
> > > > > Then we should be able to simplify the
> > > > > tracepoint code as it no longer will have the requirement to enable
> > > > > tracepoints that do not exist.
> > > > 
> > > > What happens for the case where we enable a tracepoint, and then the
> > > > only module containing a callsite of that tracepoint is unloaded, and
> > > > then reloaded ?
> > > 
> > > When a module is unloaded, it usually loses state. Is there any state
> > > that is maintained for a module being unloaded and reloaded again?
> > > Besides tracepoints? If not, then the module should lose its state for
> > > tracepoints as well.
> > 
> > What you're missing here is that the same tracepoint can appear within
> > more than one module.
> 
> So?
> 
> > 
> > I think there is a core, fundamental difference between the ways we see
> > tracing. You seem to see tracing as being solely part of the modules
> > that contain the caller tracing code. From my perspective, tracing fits
> > much better in the "aspect oriented programming" paradigm, since it spreads
> > the same "crosscutting concerns" across multiple modules. So yes, in AOP,
> > it makes perfect sense that tracing activation state goes beyond the
> > lifetime of a single loaded module: it spawns across core kernel, and zero,
> > one, or more loaded modules, independently of whether they are unloaded and
> > reloaded again.
> 
> Have your LTTNg module dictate that policy.

I'm OK with that, as long as the tracepoint infrastructure itself is consistent.

> 
> > 
> > 
> > So going back on the changes you proposed to the tracepoint infrastructure:
> > 
> > A) Making tracepoint_probe_register() unregister the probe and fail when
> >    there is no tracepoint call site present for a probe at that time:
> > 
> > With this approach, if the probe is registered when the module containing
> > the call site is loaded, and then that module is unloaded, it will leave
> > the probe in a registered state, leaving the tracer lying to the user,
> > letting them think there are currently callsites for this tracepoint when
> > there are none.
> 
> Hmm, currently the probe lies to the user when the tracepoint doesn't
> exist. We enable it and nothing happens. This BROKE IN TREE USERS!!!!

The root of the original problem here was that tracepoint.c was skipping
non-signed modules without printing any error. We added an printk error
message for this.

> 
> I suggest that we remove the probe when the module is unloaded. No
> lying. Either a tracepoint exists when you enable it, or it doesn't.

See other leg of this thread for the "high signal/noise" ratio technical
discussion.

> 
> 
> > 
> > B) Making tracepoint_probe_register() just return an error (-ENOEDV), but
> >    leaving the probe registered.
> > 
> > I think we have already agreed that this compromise solution is a weird API
> > choice, since an error condition must be treated as a success by the
> > caller.
> > Moreover, it has the same issue as (A) above: if the module is unloaded
> > after probe registration, the tracer is lying to the user, making them
> > think there is a call site loaded when there are none.
> 
> But isn't that what we do today?

No, currently tracepoint offers no API to query the state of tracepoint callsites
other than the seqfile iterator (which I proposed to removed in a RFC patch
yesterday).

> 
> > 
> > C) Adding module load arguments to specify which tracepoint should be
> >    enabled.
> > 
> > Please refer to my response above in this email to understand why I object.
> 
> I didn't suggest this in this thread. I only pointed out that is the
> most sane situation. But I've been trying to work with you, and you
> don't seem to see that.

I appreciate the compromise efforts you have made, but my points here are about
sound design/API, and I have technical concerns still unanswered (see other leg
of the thread). This is not about what LTTng needs, but about keeping the
tracepoint API consistent.

> 
> > 
> > Back to your original requirement: you want to let the tracer communicate
> > to end users whether a specific tracepoint has call sites or not. I'm
> > currently preparing a patch implementing a "tracepoint_callsite_count()"
> > API that will allow this, and it won't lie to the tracer like solutions
> > (A) and (B) above. Whenever the tracer queries the state, it will get the
> > current information about the number of enabled call sites, not some stale
> > data based on a prior registration error.
> 
> No, I don't want to know if this call site exists or not. This stupid
> "tracepoint_probe_register()" that activates a tracepoint by name. But
> if that tracepoint doesn't exist, it just returns normally as if
> everything is fine and dandy. WTF!
> 
> Look Mathieu, I've been trying to be nice here. This crap has broken
> in tree kernel users, and this magical "enable a non-existent
> tracepoint, so when one is created, it will be enabled" is only used by
> LTTng! NOBODY ELSE USES IT. This means that I can strip this usage out
> of the kernel, as there are no uses of it in the kernel.
> 
> What you don't seem to realize, is that your methods broke the stuff
> that is in the kernel, and you don't seem to give a shit about that.
> 
> The solution I like the most that I believe will work for both of us,
> is to to move this magic "enable tracepoint in the future" to your
> LTTng module. Have your module register a module load and unload handler
> to be able to see the tracepoints that exist in the module, and you can
> enable them then. When a module is unloaded, your module can do the
> accounting to record that, and the state of its tracepoints.
> 
> Looks like we can have it both ways. A way that works well for the
> kernel, and a way that works well for you. But your module will need to
> do the heavy work for what you want.
> 
> To me, a tracepoint should only be enabled when it exists. If it is
> enabled in module when the module is unloaded, then it should be
> removed after the module has left. If the module is loaded again, it is
> up to the user (or your module) to enable that tracepoint again.

I don't mind doing the heavy lifting in LTTng at all. My concern here
is (again) about keeping the tracepoint API semantically consistent when
module load/unload occur. A lot of thinking went into this when I initially
created tracepoint.c, and I don't want it to be degraded by a change that is
not thought thoroughly.

Thanks,

Mathieu


> 
> -- Steve
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Steven Rostedt]

On Wed, 12 Mar 2014 16:40:51 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
 
> > Please tell me one in-tree user that will be impacted?
> 
> I'm talking about all users of tracepoints, all in tree and out of tree
> users. We need the design and API for tracepoint to stay sound, and for that
> we need to study carefully the impact of the changes you propose on module
> load/unload scenarios.

Right now the current change actually fixes a bug. It tells the tracing
utility that the tracepoint that was to be enabled was or wasn't.


> > And the distribution just magically installs itself onto a computer? No,
> > a user does, and in doing so, they set up udev. A person installing a
> > distribution is a sysadmin. Even if it's grandma installing it herself.
> > She owns the box, no one else is doing anything for her (unless it's
> > your grandma).
> 
> In a corporate environment, it's pretty much never the end user who ends
> up being the system administrator of his machine.

And they shouldn't be tracing their modules. That should be up to the
corporate system administrators.


> > Tracing is a root privilege.
> 
> Tracing the kernel should require root privilege for the process interacting
> with the kernel ABI, I agree with that part. However, the end user of the
> machine should not need to be in a root shell to interact with tracing. For
> instance, if the tracer only provides summarized information pinpointing
> issues, the system administrator could very well allow the end user to use
> tracing without being root.
> 
> An example of this is the way laptops nowadays handle wifi connexions through
> a root daemon setting up the network (interfacing with kernel ABI), but which
> gets the user interaction though a user interface running in user context.

This is all out of scope with the latest proposal, so I will ignore it.

 
> > This seems very specific to a single tracing tool that happens not to
> > be in the kernel tree.
> 
> And the reason for that is because the said kernel community has been deaf to
> the requirements of the user-base we target. Unlike Ftrace, we are not targeting
> kernel developers, and unlike perf, we are not targeting sampling use-cases. Yes,
> there is a partial infrastructure overlap, but LTTng has been rejected on the
> ground of "fear of work duplication" which, frankly, makes no sense, since we are
> targeting different use cases. Some of the infrastructure work needs to be done
> in common, and some of the work needs to go in different directions.

Honestly, LTTng has been rejected because the maintainer of it tends to
sacrifice everything else to make tracing better.


> > 
> > Why would grandma want to trace the kernel?
> 
> Perhaps because her kernel crashes and she hates to lose the emails she is
> writing to her grandchildren when the crash happens, and because there is a
> nice tool that allow her to click "yes, I want to report issues to my
> favorite Linux distribution", which will then analyze the trace report and
> come up with an automated kernel upgrade a few weeks later.

A distro could set something like this up, with todays tools. If
grandma needed to start tracing, it was already too late for her to get
a report.



> > I actually never said that. I said that you could have a proxy to
> > enable or disable the tracepoint. I don't know where you're getting
> > this crap from. Yes, I have suggested in the past that the ideal
> > approach is to have modprobe handle it, just like it handles other
> > module changes for one time deals. And yes, tracing a module is a one
> > time deal. But you seem to be getting in a tizzy thinking that's what I
> > recommended yesterday.
> 
> This was the logical consequence of your recommendation of having a root proxy
> daemon to handle this.

You read too much into what I say. Like the "No signed-off-by". I was
actually telling you that tracepoints should not be enabled by non root
unless there was a daemon to control it. We both got off an tangents.


> > I wouldn't want people to have access to any box I own to be able to
> > just stick any usb device into the computer. Look at all the USB drivers
> > we have. You think they are all perfect. I'm sure it wouldn't be too
> > hard to find a bad USB device driver and make your own dongle to stick
> > in, that can root the box.
> 
> Yes, no code is perfect, and that includes USB drivers. But in that line of
> reasoning, why bother about security at all ? Since the ssh server code is
> imperfect, you might as well publish your private key. Sorry, this line of
> reasoning makes no sense.

ssh code is a small set of code compared to the thousands of random usb
drivers, including several that are in staging, that still happen to be
turned on by distros.


> > Have your LTTNg module dictate that policy.
> 
> I'm OK with that, as long as the tracepoint infrastructure itself is consistent.

We're working on that.

 
> > Hmm, currently the probe lies to the user when the tracepoint doesn't
> > exist. We enable it and nothing happens. This BROKE IN TREE USERS!!!!
> 
> The root of the original problem here was that tracepoint.c was skipping
> non-signed modules without printing any error. We added an printk error
> message for this.

Yes, but it still lies to the internal user in this case. Which is
wrong.

> 
> > 
> > I suggest that we remove the probe when the module is unloaded. No
> > lying. Either a tracepoint exists when you enable it, or it doesn't.
> 
> See other leg of this thread for the "high signal/noise" ratio technical
> discussion.
> 
> > 
> > 
> > > 
> > > B) Making tracepoint_probe_register() just return an error (-ENOEDV), but
> > >    leaving the probe registered.
> > > 
> > > I think we have already agreed that this compromise solution is a weird API
> > > choice, since an error condition must be treated as a success by the
> > > caller.
> > > Moreover, it has the same issue as (A) above: if the module is unloaded
> > > after probe registration, the tracer is lying to the user, making them
> > > think there is a call site loaded when there are none.
> > 
> > But isn't that what we do today?
> 
> No, currently tracepoint offers no API to query the state of tracepoint callsites
> other than the seqfile iterator (which I proposed to removed in a RFC patch
> yesterday).

To me that's a work around, not a real fix.

> 
> > 
> > > 
> > > C) Adding module load arguments to specify which tracepoint should be
> > >    enabled.
> > > 
> > > Please refer to my response above in this email to understand why I object.
> > 
> > I didn't suggest this in this thread. I only pointed out that is the
> > most sane situation. But I've been trying to work with you, and you
> > don't seem to see that.
> 
> I appreciate the compromise efforts you have made, but my points here are about
> sound design/API, and I have technical concerns still unanswered (see other leg
> of the thread). This is not about what LTTng needs, but about keeping the
> tracepoint API consistent.

Yep, see my responses.


> I don't mind doing the heavy lifting in LTTng at all. My concern here
> is (again) about keeping the tracepoint API semantically consistent when
> module load/unload occur. A lot of thinking went into this when I initially
> created tracepoint.c, and I don't want it to be degraded by a change that is
> not thought thoroughly.

I have a good idea of what needs to be done. I just need you to look at
things with a different perspective than what you are use to.

-- Steve

[for-next][PATCH 09/20] tracing: Fix event header writeback.h to include tracepoint.h

[Steven Rostedt]

[-- Attachment #1: 0009-tracing-Fix-event-header-writeback.h-to-include-trac.patch --]
[-- Type: text/plain, Size: 966 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The trace event headers are required to include tracepoint.h. The only reason
they worked now is because module.h included tracepoint.h, and that will soon
change.

Link: http://lkml.kernel.org/r/20140226190644.442886305@goodmis.org

Fixes: 455b2864686d "writeback: Initial tracing support"
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Jens Axboe <jaxboe@fusionio.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/trace/events/writeback.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index c7bbbe7..309a086 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -4,6 +4,7 @@
 #if !defined(_TRACE_WRITEBACK_H) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_WRITEBACK_H
 
+#include <linux/tracepoint.h>
 #include <linux/backing-dev.h>
 #include <linux/writeback.h>
 
-- 
1.8.5.3

[for-next][PATCH 10/20] tracing: Fix event header migrate.h to include tracepoint.h

[Steven Rostedt]

[-- Attachment #1: 0010-tracing-Fix-event-header-migrate.h-to-include-tracep.patch --]
[-- Type: text/plain, Size: 986 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The trace event headers are required to include tracepoint.h. The only reason
they worked now is because module.h included tracepoint.h, and that will soon
change.

Link: http://lkml.kernel.org/r/20140226190644.591040764@goodmis.org

Fixes: 7b2a2d4a18ff "mm: migrate: Add a tracepoint for migrate_pages"
Acked-by: Mel Gorman <mgorman@suse.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/trace/events/migrate.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/trace/events/migrate.h b/include/trace/events/migrate.h
index 3075ffb..4e4f2f8 100644
--- a/include/trace/events/migrate.h
+++ b/include/trace/events/migrate.h
@@ -4,6 +4,8 @@
 #if !defined(_TRACE_MIGRATE_H) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_MIGRATE_H
 
+#include <linux/tracepoint.h>
+
 #define MIGRATE_MODE						\
 	{MIGRATE_ASYNC,		"MIGRATE_ASYNC"},		\
 	{MIGRATE_SYNC_LIGHT,	"MIGRATE_SYNC_LIGHT"},		\
-- 
1.8.5.3

[for-next][PATCH 11/20] tracing/module: Replace include of tracepoint.h with jump_label.h in module.h

[Steven Rostedt]

[-- Attachment #1: 0011-tracing-module-Replace-include-of-tracepoint.h-with-.patch --]
[-- Type: text/plain, Size: 1027 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

There's nothing in the module.h header that requires tracepoint.h to be
included, and there may be cases that tracepoint.h may need to include
module.h, which will cause recursive header issues.

But module.h requires seeing HAVE_JUMP_LABEL which is set in jump_label.h
which it just coincidentally gets from tracepoint.h.

Link: http://lkml.kernel.org/r/20140307084712.5c68641a@gandalf.local.home

Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/module.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index eaf60ff..5a50539 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -15,7 +15,7 @@
 #include <linux/stringify.h>
 #include <linux/kobject.h>
 #include <linux/moduleparam.h>
-#include <linux/tracepoint.h>
+#include <linux/jump_label.h>
 #include <linux/export.h>
 
 #include <linux/percpu.h>
-- 
1.8.5.3

[for-next][PATCH 12/20] tracing: Correctly expand len expressions from __dynamic_array macro

[Steven Rostedt]

[-- Attachment #1: 0012-tracing-Correctly-expand-len-expressions-from-__dyna.patch --]
[-- Type: text/plain, Size: 1322 bytes --]

From: Filipe Brandenburger <filbranden@google.com>

This fixes expansion of the len argument in __dynamic_array macros.
The previous code from commit 7d536cb3f would not fully evaluate the
expression before multiplying its result by the size of the type.

This went unnoticed because the length stored in the high 16 bits of the
offset (which is the one that was broken here) is only used by
filter_pred_strloc which only acts on strings for which the size of the
type is 1.

Link: http://lkml.kernel.org/r/1393651938-16418-2-git-send-email-filbranden@google.com

Signed-off-by: Filipe Brandenburger <filbranden@google.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/trace/ftrace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 1cc2265..2125005 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -363,7 +363,7 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call)	\
 #define __dynamic_array(type, item, len)				\
 	__data_offsets->item = __data_size +				\
 			       offsetof(typeof(*entry), __data);	\
-	__data_offsets->item |= (len * sizeof(type)) << 16;		\
+	__data_offsets->item |= ((len) * sizeof(type)) << 16;		\
 	__data_size += (len) * sizeof(type);
 
 #undef __string
-- 
1.8.5.3

[for-next][PATCH 13/20] tracing: Evaluate len expression only once in __dynamic_array macro

[Steven Rostedt]

[-- Attachment #1: 0013-tracing-Evaluate-len-expression-only-once-in-__dynam.patch --]
[-- Type: text/plain, Size: 1567 bytes --]

From: Filipe Brandenburger <filbranden@google.com>

Use a temporary variable to store the expansion of the len expression.
If the evaluation is expensive, this commit will ensure it is evaluated
only once inside ftrace_get_offsets_<call>.

Link: http://lkml.kernel.org/r/1393651938-16418-3-git-send-email-filbranden@google.com

Signed-off-by: Filipe Brandenburger <filbranden@google.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/trace/ftrace.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 2125005..e15ae401 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -361,10 +361,11 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call)	\
 
 #undef __dynamic_array
 #define __dynamic_array(type, item, len)				\
+	__item_length = (len) * sizeof(type);				\
 	__data_offsets->item = __data_size +				\
 			       offsetof(typeof(*entry), __data);	\
-	__data_offsets->item |= ((len) * sizeof(type)) << 16;		\
-	__data_size += (len) * sizeof(type);
+	__data_offsets->item |= __item_length << 16;			\
+	__data_size += __item_length;
 
 #undef __string
 #define __string(item, src) __dynamic_array(char, item,			\
@@ -376,6 +377,7 @@ static inline notrace int ftrace_get_offsets_##call(			\
 	struct ftrace_data_offsets_##call *__data_offsets, proto)       \
 {									\
 	int __data_size = 0;						\
+	int __maybe_unused __item_length;				\
 	struct ftrace_raw_##call __maybe_unused *entry;			\
 									\
 	tstruct;							\
-- 
1.8.5.3

[for-next][PATCH 14/20] ftrace: Cleanup of global variables ftrace_new_pgs and ftrace_update_cnt

[Steven Rostedt]

[-- Attachment #1: 0014-ftrace-Cleanup-of-global-variables-ftrace_new_pgs-an.patch --]
[-- Type: text/plain, Size: 3482 bytes --]

From: Jiri Slaby <jslaby@suse.cz>

Some of them can be local to functions, so make them local and pass
them as parameters where needed:
* __start_mcount_loc+__stop_mcount_loc are local to ftrace_init
* ftrace_new_pgs -> new_pgs/start_pg
* ftrace_update_cnt -> local update_cnt in ftrace_update_code

Link: http://lkml.kernel.org/r/1393268401-24379-1-git-send-email-jslaby@suse.cz

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 5313c11..3f95bbe 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1174,8 +1174,6 @@ struct ftrace_page {
 	int			size;
 };
 
-static struct ftrace_page *ftrace_new_pgs;
-
 #define ENTRY_SIZE sizeof(struct dyn_ftrace)
 #define ENTRIES_PER_PAGE (PAGE_SIZE / ENTRY_SIZE)
 
@@ -2246,7 +2244,6 @@ static void ftrace_shutdown_sysctl(void)
 }
 
 static cycle_t		ftrace_update_time;
-static unsigned long	ftrace_update_cnt;
 unsigned long		ftrace_update_tot_cnt;
 
 static inline int ops_traces_mod(struct ftrace_ops *ops)
@@ -2302,11 +2299,12 @@ static int referenced_filters(struct dyn_ftrace *rec)
 	return cnt;
 }
 
-static int ftrace_update_code(struct module *mod)
+static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
 {
 	struct ftrace_page *pg;
 	struct dyn_ftrace *p;
 	cycle_t start, stop;
+	unsigned long update_cnt = 0;
 	unsigned long ref = 0;
 	bool test = false;
 	int i;
@@ -2332,9 +2330,8 @@ static int ftrace_update_code(struct module *mod)
 	}
 
 	start = ftrace_now(raw_smp_processor_id());
-	ftrace_update_cnt = 0;
 
-	for (pg = ftrace_new_pgs; pg; pg = pg->next) {
+	for (pg = new_pgs; pg; pg = pg->next) {
 
 		for (i = 0; i < pg->index; i++) {
 			int cnt = ref;
@@ -2355,7 +2352,7 @@ static int ftrace_update_code(struct module *mod)
 			if (!ftrace_code_disable(mod, p))
 				break;
 
-			ftrace_update_cnt++;
+			update_cnt++;
 
 			/*
 			 * If the tracing is enabled, go ahead and enable the record.
@@ -2374,11 +2371,9 @@ static int ftrace_update_code(struct module *mod)
 		}
 	}
 
-	ftrace_new_pgs = NULL;
-
 	stop = ftrace_now(raw_smp_processor_id());
 	ftrace_update_time = stop - start;
-	ftrace_update_tot_cnt += ftrace_update_cnt;
+	ftrace_update_tot_cnt += update_cnt;
 
 	return 0;
 }
@@ -4270,9 +4265,6 @@ static int ftrace_process_locs(struct module *mod,
 	/* Assign the last page to ftrace_pages */
 	ftrace_pages = pg;
 
-	/* These new locations need to be initialized */
-	ftrace_new_pgs = start_pg;
-
 	/*
 	 * We only need to disable interrupts on start up
 	 * because we are modifying code that an interrupt
@@ -4283,7 +4275,7 @@ static int ftrace_process_locs(struct module *mod,
 	 */
 	if (!mod)
 		local_irq_save(flags);
-	ftrace_update_code(mod);
+	ftrace_update_code(mod, start_pg);
 	if (!mod)
 		local_irq_restore(flags);
 	ret = 0;
@@ -4392,11 +4384,10 @@ struct notifier_block ftrace_module_exit_nb = {
 	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
 };
 
-extern unsigned long __start_mcount_loc[];
-extern unsigned long __stop_mcount_loc[];
-
 void __init ftrace_init(void)
 {
+	extern unsigned long __start_mcount_loc[];
+	extern unsigned long __stop_mcount_loc[];
 	unsigned long count, addr, flags;
 	int ret;
 
-- 
1.8.5.3

[for-next][PATCH 15/20] ftrace: Inline the code from ftrace_dyn_table_alloc()

[Steven Rostedt]

[-- Attachment #1: 0015-ftrace-Inline-the-code-from-ftrace_dyn_table_alloc.patch --]
[-- Type: text/plain, Size: 1771 bytes --]

From: Jiri Slaby <jslaby@suse.cz>

The function used to do allocations some time ago. This no longer
happens and it only checks the count and prints some info. This patch
inlines the body to the only caller. There are two reasons:
* the name of the function was misleading
* it's clear what is going on in ftrace_init now

Link: http://lkml.kernel.org/r/1393268401-24379-2-git-send-email-jslaby@suse.cz

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 3f95bbe..76b6ed2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2465,22 +2465,6 @@ ftrace_allocate_pages(unsigned long num_to_init)
 	return NULL;
 }
 
-static int __init ftrace_dyn_table_alloc(unsigned long num_to_init)
-{
-	int cnt;
-
-	if (!num_to_init) {
-		pr_info("ftrace: No functions to be traced?\n");
-		return -1;
-	}
-
-	cnt = num_to_init / ENTRIES_PER_PAGE;
-	pr_info("ftrace: allocating %ld entries in %d pages\n",
-		num_to_init, cnt + 1);
-
-	return 0;
-}
-
 #define FTRACE_BUFF_MAX (KSYM_SYMBOL_LEN+4) /* room for wildcards */
 
 struct ftrace_iterator {
@@ -4403,10 +4387,13 @@ void __init ftrace_init(void)
 		goto failed;
 
 	count = __stop_mcount_loc - __start_mcount_loc;
-
-	ret = ftrace_dyn_table_alloc(count);
-	if (ret)
+	if (!count) {
+		pr_info("ftrace: No functions to be traced?\n");
 		goto failed;
+	}
+
+	pr_info("ftrace: allocating %ld entries in %ld pages\n",
+		count, count / ENTRIES_PER_PAGE + 1);
 
 	last_ftrace_enabled = ftrace_enabled = 1;
 
-- 
1.8.5.3

[for-next][PATCH 16/20] ftrace: Pass retval through return in ftrace_dyn_arch_init()

[Steven Rostedt]

[-- Attachment #1: 0016-ftrace-Pass-retval-through-return-in-ftrace_dyn_arch.patch --]
[-- Type: text/plain, Size: 6916 bytes --]

From: Jiri Slaby <jslaby@suse.cz>

No architecture uses the "data" parameter in ftrace_dyn_arch_init() in any
way, it just sets the value to 0. And this is used as a return value
in the caller -- ftrace_init, which just checks the retval against
zero.

Note there is also "return 0" in every ftrace_dyn_arch_init.  So it is
enough to check the retval and remove all the indirect sets of data on
all archs.

Link: http://lkml.kernel.org/r/1393268401-24379-3-git-send-email-jslaby@suse.cz

Cc: linux-arch@vger.kernel.org
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 Documentation/trace/ftrace-design.txt | 3 ---
 arch/arm/kernel/ftrace.c              | 2 --
 arch/blackfin/kernel/ftrace.c         | 3 ---
 arch/ia64/kernel/ftrace.c             | 2 --
 arch/metag/kernel/ftrace.c            | 3 ---
 arch/microblaze/kernel/ftrace.c       | 3 ---
 arch/mips/kernel/ftrace.c             | 3 ---
 arch/powerpc/kernel/ftrace.c          | 5 -----
 arch/s390/kernel/ftrace.c             | 1 -
 arch/sh/kernel/ftrace.c               | 3 ---
 arch/sparc/kernel/ftrace.c            | 4 ----
 arch/tile/kernel/ftrace.c             | 2 --
 arch/x86/kernel/ftrace.c              | 3 ---
 kernel/trace/ftrace.c                 | 6 ++----
 14 files changed, 2 insertions(+), 41 deletions(-)

diff --git a/Documentation/trace/ftrace-design.txt b/Documentation/trace/ftrace-design.txt
index 79fcafc..1171688 100644
--- a/Documentation/trace/ftrace-design.txt
+++ b/Documentation/trace/ftrace-design.txt
@@ -360,9 +360,6 @@ function below should be sufficient for most people:
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	/* return value is done indirectly via data */
-	*(unsigned long *)data = 0;
-
 	return 0;
 }
 
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 34e5664..5cd0d05 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -158,8 +158,6 @@ int ftrace_make_nop(struct module *mod,
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	*(unsigned long *)data = 0;
-
 	return 0;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
diff --git a/arch/blackfin/kernel/ftrace.c b/arch/blackfin/kernel/ftrace.c
index 9277905..f74c5ae 100644
--- a/arch/blackfin/kernel/ftrace.c
+++ b/arch/blackfin/kernel/ftrace.c
@@ -67,9 +67,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	/* return value is done indirectly via data */
-	*(unsigned long *)data = 0;
-
 	return 0;
 }
 
diff --git a/arch/ia64/kernel/ftrace.c b/arch/ia64/kernel/ftrace.c
index 7fc8c96..cfaa93a 100644
--- a/arch/ia64/kernel/ftrace.c
+++ b/arch/ia64/kernel/ftrace.c
@@ -200,7 +200,5 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 /* run from kstop_machine */
 int __init ftrace_dyn_arch_init(void *data)
 {
-	*(unsigned long *)data = 0;
-
 	return 0;
 }
diff --git a/arch/metag/kernel/ftrace.c b/arch/metag/kernel/ftrace.c
index a774f32..bf59393 100644
--- a/arch/metag/kernel/ftrace.c
+++ b/arch/metag/kernel/ftrace.c
@@ -119,8 +119,5 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 /* run from kstop_machine */
 int __init ftrace_dyn_arch_init(void *data)
 {
-	/* The return code is returned via data */
-	writel(0, data);
-
 	return 0;
 }
diff --git a/arch/microblaze/kernel/ftrace.c b/arch/microblaze/kernel/ftrace.c
index e8a5e9c..ffa595c 100644
--- a/arch/microblaze/kernel/ftrace.c
+++ b/arch/microblaze/kernel/ftrace.c
@@ -173,9 +173,6 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	/* The return code is retured via data */
-	*(unsigned long *)data = 0;
-
 	return 0;
 }
 
diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 185ba25..013016b 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -206,9 +206,6 @@ int __init ftrace_dyn_arch_init(void *data)
 	/* Remove "b ftrace_stub" to ensure ftrace_caller() is executed */
 	ftrace_modify_code(MCOUNT_ADDR, INSN_NOP);
 
-	/* The return code is retured via data */
-	*(unsigned long *)data = 0;
-
 	return 0;
 }
 #endif	/* CONFIG_DYNAMIC_FTRACE */
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 9b27b29..d059664 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -533,11 +533,6 @@ void arch_ftrace_update_code(int command)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	/* caller expects data to be zero */
-	unsigned long *p = data;
-
-	*p = 0;
-
 	return 0;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
index 224db03..77b2f3a 100644
--- a/arch/s390/kernel/ftrace.c
+++ b/arch/s390/kernel/ftrace.c
@@ -132,7 +132,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	*(unsigned long *) data = 0;
 	return 0;
 }
 
diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
index 30e1319..4939975 100644
--- a/arch/sh/kernel/ftrace.c
+++ b/arch/sh/kernel/ftrace.c
@@ -274,9 +274,6 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	/* The return code is retured via data */
-	__raw_writel(0, (unsigned long)data);
-
 	return 0;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
index 03ab022..ee813b8 100644
--- a/arch/sparc/kernel/ftrace.c
+++ b/arch/sparc/kernel/ftrace.c
@@ -84,10 +84,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	unsigned long *p = data;
-
-	*p = 0;
-
 	return 0;
 }
 #endif
diff --git a/arch/tile/kernel/ftrace.c b/arch/tile/kernel/ftrace.c
index f1c4520..34d9ea0 100644
--- a/arch/tile/kernel/ftrace.c
+++ b/arch/tile/kernel/ftrace.c
@@ -169,8 +169,6 @@ int ftrace_make_nop(struct module *mod,
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	*(unsigned long *)data = 0;
-
 	return 0;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 8cabf63..bbe5a5b8 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -670,9 +670,6 @@ void arch_ftrace_update_code(int command)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	/* The return code is retured via data */
-	*(unsigned long *)data = 0;
-
 	return 0;
 }
 #endif
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 76b6ed2..083c6d5 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4379,11 +4379,9 @@ void __init ftrace_init(void)
 	addr = (unsigned long)ftrace_stub;
 
 	local_irq_save(flags);
-	ftrace_dyn_arch_init(&addr);
+	ret = ftrace_dyn_arch_init(&addr);
 	local_irq_restore(flags);
-
-	/* ftrace_dyn_arch_init places the return code in addr */
-	if (addr)
+	if (ret)
 		goto failed;
 
 	count = __stop_mcount_loc - __start_mcount_loc;
-- 
1.8.5.3

[for-next][PATCH 17/20] ftrace: Do not pass data to ftrace_dyn_arch_init

[Steven Rostedt]

[-- Attachment #1: 0017-ftrace-Do-not-pass-data-to-ftrace_dyn_arch_init.patch --]
[-- Type: text/plain, Size: 7606 bytes --]

From: Jiri Slaby <jslaby@suse.cz>

As the data parameter is not really used by any ftrace_dyn_arch_init,
remove that from ftrace_dyn_arch_init. This also removes the addr
local variable from ftrace_init which is now unused.

Note the documentation was imprecise as it did not suggest to set
(*data) to 0.

Link: http://lkml.kernel.org/r/1393268401-24379-4-git-send-email-jslaby@suse.cz

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-arch@vger.kernel.org
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 Documentation/trace/ftrace-design.txt | 2 +-
 arch/arm/kernel/ftrace.c              | 2 +-
 arch/blackfin/kernel/ftrace.c         | 2 +-
 arch/ia64/kernel/ftrace.c             | 2 +-
 arch/metag/kernel/ftrace.c            | 2 +-
 arch/microblaze/kernel/ftrace.c       | 2 +-
 arch/mips/kernel/ftrace.c             | 2 +-
 arch/powerpc/kernel/ftrace.c          | 2 +-
 arch/s390/kernel/ftrace.c             | 2 +-
 arch/sh/kernel/ftrace.c               | 2 +-
 arch/sparc/kernel/ftrace.c            | 2 +-
 arch/tile/kernel/ftrace.c             | 2 +-
 arch/x86/kernel/ftrace.c              | 2 +-
 include/linux/ftrace.h                | 2 +-
 kernel/trace/ftrace.c                 | 7 ++-----
 15 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/Documentation/trace/ftrace-design.txt b/Documentation/trace/ftrace-design.txt
index 1171688..3f669b9 100644
--- a/Documentation/trace/ftrace-design.txt
+++ b/Documentation/trace/ftrace-design.txt
@@ -358,7 +358,7 @@ Every arch has an init callback function.  If you need to do something early on
 to initialize some state, this is the time to do that.  Otherwise, this simple
 function below should be sufficient for most people:
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 5cd0d05..c108ddc 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -156,7 +156,7 @@ int ftrace_make_nop(struct module *mod,
 	return ret;
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/blackfin/kernel/ftrace.c b/arch/blackfin/kernel/ftrace.c
index f74c5ae..095de0f 100644
--- a/arch/blackfin/kernel/ftrace.c
+++ b/arch/blackfin/kernel/ftrace.c
@@ -65,7 +65,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ftrace_modify_code(ip, call, sizeof(call));
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/ia64/kernel/ftrace.c b/arch/ia64/kernel/ftrace.c
index cfaa93a..3b0c2aa 100644
--- a/arch/ia64/kernel/ftrace.c
+++ b/arch/ia64/kernel/ftrace.c
@@ -198,7 +198,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 }
 
 /* run from kstop_machine */
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/metag/kernel/ftrace.c b/arch/metag/kernel/ftrace.c
index bf59393..ed1d685 100644
--- a/arch/metag/kernel/ftrace.c
+++ b/arch/metag/kernel/ftrace.c
@@ -117,7 +117,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 }
 
 /* run from kstop_machine */
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/microblaze/kernel/ftrace.c b/arch/microblaze/kernel/ftrace.c
index ffa595c..bbcd253 100644
--- a/arch/microblaze/kernel/ftrace.c
+++ b/arch/microblaze/kernel/ftrace.c
@@ -171,7 +171,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	return ret;
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 013016b..1ba7afe 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -198,7 +198,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ftrace_modify_code(FTRACE_CALL_IP, new);
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	/* Encode the instructions when booting */
 	ftrace_dyn_arch_init_insns();
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index d059664..71ce4cb 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -531,7 +531,7 @@ void arch_ftrace_update_code(int command)
 		ftrace_disable_ftrace_graph_caller();
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
index 77b2f3a..54d6493 100644
--- a/arch/s390/kernel/ftrace.c
+++ b/arch/s390/kernel/ftrace.c
@@ -130,7 +130,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return 0;
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
index 4939975..3c74f53 100644
--- a/arch/sh/kernel/ftrace.c
+++ b/arch/sh/kernel/ftrace.c
@@ -272,7 +272,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	return ftrace_modify_code(rec->ip, old, new);
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
index ee813b8..0a2d2dd 100644
--- a/arch/sparc/kernel/ftrace.c
+++ b/arch/sparc/kernel/ftrace.c
@@ -82,7 +82,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ftrace_modify_code(ip, old, new);
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/tile/kernel/ftrace.c b/arch/tile/kernel/ftrace.c
index 34d9ea0..8d52d83 100644
--- a/arch/tile/kernel/ftrace.c
+++ b/arch/tile/kernel/ftrace.c
@@ -167,7 +167,7 @@ int ftrace_make_nop(struct module *mod,
 	return ret;
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index bbe5a5b8..4b66adf 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -668,7 +668,7 @@ void arch_ftrace_update_code(int command)
 	atomic_dec(&modifying_ftrace_code);
 }
 
-int __init ftrace_dyn_arch_init(void *data)
+int __init ftrace_dyn_arch_init(void)
 {
 	return 0;
 }
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index e6141be..1bbb2cd 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -423,7 +423,7 @@ ftrace_set_early_filter(struct ftrace_ops *ops, char *buf, int enable);
 
 /* defined in arch */
 extern int ftrace_ip_converted(unsigned long ip);
-extern int ftrace_dyn_arch_init(void *data);
+extern int ftrace_dyn_arch_init(void);
 extern void ftrace_replace_code(int enable);
 extern int ftrace_update_ftrace_func(ftrace_func_t func);
 extern void ftrace_caller(void);
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 083c6d5..5bd70e8 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4372,14 +4372,11 @@ void __init ftrace_init(void)
 {
 	extern unsigned long __start_mcount_loc[];
 	extern unsigned long __stop_mcount_loc[];
-	unsigned long count, addr, flags;
+	unsigned long count, flags;
 	int ret;
 
-	/* Keep the ftrace pointer to the stub */
-	addr = (unsigned long)ftrace_stub;
-
 	local_irq_save(flags);
-	ret = ftrace_dyn_arch_init(&addr);
+	ret = ftrace_dyn_arch_init();
 	local_irq_restore(flags);
 	if (ret)
 		goto failed;
-- 
1.8.5.3

[for-next][PATCH 18/20] ftrace: Remove freelist from struct dyn_ftrace

[Steven Rostedt]

[-- Attachment #1: 0018-ftrace-Remove-freelist-from-struct-dyn_ftrace.patch --]
[-- Type: text/plain, Size: 1250 bytes --]

From: Jiri Slaby <jslaby@suse.cz>

The 'freelist' member was introduced to 'struct dyn_ftrace' in commit
ee000b7f9fe429d2470c674ccec8d344f6789e0d (tracing: use union for
multi-usages field), but the use of this member was later removed in
3208230983a0ee3d95be22d463257e530c684956 (ftrace: Remove usage of
"freed" records). Remove also the 'freelist' member now.

Link: http://lkml.kernel.org/r/1393268401-24379-5-git-send-email-jslaby@suse.cz

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1bbb2cd..20da0561 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -330,12 +330,9 @@ enum {
 #define FTRACE_REF_MAX		((1UL << 29) - 1)
 
 struct dyn_ftrace {
-	union {
-		unsigned long		ip; /* address of mcount call-site */
-		struct dyn_ftrace	*freelist;
-	};
+	unsigned long		ip; /* address of mcount call-site */
 	unsigned long		flags;
-	struct dyn_arch_ftrace		arch;
+	struct dyn_arch_ftrace	arch;
 };
 
 int ftrace_force_update(void);
-- 
1.8.5.3

[for-next][PATCH 19/20] ftrace: Warn on error when modifying ftrace function

[Steven Rostedt]

[-- Attachment #1: 0019-ftrace-Warn-on-error-when-modifying-ftrace-function.patch --]
[-- Type: text/plain, Size: 2202 bytes --]

From: Petr Mladek <pmladek@suse.cz>

We should print some warning and kill ftrace functionality when the ftrace
function is not set correctly. Otherwise, ftrace might do crazy things without
an explanation. The error value has been ignored so far.

Note that an error that happens during updating all the traced calls is handled
in ftrace_replace_code(). We print more details about the particular
failing address via ftrace_bug() there.

Link: http://lkml.kernel.org/r/1393258342-29978-3-git-send-email-pmladek@suse.cz

Signed-off-by: Petr Mladek <pmladek@suse.cz>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 5bd70e8..0e48ff4 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1994,6 +1994,7 @@ int __weak ftrace_arch_code_modify_post_process(void)
 void ftrace_modify_all_code(int command)
 {
 	int update = command & FTRACE_UPDATE_TRACE_FUNC;
+	int err = 0;
 
 	/*
 	 * If the ftrace_caller calls a ftrace_ops func directly,
@@ -2005,8 +2006,11 @@ void ftrace_modify_all_code(int command)
 	 * to make sure the ops are having the right functions
 	 * traced.
 	 */
-	if (update)
-		ftrace_update_ftrace_func(ftrace_ops_list_func);
+	if (update) {
+		err = ftrace_update_ftrace_func(ftrace_ops_list_func);
+		if (FTRACE_WARN_ON(err))
+			return;
+	}
 
 	if (command & FTRACE_UPDATE_CALLS)
 		ftrace_replace_code(1);
@@ -2019,13 +2023,16 @@ void ftrace_modify_all_code(int command)
 		/* If irqs are disabled, we are in stop machine */
 		if (!irqs_disabled())
 			smp_call_function(ftrace_sync_ipi, NULL, 1);
-		ftrace_update_ftrace_func(ftrace_trace_function);
+		err = ftrace_update_ftrace_func(ftrace_trace_function);
+		if (FTRACE_WARN_ON(err))
+			return;
 	}
 
 	if (command & FTRACE_START_FUNC_RET)
-		ftrace_enable_ftrace_graph_caller();
+		err = ftrace_enable_ftrace_graph_caller();
 	else if (command & FTRACE_STOP_FUNC_RET)
-		ftrace_disable_ftrace_graph_caller();
+		err = ftrace_disable_ftrace_graph_caller();
+	FTRACE_WARN_ON(err);
 }
 
 static int __ftrace_modify_code(void *data)
-- 
1.8.5.3

[for-next][PATCH 20/20] ftrace/x86: BUG when ftrace recovery fails

[Steven Rostedt]

[-- Attachment #1: 0020-ftrace-x86-BUG-when-ftrace-recovery-fails.patch --]
[-- Type: text/plain, Size: 2293 bytes --]

From: Petr Mladek <pmladek@suse.cz>

Ftrace modifies function calls using Int3 breakpoints on x86.
The breakpoints are handled only when the patching is in progress.
If something goes wrong, there is a recovery code that removes
the breakpoints. If this fails, the system might get silently
rebooted when a remaining break is not handled or an invalid
instruction is proceed.

We should BUG() when the breakpoint could not be removed. Otherwise,
the system silently crashes when the function finishes the Int3
handler is disabled.

Note that we need to modify remove_breakpoint() to return non-zero
value only when there is an error. The return value was ignored before,
so it does not cause any troubles.

Link: http://lkml.kernel.org/r/1393258342-29978-4-git-send-email-pmladek@suse.cz

Signed-off-by: Petr Mladek <pmladek@suse.cz>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 4b66adf..52819e8 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -425,7 +425,7 @@ static int remove_breakpoint(struct dyn_ftrace *rec)
 
 	/* If this does not have a breakpoint, we are done */
 	if (ins[0] != brk)
-		return -1;
+		return 0;
 
 	nop = ftrace_nop_replace();
 
@@ -625,7 +625,12 @@ void ftrace_replace_code(int enable)
 	printk(KERN_WARNING "Failed on %s (%d):\n", report, count);
 	for_ftrace_rec_iter(iter) {
 		rec = ftrace_rec_iter_record(iter);
-		remove_breakpoint(rec);
+		/*
+		 * Breakpoints are handled only when this function is in
+		 * progress. The system could not work with them.
+		 */
+		if (remove_breakpoint(rec))
+			BUG();
 	}
 	run_sync();
 }
@@ -649,12 +654,19 @@ ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 	run_sync();
 
 	ret = ftrace_write(ip, new_code, 1);
+	/*
+	 * The breakpoint is handled only when this function is in progress.
+	 * The system could not work if we could not remove it.
+	 */
+	BUG_ON(ret);
  out:
 	run_sync();
 	return ret;
 
  fail_update:
-	ftrace_write(ip, old_code, 1);
+	/* Also here the system could not work with the breakpoint */
+	if (ftrace_write(ip, old_code, 1))
+		BUG();
 	goto out;
 }
 
-- 
1.8.5.3