[PATCH 1/3] ftrace: add proper bin iterator support

[PATCH 1/3] ftrace: add proper bin iterator support

[Aneesh Kumar K.V]

In s_show after print_trace_line we call trace_print_seq which
does a strlen on the buffer containing binary data. I guess that would
give wrong results for bin data even though we are null terminating it.
We also don't want header in the output file if we have  TRACE_ITER_BIN.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 kernel/trace/trace.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8a499e2..fb32a5c 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1833,10 +1833,51 @@ static enum print_line_t print_trace_line(struct trace_iterator *iter)
 	return print_trace_fmt(iter);
 }
 
+static int s_bin_show(struct seq_file *m, void *v)
+{
+	int len;
+	enum print_line_t ret;
+	struct trace_iterator *iter = v;
+
+	if (iter->ent == NULL)
+		return 0;
+
+	if (iter->trace && iter->trace->print_line) {
+		ret = iter->trace->print_line(iter);
+		if (ret != TRACE_TYPE_UNHANDLED)
+			goto trace_handled;
+	}
+	/*
+	 * If  trace type is unhandled or if we don't have
+	 * a print_line call the standard bin_fmt callback
+	 */
+	print_bin_fmt(iter);
+
+trace_handled:
+	/* copy the trace buffer to seq file buffer */
+	if (iter->seq.len >= PAGE_SIZE)
+		len = PAGE_SIZE - 1;
+	else
+		len = iter->seq.len;
+
+	if (m->count + len < m->size) {
+		memcpy(m->buf + m->count, iter->seq.buffer, len);
+		m->count += len;
+	} else
+		m->count = m->size;
+	trace_seq_reset(&iter->seq);
+
+	return 0;
+}
+
+
 static int s_show(struct seq_file *m, void *v)
 {
 	struct trace_iterator *iter = v;
 
+	if (trace_flags & TRACE_ITER_BIN)
+		return s_bin_show(m, v);
+
 	if (iter->ent == NULL) {
 		if (iter->tr) {
 			seq_printf(m, "# tracer: %s\n", iter->trace->name);
-- 
1.6.0.4.608.ga9645

[PATCH 2/3] ftrace: Add debug_print trace to print data from kernel to userspace

[Aneesh Kumar K.V]

The trace add a new interface debug_print() which can be used
to dump data from kernel to user space using ftrace framework.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 include/linux/debug_print.h     |   12 ++++
 kernel/trace/Kconfig            |    7 ++
 kernel/trace/Makefile           |    1 +
 kernel/trace/trace_debugprint.c |  134 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/debug_print.h
 create mode 100644 kernel/trace/trace_debugprint.c

diff --git a/include/linux/debug_print.h b/include/linux/debug_print.h
new file mode 100644
index 0000000..db4397b
--- /dev/null
+++ b/include/linux/debug_print.h
@@ -0,0 +1,12 @@
+#ifndef _LINUX_DEBUGPRINT_H
+#define _LINUX_DEBUGPRINT_H
+#include <linux/marker.h>
+
+#ifdef CONFIG_DEBUG_PRINT
+extern int do_dp_printk(const char *fmt, ...);
+#define dp_printk(fmt, arg...) \
+	do_dp_printk(fmt, ##arg)
+#else
+#define dp_printk(fmt, arg...)
+#endif
+#endif /*  _LINUX_DEBUGPRINT_H */
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index b58f43b..9b832e0 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -195,4 +195,11 @@ config FTRACE_STARTUP_TEST
 	  functioning properly. It will do tests on all the configured
 	  tracers of ftrace.
 
+config DEBUG_PRINT
+	bool "Trace based printk support"
+	depends on DEBUG_KERNEL
+	select TRACING
+	help
+	  This tracer can be used for printk style debugging.
+
 endmenu
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index c8228b1..afbf059 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -24,5 +24,6 @@ obj-$(CONFIG_NOP_TRACER) += trace_nop.o
 obj-$(CONFIG_STACK_TRACER) += trace_stack.o
 obj-$(CONFIG_MMIOTRACE) += trace_mmiotrace.o
 obj-$(CONFIG_BOOT_TRACER) += trace_boot.o
+obj-$(CONFIG_DEBUG_PRINT) += trace_debugprint.o
 
 libftrace-y := ftrace.o
diff --git a/kernel/trace/trace_debugprint.c b/kernel/trace/trace_debugprint.c
new file mode 100644
index 0000000..feb38d8
--- /dev/null
+++ b/kernel/trace/trace_debugprint.c
@@ -0,0 +1,134 @@
+/*
+ * Copyright IBM Corporation, 2008
+ * Author Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2.1 of the GNU Lesser General Public License
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/debugfs.h>
+#include <linux/ftrace.h>
+
+#include "trace.h"
+
+static struct trace_array	*ctx_trace;
+static int __read_mostly	tracer_enabled;
+
+int do_dp_printk(const char *fmt, ...)
+{
+	int ret;
+	va_list args;
+
+	if (!tracer_enabled)
+		return 0;
+
+	va_start(args, fmt);
+	ret = trace_vprintk(0, fmt, args);
+	va_end(args);
+	return ret;
+}
+EXPORT_SYMBOL(do_dp_printk);
+
+static void dp_reset(struct trace_array *tr)
+{
+	int cpu;
+
+	tr->time_start = ftrace_now(tr->cpu);
+
+	for_each_online_cpu(cpu)
+		tracing_reset(tr, cpu);
+}
+
+static void start_dp_trace(struct trace_array *tr)
+{
+	dp_reset(tr);
+	tracer_enabled = 1;
+}
+
+static void stop_dp_trace(struct trace_array *tr)
+{
+	tracer_enabled = 0;
+	/* Nothing to do! */
+}
+
+static void dp_trace_init(struct trace_array *tr)
+{
+	ctx_trace = tr;
+
+	if (tr->ctrl)
+		start_dp_trace(tr);
+}
+
+static void dp_trace_reset(struct trace_array *tr)
+{
+	if (tr->ctrl)
+		stop_dp_trace(tr);
+}
+
+static void dp_trace_ctrl_update(struct trace_array *tr)
+{
+	/* When starting a new trace, reset the buffers */
+	if (tr->ctrl)
+		start_dp_trace(tr);
+	else
+		stop_dp_trace(tr);
+}
+
+#ifdef CONFIG_NOP_TRACER
+int
+trace_selftest_startup_dp(struct tracer *trace, struct trace_array *tr)
+{
+	/* What could possibly go wrong? */
+	return 0;
+}
+#endif
+
+static enum print_line_t debug_print_line(struct trace_iterator *iter)
+{
+	struct trace_seq *s = &iter->seq;
+	struct trace_entry *entry;
+
+	entry = iter->ent;
+	switch (entry->type) {
+	case TRACE_PRINT: {
+		struct print_entry *field;
+		trace_assign_type(field, entry);
+
+		trace_seq_printf(s, "%s", field->buf);
+		if (entry->flags & TRACE_FLAG_CONT)
+			trace_seq_print_cont(s, iter);
+		break;
+	}
+	default:
+		printk(KERN_INFO "Unsupported type in debug_print\n");
+		return TRACE_TYPE_UNHANDLED;
+	}
+
+	return TRACE_TYPE_HANDLED;
+}
+
+struct tracer dp_trace __read_mostly =
+{
+	.name		= "debug_print",
+	.init		= dp_trace_init,
+	.reset		= dp_trace_reset,
+	.ctrl_update	= dp_trace_ctrl_update,
+#ifdef CONFIG_FTRACE_SELFTEST
+	.selftest	= trace_selftest_startup_dp,
+#endif
+	.print_line	= debug_print_line,
+};
+
+__init static int init_dp_trace(void)
+{
+	return register_tracer(&dp_trace);
+}
+device_initcall(init_dp_trace);
-- 
1.6.0.4.608.ga9645

[PATCH 3/3] ftrace: Add debug_dump trace to dump binary data from kernel to userspace

[Aneesh Kumar K.V]

The trace add a new interface debug_dump() which can be used
to dump binary data using ftrace framework. Use the bin mode
of iterator by 'echo bin > iter_ctrl' to read the value in
userspace

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 include/linux/debug_dump.h     |   10 +++
 kernel/trace/Kconfig           |    6 ++
 kernel/trace/Makefile          |    1 +
 kernel/trace/trace.c           |    4 +
 kernel/trace/trace.h           |    3 +
 kernel/trace/trace_debugdump.c |  172 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 196 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/debug_dump.h
 create mode 100644 kernel/trace/trace_debugdump.c

diff --git a/include/linux/debug_dump.h b/include/linux/debug_dump.h
new file mode 100644
index 0000000..2c1836d
--- /dev/null
+++ b/include/linux/debug_dump.h
@@ -0,0 +1,10 @@
+#ifndef _LINUX_DEBUGDUMP_H
+#define _LINUX_DEBUGDUMP_H
+#include <linux/marker.h>
+
+#ifdef CONFIG_DEBUG_DUMP
+extern int debug_dump(void *p, int len);
+#else
+#define debug_dump(p, len)
+#endif
+#endif /*  _LINUX_DEBUGDUMP_H */
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 9b832e0..ddde659 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -202,4 +202,10 @@ config DEBUG_PRINT
 	help
 	  This tracer can be used for printk style debugging.
 
+config DEBUG_DUMP
+	bool "Trace based dump support"
+	depends on DEBUG_KERNEL
+	select TRACING
+	help
+	  This tracer can be used to dump binary data from kernel
 endmenu
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index afbf059..afd1be7 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -25,5 +25,6 @@ obj-$(CONFIG_STACK_TRACER) += trace_stack.o
 obj-$(CONFIG_MMIOTRACE) += trace_mmiotrace.o
 obj-$(CONFIG_BOOT_TRACER) += trace_boot.o
 obj-$(CONFIG_DEBUG_PRINT) += trace_debugprint.o
+obj-$(CONFIG_DEBUG_DUMP) += trace_debugdump.o
 
 libftrace-y := ftrace.o
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index fb32a5c..b62eebe 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -156,6 +156,10 @@ static DECLARE_WAIT_QUEUE_HEAD(trace_wait);
 /* trace_flags holds iter_ctrl options */
 unsigned long trace_flags = TRACE_ITER_PRINT_PARENT;
 
+struct trace_array *trace_get_global_trace(void)
+{
+	return &global_trace;
+}
 /**
  * trace_wake_up - wake up tasks waiting for trace input
  *
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 8465ad0..345bea2 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -22,6 +22,7 @@ enum trace_type {
 	TRACE_MMIO_RW,
 	TRACE_MMIO_MAP,
 	TRACE_BOOT,
+	TRACE_BIN_DUMP,
 
 	__TRACE_LAST_TYPE
 };
@@ -219,6 +220,7 @@ extern void __ftrace_bad_type(void);
 		IF_ASSIGN(var, ent, struct trace_mmiotrace_map,		\
 			  TRACE_MMIO_MAP);				\
 		IF_ASSIGN(var, ent, struct trace_boot, TRACE_BOOT);	\
+		IF_ASSIGN(var, ent, struct dump_entry, TRACE_BIN_DUMP);	\
 		__ftrace_bad_type();					\
 	} while (0)
 
@@ -393,6 +395,7 @@ extern ssize_t trace_seq_to_user(struct trace_seq *s, char __user *ubuf,
 				 size_t cnt);
 extern long ns2usecs(cycle_t nsec);
 extern int trace_vprintk(unsigned long ip, const char *fmt, va_list args);
+extern struct trace_array *trace_get_global_trace(void);
 
 extern unsigned long trace_flags;
 
diff --git a/kernel/trace/trace_debugdump.c b/kernel/trace/trace_debugdump.c
new file mode 100644
index 0000000..8c7c56f
--- /dev/null
+++ b/kernel/trace/trace_debugdump.c
@@ -0,0 +1,172 @@
+/*
+ * Copyright IBM Corporation, 2008
+ * Author Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2.1 of the GNU Lesser General Public License
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/debugfs.h>
+#include <linux/ftrace.h>
+
+#include "trace.h"
+
+struct dump_entry {
+	struct trace_entry ent;
+	int	len;
+	char	buf[];
+};
+
+static struct trace_array	*ctx_trace;
+static int __read_mostly	tracer_enabled;
+
+int debug_dump(void *bin_data, int len)
+{
+	struct ring_buffer_event *event;
+	struct trace_array *tr;
+	struct trace_array_cpu *data;
+	struct dump_entry *entry;
+	unsigned long flags, irq_flags;
+	int cpu, size, pc;
+
+	if (!tracer_enabled)
+		return 0;
+	tr = trace_get_global_trace();
+	pc = preempt_count();
+	preempt_disable_notrace();
+	cpu = raw_smp_processor_id();
+	data = tr->data[cpu];
+
+	local_irq_save(flags);
+	size = sizeof(*entry) + len;
+	event = ring_buffer_lock_reserve(tr->buffer, size, &irq_flags);
+	if (!event)
+		goto out;
+	entry = ring_buffer_event_data(event);
+	tracing_generic_entry_update(&entry->ent, flags, pc);
+	entry->ent.type	= TRACE_BIN_DUMP;
+	entry->len	= len;
+
+	memcpy(&entry->buf, bin_data, len);
+	ring_buffer_unlock_commit(tr->buffer, event, irq_flags);
+
+out:
+	local_irq_restore(flags);
+	preempt_enable_notrace();
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(debug_dump);
+
+static void dd_reset(struct trace_array *tr)
+{
+	int cpu;
+
+	tr->time_start = ftrace_now(tr->cpu);
+
+	for_each_online_cpu(cpu)
+		tracing_reset(tr, cpu);
+}
+
+static void start_dd_trace(struct trace_array *tr)
+{
+	dd_reset(tr);
+	tracer_enabled = 1;
+}
+
+static void stop_dd_trace(struct trace_array *tr)
+{
+	tracer_enabled = 0;
+	/* Nothing to do! */
+}
+
+static void dd_trace_init(struct trace_array *tr)
+{
+	ctx_trace = tr;
+
+	if (tr->ctrl)
+		start_dd_trace(tr);
+}
+
+static void dd_trace_reset(struct trace_array *tr)
+{
+	if (tr->ctrl)
+		stop_dd_trace(tr);
+}
+
+static void dd_trace_ctrl_update(struct trace_array *tr)
+{
+	/* When starting a new trace, reset the buffers */
+	if (tr->ctrl)
+		start_dd_trace(tr);
+	else
+		stop_dd_trace(tr);
+}
+
+#ifdef CONFIG_NOP_TRACER
+int
+trace_selftest_startup_dd(struct tracer *trace, struct trace_array *tr)
+{
+	/* What could possibly go wrong? */
+	return 0;
+}
+#endif
+
+static int
+trace_seq_putmem(struct trace_seq *s, void *mem, size_t len)
+{
+	if (len > ((PAGE_SIZE - 1) - s->len))
+		return 0;
+
+	memcpy(s->buffer + s->len, mem, len);
+	s->len += len;
+
+	return len;
+}
+
+static enum print_line_t debug_dump_entry(struct trace_iterator *iter)
+{
+	struct trace_seq *s = &iter->seq;
+	struct trace_entry *entry;
+
+	entry = iter->ent;
+	switch (entry->type) {
+	case TRACE_BIN_DUMP: {
+		struct dump_entry *field;
+		trace_assign_type(field, entry);
+		trace_seq_putmem(s, field->buf, field->len);
+		break;
+	}
+	default:
+		printk(KERN_INFO "Unsupported type in debug_dump\n");
+		return TRACE_TYPE_UNHANDLED;
+	}
+
+	return TRACE_TYPE_HANDLED;
+}
+
+struct tracer dd_trace __read_mostly =
+{
+	.name		= "debug_dump",
+	.init		= dd_trace_init,
+	.reset		= dd_trace_reset,
+	.ctrl_update	= dd_trace_ctrl_update,
+#ifdef CONFIG_FTRACE_SELFTEST
+	.selftest	= trace_selftest_startup_dd,
+#endif
+	.print_line	= debug_dump_entry,
+};
+
+__init static int init_dd_trace(void)
+{
+	return register_tracer(&dd_trace);
+}
+device_initcall(init_dd_trace);
-- 
1.6.0.4.608.ga9645

Re: [PATCH 3/3] ftrace: Add debug_dump trace to dump binary data from kernel to userspace

[Frédéric Weisbecker]

2008/11/14 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
> +struct trace_array *trace_get_global_trace(void)
> +{
> +       return &global_trace;
> +}


I'm not sure this is necessary to export the global_trace.
It is transmitted through init callback of a tracer (struct trace_array *tr).
You have a ctx_trace that you make pointing to global_trace using tr.
And after that you never use it.
That would be better to use your ctx_trace instead of exporting such function.

> +int debug_dump(void *bin_data, int len)
> +{
> +       struct ring_buffer_event *event;
> +       struct trace_array *tr;
> +       struct trace_array_cpu *data;
> +       struct dump_entry *entry;
> +       unsigned long flags, irq_flags;
> +       int cpu, size, pc;
> +
> +       if (!tracer_enabled)
> +               return 0;
> +       tr = trace_get_global_trace();
> +       pc = preempt_count();
> +       preempt_disable_notrace();
> +       cpu = raw_smp_processor_id();
> +       data = tr->data[cpu];
> +
> +       local_irq_save(flags);
> +       size = sizeof(*entry) + len;
> +       event = ring_buffer_lock_reserve(tr->buffer, size, &irq_flags);
> +       if (!event)
> +               goto out;
> +       entry = ring_buffer_event_data(event);
> +       tracing_generic_entry_update(&entry->ent, flags, pc);
> +       entry->ent.type = TRACE_BIN_DUMP;
> +       entry->len      = len;
> +
> +       memcpy(&entry->buf, bin_data, len);
> +       ring_buffer_unlock_commit(tr->buffer, event, irq_flags);
> +
> +out:
> +       local_irq_restore(flags);
> +       preempt_enable_notrace();
> +
> +       return len;
> +}
> +EXPORT_SYMBOL_GPL(debug_dump);


I don't think this is necessary here to protect against preemption or
interrupts.
IE: it is necessary for function tracing to avoid recursion. But for
other usual cases
I think this it is not needed. If you thought about protecting the
ring buffer, it is already
able to protect itself :-)


> +static void dd_trace_ctrl_update(struct trace_array *tr)
> +{
> +       /* When starting a new trace, reset the buffers */
> +       if (tr->ctrl)
> +               start_dd_trace(tr);
> +       else
> +               stop_dd_trace(tr);
> +}


As I said, this callback recently disappeared.


> +
> +#ifdef CONFIG_NOP_TRACER
> +int
> +trace_selftest_startup_dd(struct tracer *trace, struct trace_array *tr)
> +{
> +       /* What could possibly go wrong? */
> +       return 0;
> +}
> +#endif


NOP_TRACER...copy-pasting is not always our friend... :-)


> +static enum print_line_t debug_dump_entry(struct trace_iterator *iter)
> +{
> +       struct trace_seq *s = &iter->seq;
> +       struct trace_entry *entry;
> +
> +       entry = iter->ent;
> +       switch (entry->type) {
> +       case TRACE_BIN_DUMP: {
> +               struct dump_entry *field;
> +               trace_assign_type(field, entry);
> +               trace_seq_putmem(s, field->buf, field->len);
> +               break;


Don't forget the TRACE_TYPE_PRINT_LINE...

Re: [PATCH 3/3] ftrace: Add debug_dump trace to dump binary data from kernel to userspace

[Frédéric Weisbecker]

2008/11/14 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
> +static int
> +trace_seq_putmem(struct trace_seq *s, void *mem, size_t len)
> +{
> +       if (len > ((PAGE_SIZE - 1) - s->len))
> +               return 0;
> +
> +       memcpy(s->buffer + s->len, mem, len);
> +       s->len += len;
> +
> +       return len;
> +}



I forgot to say that I like the idea of this tracer. That would be
useful to dump some random datas from memory.
But I would find it much more useful if I could choose whether the
output is raw bytes or an hexadecimal drawing of the dump,
pretty much like hexdump does. That's better to know where we are in
the dump, and to have a directly human readable dump.

Currently this is not yet possible to choose whether we want a kind of
output or one other. Or perhaps by listen to the iter flags.
I will probably send a patch to make a tracer able to support custom
flags through the old-named iter_ctrl file.

Re: [PATCH 3/3] ftrace: Add debug_dump trace to dump binary data from kernel to userspace

[Frédéric Weisbecker]

2008/11/14 Frédéric Weisbecker <fweisbec@gmail.com>:
> 2008/11/14 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
>> +static int
>> +trace_seq_putmem(struct trace_seq *s, void *mem, size_t len)
>> +{
>> +       if (len > ((PAGE_SIZE - 1) - s->len))
>> +               return 0;
>> +
>> +       memcpy(s->buffer + s->len, mem, len);
>> +       s->len += len;
>> +
>> +       return len;
>> +}
>
>
>
> I forgot to say that I like the idea of this tracer. That would be
> useful to dump some random datas from memory.
> But I would find it much more useful if I could choose whether the
> output is raw bytes or an hexadecimal drawing of the dump,
> pretty much like hexdump does. That's better to know where we are in
> the dump, and to have a directly human readable dump.
>
> Currently this is not yet possible to choose whether we want a kind of
> output or one other. Or perhaps by listen to the iter flags.
> I will probably send a patch to make a tracer able to support custom
> flags through the old-named iter_ctrl file.


Actually yes, what you could do is verifying whether the current
trace_flags is TRACE_ITER_HEX or
TRACE_ITER_BIN and format your output depending on which one is set....

Re: [PATCH 2/3] ftrace: Add debug_print trace to print data from kernel to userspace

[Frédéric Weisbecker]

Hi Aneesh!

2008/11/14 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
> The trace add a new interface debug_print() which can be used
> to dump data from kernel to user space using ftrace framework.


The actual "nop tracer" which is the default selected (if trace_boot
is not configured) let
the tracing engine able to receive and handle TRACE_PRINT events.
Even if nop tracer doesn't handle its output itself, the TRACE_PRINT
event output is relayed by
print_trace_fmt() in trace.c

Your output does almost the same but it is already implemented.



> +static void dp_trace_ctrl_update(struct trace_array *tr)
> +{
> +       /* When starting a new trace, reset the buffers */
> +       if (tr->ctrl)
> +               start_dp_trace(tr);
> +       else
> +               stop_dp_trace(tr);
> +}


BTW, ctrl_update() have been removed very recently.
Perhaps are you implementing this against the mainline? Its a better idea to
submit a new tracer against latest -tip tree.

> +
> +#ifdef CONFIG_NOP_TRACER
> +int
> +trace_selftest_startup_dp(struct tracer *trace, struct trace_array *tr)
> +{
> +       /* What could possibly go wrong? */
> +       return 0;
> +}
> +#endif


CONFIG_NOP_TRACER ?
Wouldn't it rather depend on CONFIG_FTRACE_SELFTEST? :-)

That would perhaps have been better to raise a ftrace_printk to make a test.
If you don't do anything in your selftest, then it is unnecessary to
implement one for
your tracer.


> +static enum print_line_t debug_print_line(struct trace_iterator *iter)
> +{
> +       struct trace_seq *s = &iter->seq;
> +       struct trace_entry *entry;
> +
> +       entry = iter->ent;
> +       switch (entry->type) {
> +       case TRACE_PRINT: {
> +               struct print_entry *field;
> +               trace_assign_type(field, entry);
> +
> +               trace_seq_printf(s, "%s", field->buf);
> +               if (entry->flags & TRACE_FLAG_CONT)
> +                       trace_seq_print_cont(s, iter);
> +               break;


You should test if trace_seq_printf successed to print the whole line.
If not, that's better to return TRACE_TYPE_PARTIAL_LINE, then the
output will be retried later.



> +       }
> +       default:
> +               printk(KERN_INFO "Unsupported type in debug_print\n");
> +               return TRACE_TYPE_UNHANDLED;
> +       }
> +
> +       return TRACE_TYPE_HANDLED;
> +}
> +
> +struct tracer dp_trace __read_mostly =
> +{
> +       .name           = "debug_print",
> +       .init           = dp_trace_init,
> +       .reset          = dp_trace_reset,
> +       .ctrl_update    = dp_trace_ctrl_update,
> +#ifdef CONFIG_FTRACE_SELFTEST
> +       .selftest       = trace_selftest_startup_dp,
> +#endif
> +       .print_line     = debug_print_line,
> +};
> +
> +__init static int init_dp_trace(void)
> +{
> +       return register_tracer(&dp_trace);
> +}
> +device_initcall(init_dp_trace);


Primarily, such a debug tracer is not a bad idea, IMHO.
And note that all of I just wrote in this answer in only my opinion.
Perhaps other people
would find other uses of this tracer that actual default output of the
tracing engine doesn't handle well, or trace.c is
would not be the right place for further enhancements that could
happen on debug entries if you need to.

But the actual simple output that you are submitting along this tracer
is already handled by the default output of the tracing internals.

Re: [PATCH 2/3] ftrace: Add debug_print trace to print data from kernel to userspace

[Aneesh Kumar K.V]

On Fri, Nov 14, 2008 at 12:21:20PM +0100, Frédéric Weisbecker wrote:
> Hi Aneesh!
> 
> 2008/11/14 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
> > The trace add a new interface debug_print() which can be used
> > to dump data from kernel to user space using ftrace framework.
> 
> 
> The actual "nop tracer" which is the default selected (if trace_boot
> is not configured) let
> the tracing engine able to receive and handle TRACE_PRINT events.
> Even if nop tracer doesn't handle its output itself, the TRACE_PRINT
> event output is relayed by
> print_trace_fmt() in trace.c
> 
> Your output does almost the same but it is already implemented.

We also want to make sure dp_printk doesn't do anything when tracer
is disabled. We do

int do_dp_printk(const char *fmt, ...)
{
	int ret;
	va_list args;

	if (!tracer_enabled)
		return 0;

.........
.......

> 
> 
> 
> > +static void dp_trace_ctrl_update(struct trace_array *tr)
> > +{
> > +       /* When starting a new trace, reset the buffers */
> > +       if (tr->ctrl)
> > +               start_dp_trace(tr);
> > +       else
> > +               stop_dp_trace(tr);
> > +}
> 
> 
> BTW, ctrl_update() have been removed very recently.
> Perhaps are you implementing this against the mainline? Its a better idea to
> submit a new tracer against latest -tip tree.

Yes the patches are against mainline.

> 
> > +
> > +#ifdef CONFIG_NOP_TRACER
> > +int
> > +trace_selftest_startup_dp(struct tracer *trace, struct trace_array *tr)
> > +{
> > +       /* What could possibly go wrong? */
> > +       return 0;
> > +}
> > +#endif
> 
> 
> CONFIG_NOP_TRACER ?
> Wouldn't it rather depend on CONFIG_FTRACE_SELFTEST? :-)
> 
> That would perhaps have been better to raise a ftrace_printk to make a test.
> If you don't do anything in your selftest, then it is unnecessary to
> implement one for
> your tracer.

I dropped the selftest callback.

> 
> 
> > +static enum print_line_t debug_print_line(struct trace_iterator *iter)
> > +{
> > +       struct trace_seq *s = &iter->seq;
> > +       struct trace_entry *entry;
> > +
> > +       entry = iter->ent;
> > +       switch (entry->type) {
> > +       case TRACE_PRINT: {
> > +               struct print_entry *field;
> > +               trace_assign_type(field, entry);
> > +
> > +               trace_seq_printf(s, "%s", field->buf);
> > +               if (entry->flags & TRACE_FLAG_CONT)
> > +                       trace_seq_print_cont(s, iter);
> > +               break;
> 
> 
> You should test if trace_seq_printf successed to print the whole line.
> If not, that's better to return TRACE_TYPE_PARTIAL_LINE, then the
> output will be retried later.

I am looking at this. I don't see TRACE_TYPE_PARTIAL_LINE in other
places in the code. Will look more.

> 
> 
> 
> > +       }
> > +       default:
> > +               printk(KERN_INFO "Unsupported type in debug_print\n");
> > +               return TRACE_TYPE_UNHANDLED;
> > +       }
> > +
> > +       return TRACE_TYPE_HANDLED;
> > +}
> > +
> > +struct tracer dp_trace __read_mostly =
> > +{
> > +       .name           = "debug_print",
> > +       .init           = dp_trace_init,
> > +       .reset          = dp_trace_reset,
> > +       .ctrl_update    = dp_trace_ctrl_update,
> > +#ifdef CONFIG_FTRACE_SELFTEST
> > +       .selftest       = trace_selftest_startup_dp,
> > +#endif
> > +       .print_line     = debug_print_line,
> > +};
> > +
> > +__init static int init_dp_trace(void)
> > +{
> > +       return register_tracer(&dp_trace);
> > +}
> > +device_initcall(init_dp_trace);
> 
> 
> Primarily, such a debug tracer is not a bad idea, IMHO.
> And note that all of I just wrote in this answer in only my opinion.
> Perhaps other people
> would find other uses of this tracer that actual default output of the
> tracing engine doesn't handle well, or trace.c is
> would not be the right place for further enhancements that could
> happen on debug entries if you need to.
> 
> But the actual simple output that you are submitting along this tracer
> is already handled by the default output of the tracing internals.

What I wanted to get was a dmesg style output. The default output will
add pid and other information. That is why i did a print_line callback.
I also wanted to drop the header in the trace file.  I didn't find a way
to do that.

-aneesh

Re: [PATCH 2/3] ftrace: Add debug_print trace to print data from kernel to userspace

[Frédéric Weisbecker]

2008/11/14 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
> We also want to make sure dp_printk doesn't do anything when tracer
> is disabled. We do
>
> int do_dp_printk(const char *fmt, ...)
> {
>        int ret;
>        va_list args;
>
>        if (!tracer_enabled)
>                return 0;


In this case you just have to do
echo 0 > /debug/tracing/tracing_enabled
And the entries will not be printed anymore....

> .........
> .......
>
>>
>>
>>
>> > +static void dp_trace_ctrl_update(struct trace_array *tr)
>> > +{
>> > +       /* When starting a new trace, reset the buffers */
>> > +       if (tr->ctrl)
>> > +               start_dp_trace(tr);
>> > +       else
>> > +               stop_dp_trace(tr);
>> > +}
>>
>>
>> BTW, ctrl_update() have been removed very recently.
>> Perhaps are you implementing this against the mainline? Its a better idea to
>> submit a new tracer against latest -tip tree.
>
> Yes the patches are against mainline.


> I am looking at this. I don't see TRACE_TYPE_PARTIAL_LINE in other
> places in the code. Will look more.


This return has been set because several entries can be submitted to a
tracer before it finally reach the user.
If the last one submitted didn't fir is the remaining free bytes in
the seq, then we want to tell the tracing api that
the seq is full and we want this entry to be handled later.
If you don't use it, you could loose entries.
Have a look in trace.c, trace_boot.c, trace_mmiotrace.c, all of these
make use of this return value.

See latest -tip:
git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-tip.git


> What I wanted to get was a dmesg style output. The default output will
> add pid and other information. That is why i did a print_line callback.


I understand...
Why not append a flag to ignore pid/cpu/timestamp on the output in trace.c ?

> I also wanted to drop the header in the trace file.  I didn't find a way
> to do that.


This one should be dropped too when the flag I mentioned above is set....
Currently, when one cat /debug/tracing/trace, we have these headers,
whatever the current tracer is.
This thing could be fixed...

Perhaps others would have better ideas....

Re: [PATCH 2/3] ftrace: Add debug_print trace to print data from kernel to userspace

[Aneesh Kumar K.V]

On Fri, Nov 14, 2008 at 03:39:29PM +0100, Frédéric Weisbecker wrote:
> 2008/11/14 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
> > We also want to make sure dp_printk doesn't do anything when tracer
> > is disabled. We do
> >
> > int do_dp_printk(const char *fmt, ...)
> > {
> >        int ret;
> >        va_list args;
> >
> >        if (!tracer_enabled)
> >                return 0;
> 
> 
> In this case you just have to do
> echo 0 > /debug/tracing/tracing_enabled
> And the entries will not be printed anymore....

That would prevent us to have both dp_printk and debug_dump
used at the same time. Both will them depend on the tracer_enabled
flag. That means the output of these call will also interact with
rest of the tracer output.

-aneesh

Re: [PATCH 2/3] ftrace: Add debug_print trace to print data from kernel to userspace

[Frédéric Weisbecker]

I forgot one thing:

2008/11/14 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
> +static struct trace_array      *ctx_trace;


It's unnecessary, you are not using it :-)