[PATCH V4 0/3] Integration of function trace with System Trace IP blocks

[PATCH V4 0/3] Integration of function trace with System Trace IP blocks

[Chunyan Zhang]

IP blocks allowing a variety of trace sources to log debugging
information to a pre-defined area have been introduced on a couple of
architecture [1][2]. These system trace blocks (also known as STM)
typically follow the MIPI STPv2 protocol [3] and provide a system wide
logging facility to any device, running a kernel or not, with access
to the block's log entry port(s).  Since each trace message has a
timestamp, it is possible to correlate events happening in the entire
system rather than being confined to the logging facility of a single
entity.

This patchset is trying to use STM IP blocks to store some function
tracing information produced by Ftrace and I'm taking the Function trace
(TRACE_FN) as the example in this patchset, but other types of traces
also can be supported.

Logging information generated by the function trace subsystem
and gathered in the coresight sink can be used in conjunction with
trace data from other board components, also collected in the same
trace sink.  This example is using ARM coresight STM but the same would
apply to any architecture wishing to do the same.

Comments and advice would be greatly appreciated.

Thanks,
Chunyan

[1]. https://lwn.net/Articles/674746/
[2]. http://lxr.free-electrons.com/source/drivers/hwtracing/intel_th/
[3]. http://mipi.org/specifications/debug#STP

Changes from v3:
* Addressed comments from Steven Rostedt:
  - Added comments for the element 'name' and 'next' of trace_export;
  - Changed to use RCU functions instead of mutex in function callback;
  - Moved the check for name to register trace_export function;
* Renamed 'trace_function_exports' to 'trace_exports' since its
  implementation is not only for function_trace;  The similar changes on
  'add_trace_export' and 'rm_trace_export'.

Changes from v2:
* Rebased on v4.8-rc1.
* Added trace_export class, and make traces can be exported to not only
ring buffer but also other area such as STM.
* Made stm_ftrace as an trace_export.
* More detailed changes are described in change log of each patch.

Changes from v1:
* Addressed comments from Alexander Shishkin:
 - Modified some ambiguous change logs.
 - Decoupled stm_ftrace and trace_output interface to STM.
 - Changed the file name from stm_ftrace.c to stm/ftrace.c.
 - Implemented link/unlink hooks for stm_ftrace.
* Removed useless header file include from stm/ftrace.c
* Added Acked-by from Steven Rostedt on 4/4.

Chunyan Zhang (3):
  tracing: add a possibility of exporting function trace to other places
    instead of ring buffer only
  stm class: ftrace: Add ftrace-export-over-stm driver
  stm: Mark the functions of writing buffer with notrace

 drivers/hwtracing/coresight/coresight-stm.c |   2 +-
 drivers/hwtracing/intel_th/sth.c            |  11 ++-
 drivers/hwtracing/stm/Kconfig               |  11 +++
 drivers/hwtracing/stm/Makefile              |   2 +
 drivers/hwtracing/stm/core.c                |   7 +-
 drivers/hwtracing/stm/dummy_stm.c           |   2 +-
 drivers/hwtracing/stm/ftrace.c              |  87 +++++++++++++++++++
 include/linux/stm.h                         |   4 +-
 include/linux/trace.h                       |  33 ++++++++
 kernel/trace/trace.c                        | 124 +++++++++++++++++++++++++++-
 kernel/trace/trace.h                        |  31 +++++++
 11 files changed, 302 insertions(+), 12 deletions(-)
 create mode 100644 drivers/hwtracing/stm/ftrace.c
 create mode 100644 include/linux/trace.h

-- 
2.7.4

[PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Chunyan Zhang]

Currently ring buffer is the only one output of Function traces, this patch
added trace_export concept which would process the traces and export
traces to a registered destination which can be ring buffer or some other
storage, in this way if we want Function traces to be sent to other
destination rather than ring buffer only, we just need to register a new
trace_export and implement its own .commit() callback or just use
'trace_generic_commit()' which this patch also added and hooks up its
own .write() functio for writing traces to the storage.

Currently, only Function trace (TRACE_FN) is supported.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 include/linux/trace.h |  33 ++++++++++++++
 kernel/trace/trace.c  | 124 +++++++++++++++++++++++++++++++++++++++++++++++++-
 kernel/trace/trace.h  |  31 +++++++++++++
 3 files changed, 187 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/trace.h

diff --git a/include/linux/trace.h b/include/linux/trace.h
new file mode 100644
index 0000000..4d4f0e1
--- /dev/null
+++ b/include/linux/trace.h
@@ -0,0 +1,33 @@
+#ifndef _LINUX_TRACE_H
+#define _LINUX_TRACE_H
+
+#include <linux/ring_buffer.h>
+struct trace_array;
+
+#ifdef CONFIG_TRACING
+/*
+ * The trace export - an export of function traces.  Every ftrace_ops
+ * has at least one export which would output function traces to ring
+ * buffer.
+ *
+ * name		- the name of this export
+ * next		- pointer to the next trace_export
+ * tr		- the trace_array this export belongs to
+ * commit	- commit the traces to ring buffer and/or some other places
+ * write	- copy traces which have been delt with ->commit() to
+ *		  the destination
+ */
+struct trace_export {
+	char name[16];
+	struct trace_export	*next;
+	struct trace_array	*tr;
+	void (*commit)(struct trace_array *, struct ring_buffer_event *);
+	void (*write)(const char *, unsigned int);
+};
+
+int register_trace_export(struct trace_export *export);
+int unregister_trace_export(struct trace_export *export);
+
+#endif	/* CONFIG_TRACING */
+
+#endif	/* _LINUX_TRACE_H */
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index dade4c9..0247ac2 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -40,6 +40,7 @@
 #include <linux/poll.h>
 #include <linux/nmi.h>
 #include <linux/fs.h>
+#include <linux/trace.h>
 #include <linux/sched/rt.h>
 
 #include "trace.h"
@@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
 	ftrace_trace_userstack(buffer, flags, pc);
 }
 
+static inline void
+trace_generic_commit(struct trace_array *tr,
+	       struct ring_buffer_event *event)
+{
+	struct trace_entry *entry;
+	struct trace_export *export = tr->export;
+	unsigned int size = 0;
+
+	entry = ring_buffer_event_data(event);
+
+	trace_entry_size(size, entry->type);
+	if (!size)
+		return;
+
+	if (export->write)
+		export->write((char *)entry, size);
+}
+
+static inline void
+trace_rb_commit(struct trace_array *tr,
+	       struct ring_buffer_event *event)
+{
+	__buffer_unlock_commit(tr->trace_buffer.buffer, event);
+}
+
+static DEFINE_MUTEX(trace_export_lock);
+
+static struct trace_export trace_export_rb __read_mostly = {
+	.name		= "rb",
+	.commit	= trace_rb_commit,
+	.next		= NULL,
+};
+static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb;
+
+inline void
+trace_exports(struct trace_array *tr, struct ring_buffer_event *event)
+{
+	struct trace_export *export;
+
+	preempt_disable_notrace();
+
+	for (export = rcu_dereference_raw_notrace(trace_exports_list);
+	     export && export->commit;
+	     export = rcu_dereference_raw_notrace(export->next)) {
+		tr->export = export;
+		export->commit(tr, event);
+	}
+
+	preempt_enable_notrace();
+}
+
+static void
+add_trace_export(struct trace_export **list, struct trace_export *export)
+{
+	export->next = *list;
+	/*
+	 * We are entering export into the list but another
+	 * CPU might be walking that list. We need to make sure
+	 * the export->next pointer is valid before another CPU sees
+	 * the export pointer included into the list.
+	 */
+	rcu_assign_pointer(*list, export);
+
+}
+
+static int
+rm_trace_export(struct trace_export **list, struct trace_export *export)
+{
+	struct trace_export **p;
+
+	for (p = list; *p != &trace_export_rb; p = &(*p)->next)
+		if (*p == export)
+			break;
+
+	if (*p != export)
+		return -1;
+
+	*p = (*p)->next;
+
+	return 0;
+}
+
+int register_trace_export(struct trace_export *export)
+{
+	if (!export->write) {
+		pr_warn("trace_export must have the write() call back.\n");
+		return -1;
+	}
+
+	if (!export->name) {
+		pr_warn("trace_export must have a name.\n");
+		return -1;
+	}
+
+	mutex_lock(&trace_export_lock);
+
+	export->tr = trace_exports_list->tr;
+	export->commit = trace_generic_commit;
+
+	add_trace_export(&trace_exports_list, export);
+
+	mutex_unlock(&trace_export_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(register_trace_export);
+
+int unregister_trace_export(struct trace_export *export)
+{
+	int ret;
+
+	mutex_lock(&trace_export_lock);
+
+	ret = rm_trace_export(&trace_exports_list, export);
+
+	mutex_unlock(&trace_export_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(unregister_trace_export);
+
 void
 trace_function(struct trace_array *tr,
 	       unsigned long ip, unsigned long parent_ip, unsigned long flags,
@@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr,
 	entry->parent_ip		= parent_ip;
 
 	if (!call_filter_check_discard(call, entry, buffer, event))
-		__buffer_unlock_commit(buffer, event);
+		trace_exports(tr, event);
 }
 
 #ifdef CONFIG_STACKTRACE
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index f783df4..a40f07c 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -260,6 +260,7 @@ struct trace_array {
 	/* function tracing enabled */
 	int			function_enabled;
 #endif
+	struct trace_export	*export;
 };
 
 enum {
@@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void)
 		break;					\
 	}
 
+#undef IF_SIZE
+#define IF_SIZE(size, var, etype, id)		\
+		if (var == id) {		\
+			size = (sizeof(etype));	\
+			break;			\
+		}
+
 /* Will cause compile errors if type is not found. */
 extern void __ftrace_bad_type(void);
 
@@ -339,6 +347,29 @@ extern void __ftrace_bad_type(void);
 	} while (0)
 
 /*
+ * The trace_entry_size return the size of specific trace type
+ *
+ * IF_SIZE(size, var);
+ *
+ * Where "var" is just the given trace type.
+ */
+#define trace_entry_size(size, var)					\
+	do {								\
+		IF_SIZE(size, var, struct ftrace_entry, TRACE_FN);	\
+		IF_SIZE(size, var, struct stack_entry, TRACE_STACK);	\
+		IF_SIZE(size, var, struct userstack_entry,		\
+			TRACE_USER_STACK);				\
+		IF_SIZE(size, var, struct print_entry, TRACE_PRINT);	\
+		IF_SIZE(size, var, struct bprint_entry, TRACE_BPRINT);	\
+		IF_SIZE(size, var, struct bputs_entry, TRACE_BPUTS);	\
+		IF_SIZE(size, var, struct trace_branch, TRACE_BRANCH);	\
+		IF_SIZE(size, var, struct ftrace_graph_ent_entry,	\
+			TRACE_GRAPH_ENT);				\
+		IF_SIZE(size, var, struct ftrace_graph_ret_entry,	\
+			TRACE_GRAPH_RET);				\
+	} while (0)
+
+/*
  * An option specific to a tracer. This is a boolean value.
  * The bit is the bit index that sets its value on the
  * flags value in struct tracer_flags.
-- 
2.7.4

[PATCH V4 2/3] stm class: ftrace: Add ftrace-export-over-stm driver

[Chunyan Zhang]

This patch adds a driver that models itself as an stm_source and
registers itself as a trace_export.  Once the stm and stm_source
have been linked via sysfs, everything that is passed to the
interface from Ftrace subsystem will endup in the STM trace engine.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/Kconfig  | 11 ++++++
 drivers/hwtracing/stm/Makefile |  2 +
 drivers/hwtracing/stm/ftrace.c | 87 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)
 create mode 100644 drivers/hwtracing/stm/ftrace.c

diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
index 847a39b..b34ea96 100644
--- a/drivers/hwtracing/stm/Kconfig
+++ b/drivers/hwtracing/stm/Kconfig
@@ -39,4 +39,15 @@ config STM_SOURCE_HEARTBEAT
 	  If you want to send heartbeat messages over STM devices,
 	  say Y.
 
+config STM_SOURCE_FTRACE
+	tristate "Copy the output from kernel Ftrace to STM engine"
+	depends on TRACING
+	help
+	  This option can be used to copy the output from kernel Ftrace
+	  to STM engine. Enabling this option will introduce a slight
+	  timing effect.
+
+	  If you want to send kernel Ftrace messages over STM devices,
+	  say Y.
+
 endif
diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
index a9ce3d4..3abd84c 100644
--- a/drivers/hwtracing/stm/Makefile
+++ b/drivers/hwtracing/stm/Makefile
@@ -6,6 +6,8 @@ obj-$(CONFIG_STM_DUMMY)	+= dummy_stm.o
 
 obj-$(CONFIG_STM_SOURCE_CONSOLE)	+= stm_console.o
 obj-$(CONFIG_STM_SOURCE_HEARTBEAT)	+= stm_heartbeat.o
+obj-$(CONFIG_STM_SOURCE_FTRACE)		+= stm_ftrace.o
 
 stm_console-y		:= console.o
 stm_heartbeat-y		:= heartbeat.o
+stm_ftrace-y		:= ftrace.o
diff --git a/drivers/hwtracing/stm/ftrace.c b/drivers/hwtracing/stm/ftrace.c
new file mode 100644
index 0000000..1101f46
--- /dev/null
+++ b/drivers/hwtracing/stm/ftrace.c
@@ -0,0 +1,87 @@
+/*
+ * Simple kernel driver to link kernel Ftrace and an STM device
+ * Copyright (c) 2016, Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/module.h>
+#include <linux/stm.h>
+#include <linux/trace.h>
+
+#define STM_FTRACE_NR_CHANNELS 1
+#define STM_FTRACE_CHAN 0
+
+static int stm_ftrace_link(struct stm_source_data *data);
+static void stm_ftrace_unlink(struct stm_source_data *data);
+
+static struct stm_ftrace {
+	struct stm_source_data	data;
+	struct trace_export	ftrace;
+} stm_ftrace = {
+	.data	= {
+		.name		= "ftrace",
+		.nr_chans	= STM_FTRACE_NR_CHANNELS,
+		.link		= stm_ftrace_link,
+		.unlink		= stm_ftrace_unlink,
+	},
+};
+
+/**
+ * stm_ftrace_write() - write data to STM via 'stm_ftrace' source
+ * @buf:	buffer containing the data packet
+ * @len:	length of the data packet
+ */
+static void notrace
+stm_ftrace_write(const char *buf, unsigned int len)
+{
+	stm_source_write(&stm_ftrace.data, STM_FTRACE_CHAN, buf, len);
+}
+
+static int stm_ftrace_link(struct stm_source_data *data)
+{
+	struct stm_ftrace *sf = container_of(data, struct stm_ftrace, data);
+
+	strcpy(sf->ftrace.name, "stm_ftrace");
+	sf->ftrace.write = stm_ftrace_write;
+	sf->ftrace.next = NULL;
+
+	return register_trace_export(&sf->ftrace);
+}
+
+static void stm_ftrace_unlink(struct stm_source_data *data)
+{
+	struct stm_ftrace *sc = container_of(data, struct stm_ftrace, data);
+
+	unregister_trace_export(&sc->ftrace);
+}
+
+static int __init stm_ftrace_init(void)
+{
+	int ret;
+
+	ret = stm_source_register_device(NULL, &stm_ftrace.data);
+	if (ret)
+		pr_err("Failed to register stm_source - ftrace.\n");
+
+	return ret;
+}
+
+static void __exit stm_ftrace_exit(void)
+{
+	stm_source_unregister_device(&stm_ftrace.data);
+}
+
+module_init(stm_ftrace_init);
+module_exit(stm_ftrace_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("stm_ftrace driver");
+MODULE_AUTHOR("Chunyan Zhang <zhang.chunyan@linaro.org>");
-- 
2.7.4

[PATCH V4 3/3] stm: Mark the functions of writing buffer with notrace

[Chunyan Zhang]

If CONFIG_STM_FTRACE is selected, Function trace data can be writen
to sink via STM, all functions that related to writing data packets
to STM should be marked 'notrace' to avoid being traced by Ftrace,
otherwise the program would stall into an endless loop.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
---
 drivers/hwtracing/coresight/coresight-stm.c |  2 +-
 drivers/hwtracing/intel_th/sth.c            | 11 +++++++----
 drivers/hwtracing/stm/core.c                |  7 ++++---
 drivers/hwtracing/stm/dummy_stm.c           |  2 +-
 include/linux/stm.h                         |  4 ++--
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index 73be58a..345c81e 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -386,7 +386,7 @@ static long stm_generic_set_options(struct stm_data *stm_data,
 	return 0;
 }
 
-static ssize_t stm_generic_packet(struct stm_data *stm_data,
+static ssize_t notrace stm_generic_packet(struct stm_data *stm_data,
 				  unsigned int master,
 				  unsigned int channel,
 				  unsigned int packet,
diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
index e1aee61..b034446 100644
--- a/drivers/hwtracing/intel_th/sth.c
+++ b/drivers/hwtracing/intel_th/sth.c
@@ -67,10 +67,13 @@ static void sth_iowrite(void __iomem *dest, const unsigned char *payload,
 	}
 }
 
-static ssize_t sth_stm_packet(struct stm_data *stm_data, unsigned int master,
-			      unsigned int channel, unsigned int packet,
-			      unsigned int flags, unsigned int size,
-			      const unsigned char *payload)
+static ssize_t notrace sth_stm_packet(struct stm_data *stm_data,
+				      unsigned int master,
+				      unsigned int channel,
+				      unsigned int packet,
+				      unsigned int flags,
+				      unsigned int size,
+				      const unsigned char *payload)
 {
 	struct sth_device *sth = container_of(stm_data, struct sth_device, stm);
 	struct intel_th_channel __iomem *out =
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 51f81d6..37d3bcb 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -425,7 +425,7 @@ static int stm_file_assign(struct stm_file *stmf, char *id, unsigned int width)
 	return ret;
 }
 
-static ssize_t stm_write(struct stm_data *data, unsigned int master,
+static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
 			  unsigned int channel, const char *buf, size_t count)
 {
 	unsigned int flags = STP_PACKET_TIMESTAMPED;
@@ -1121,8 +1121,9 @@ void stm_source_unregister_device(struct stm_source_data *data)
 }
 EXPORT_SYMBOL_GPL(stm_source_unregister_device);
 
-int stm_source_write(struct stm_source_data *data, unsigned int chan,
-		     const char *buf, size_t count)
+int notrace stm_source_write(struct stm_source_data *data,
+			     unsigned int chan,
+			     const char *buf, size_t count)
 {
 	struct stm_source_device *src = data->src;
 	struct stm_device *stm;
diff --git a/drivers/hwtracing/stm/dummy_stm.c b/drivers/hwtracing/stm/dummy_stm.c
index a86612d..c5f94ca 100644
--- a/drivers/hwtracing/stm/dummy_stm.c
+++ b/drivers/hwtracing/stm/dummy_stm.c
@@ -21,7 +21,7 @@
 #include <linux/slab.h>
 #include <linux/stm.h>
 
-static ssize_t
+static ssize_t notrace
 dummy_stm_packet(struct stm_data *stm_data, unsigned int master,
 		 unsigned int channel, unsigned int packet, unsigned int flags,
 		 unsigned int size, const unsigned char *payload)
diff --git a/include/linux/stm.h b/include/linux/stm.h
index 8369d8a..210ff22 100644
--- a/include/linux/stm.h
+++ b/include/linux/stm.h
@@ -133,7 +133,7 @@ int stm_source_register_device(struct device *parent,
 			       struct stm_source_data *data);
 void stm_source_unregister_device(struct stm_source_data *data);
 
-int stm_source_write(struct stm_source_data *data, unsigned int chan,
-		     const char *buf, size_t count);
+int notrace stm_source_write(struct stm_source_data *data, unsigned int chan,
+			     const char *buf, size_t count);
 
 #endif /* _STM_H_ */
-- 
2.7.4

Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[kbuild test robot]

Hi Chunyan,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.8-rc2 next-20160815]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/tracing-add-a-possibility-of-exporting-function-trace-to-other-places-instead-of-ring-buffer-only/20160815-195631
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/compiler.h:230:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> kernel/trace/trace.c:2173:23: sparse: incompatible types in comparison expression (different address spaces)
   kernel/trace/trace.c:2175:23: sparse: incompatible types in comparison expression (different address spaces)

vim +2173 kernel/trace/trace.c

  2157	static DEFINE_MUTEX(trace_export_lock);
  2158	
  2159	static struct trace_export trace_export_rb __read_mostly = {
  2160		.name		= "rb",
  2161		.commit	= trace_rb_commit,
  2162		.next		= NULL,
  2163	};
  2164	static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb;
  2165	
  2166	inline void
  2167	trace_exports(struct trace_array *tr, struct ring_buffer_event *event)
  2168	{
  2169		struct trace_export *export;
  2170	
  2171		preempt_disable_notrace();
  2172	
> 2173		for (export = rcu_dereference_raw_notrace(trace_exports_list);
  2174		     export && export->commit;
  2175		     export = rcu_dereference_raw_notrace(export->next)) {
  2176			tr->export = export;
  2177			export->commit(tr, event);
  2178		}
  2179	
  2180		preempt_enable_notrace();
  2181	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Steven Rostedt]

On Mon, 15 Aug 2016 19:50:01 +0800
Chunyan Zhang <zhang.chunyan@linaro.org> wrote:

> +int register_trace_export(struct trace_export *export);
> +int unregister_trace_export(struct trace_export *export);
> +
> +#endif	/* CONFIG_TRACING */
> +
> +#endif	/* _LINUX_TRACE_H */
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index dade4c9..0247ac2 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -40,6 +40,7 @@
>  #include <linux/poll.h>
>  #include <linux/nmi.h>
>  #include <linux/fs.h>
> +#include <linux/trace.h>
>  #include <linux/sched/rt.h>
>  
>  #include "trace.h"
> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
>  	ftrace_trace_userstack(buffer, flags, pc);
>  }
>  
> +static inline void
> +trace_generic_commit(struct trace_array *tr,
> +	       struct ring_buffer_event *event)

Why is this marked inline? It is never called directly here.

> +{
> +	struct trace_entry *entry;
> +	struct trace_export *export = tr->export;
> +	unsigned int size = 0;
> +
> +	entry = ring_buffer_event_data(event);
> +
> +	trace_entry_size(size, entry->type);
> +	if (!size)
> +		return;
> +
> +	if (export->write)
> +		export->write((char *)entry, size);
> +}
> +
> +static inline void

Same here.

> +trace_rb_commit(struct trace_array *tr,
> +	       struct ring_buffer_event *event)
> +{
> +	__buffer_unlock_commit(tr->trace_buffer.buffer, event);
> +}
> +
> +static DEFINE_MUTEX(trace_export_lock);
> +
> +static struct trace_export trace_export_rb __read_mostly = {
> +	.name		= "rb",
> +	.commit	= trace_rb_commit,

Make sure equal signs are lined up.

> +	.next		= NULL,
> +};
> +static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb;

trace_exports_list needs to be annotated as rcu, if you are going to
dereference it via rcu_dereference. That was the kbuild warning you
received.



> +
> +inline void
> +trace_exports(struct trace_array *tr, struct ring_buffer_event *event)
> +{
> +	struct trace_export *export;
> +
> +	preempt_disable_notrace();
> +
> +	for (export = rcu_dereference_raw_notrace(trace_exports_list);
> +	     export && export->commit;
> +	     export = rcu_dereference_raw_notrace(export->next)) {
> +		tr->export = export;
> +		export->commit(tr, event);
> +	}
> +
> +	preempt_enable_notrace();
> +}

I'm not too happy about the added overhead to normal function tracing
(it will be noticeable), but I can fix that later.

> +
> +static void
> +add_trace_export(struct trace_export **list, struct trace_export *export)
> +{
> +	export->next = *list;

As export->next will most likely need to be marked __rcu as well, this
may need an rcu_assign_pointer() too.

> +	/*
> +	 * We are entering export into the list but another
> +	 * CPU might be walking that list. We need to make sure
> +	 * the export->next pointer is valid before another CPU sees
> +	 * the export pointer included into the list.
> +	 */
> +	rcu_assign_pointer(*list, export);
> +
> +}
> +
> +static int
> +rm_trace_export(struct trace_export **list, struct trace_export *export)
> +{
> +	struct trace_export **p;
> +
> +	for (p = list; *p != &trace_export_rb; p = &(*p)->next)
> +		if (*p == export)
> +			break;
> +
> +	if (*p != export)
> +		return -1;
> +
> +	*p = (*p)->next;

I believe this requires an rcu_assign_pointer() as well.

> +
> +	return 0;
> +}
> +
> +int register_trace_export(struct trace_export *export)
> +{
> +	if (!export->write) {
> +		pr_warn("trace_export must have the write() call back.\n");

Probably should be a "WARN_ON_ONCE()".

> +		return -1;
> +	}
> +
> +	if (!export->name) {
> +		pr_warn("trace_export must have a name.\n");
> +		return -1;
> +	}

If name isn't used don't add it till it is. That is, this patch should
not have "name" in the structure. It's confusing, because I don't see a
purpose for it.

> +
> +	mutex_lock(&trace_export_lock);
> +
> +	export->tr = trace_exports_list->tr;

I don't see where tr is ever assigned.

> +	export->commit = trace_generic_commit;

Shouldn't the caller pass in the commit function too?

> +
> +	add_trace_export(&trace_exports_list, export);
> +
> +	mutex_unlock(&trace_export_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_trace_export);
> +
> +int unregister_trace_export(struct trace_export *export)
> +{
> +	int ret;
> +
> +	mutex_lock(&trace_export_lock);
> +
> +	ret = rm_trace_export(&trace_exports_list, export);
> +
> +	mutex_unlock(&trace_export_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(unregister_trace_export);
> +
>  void
>  trace_function(struct trace_array *tr,
>  	       unsigned long ip, unsigned long parent_ip, unsigned long flags,
> @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr,
>  	entry->parent_ip		= parent_ip;
>  
>  	if (!call_filter_check_discard(call, entry, buffer, event))

How do you handle the discard? If the entry doesn't match the filter,
it will try to discard the event. I don't see how the "trace_exports"
handle that.


> -		__buffer_unlock_commit(buffer, event);
> +		trace_exports(tr, event);
>  }
>  
>  #ifdef CONFIG_STACKTRACE
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index f783df4..a40f07c 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -260,6 +260,7 @@ struct trace_array {
>  	/* function tracing enabled */
>  	int			function_enabled;
>  #endif
> +	struct trace_export	*export;
>  };
>  
>  enum {
> @@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void)
>  		break;					\
>  	}
>  
> +#undef IF_SIZE
> +#define IF_SIZE(size, var, etype, id)		\
> +		if (var == id) {		\
> +			size = (sizeof(etype));	\
> +			break;			\
> +		}
> +
>  /* Will cause compile errors if type is not found. */
>  extern void __ftrace_bad_type(void);
>  
> @@ -339,6 +347,29 @@ extern void __ftrace_bad_type(void);
>  	} while (0)
>  
>  /*
> + * The trace_entry_size return the size of specific trace type
> + *
> + * IF_SIZE(size, var);
> + *
> + * Where "var" is just the given trace type.
> + */
> +#define trace_entry_size(size, var)					\
> +	do {								\
> +		IF_SIZE(size, var, struct ftrace_entry, TRACE_FN);	\
> +		IF_SIZE(size, var, struct stack_entry, TRACE_STACK);	\
> +		IF_SIZE(size, var, struct userstack_entry,		\
> +			TRACE_USER_STACK);				\
> +		IF_SIZE(size, var, struct print_entry, TRACE_PRINT);	\
> +		IF_SIZE(size, var, struct bprint_entry, TRACE_BPRINT);	\
> +		IF_SIZE(size, var, struct bputs_entry, TRACE_BPUTS);	\
> +		IF_SIZE(size, var, struct trace_branch, TRACE_BRANCH);	\
> +		IF_SIZE(size, var, struct ftrace_graph_ent_entry,	\
> +			TRACE_GRAPH_ENT);				\
> +		IF_SIZE(size, var, struct ftrace_graph_ret_entry,	\
> +			TRACE_GRAPH_RET);				\

I really really dislike this. This is a big if statement that all
needs to go down one by one. Very inefficient!

-- Steve

> +	} while (0)
> +
> +/*
>   * An option specific to a tracer. This is a boolean value.
>   * The bit is the bit index that sets its value on the
>   * flags value in struct tracer_flags.

Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Chunyan Zhang]

Hello Steve,

Please correct me if I'm missing something in my following reply.

On 15 August 2016 at 22:28, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 15 Aug 2016 19:50:01 +0800
> Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
>
>> +int register_trace_export(struct trace_export *export);
>> +int unregister_trace_export(struct trace_export *export);
>> +
>> +#endif       /* CONFIG_TRACING */
>> +
>> +#endif       /* _LINUX_TRACE_H */
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index dade4c9..0247ac2 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -40,6 +40,7 @@
>>  #include <linux/poll.h>
>>  #include <linux/nmi.h>
>>  #include <linux/fs.h>
>> +#include <linux/trace.h>
>>  #include <linux/sched/rt.h>
>>
>>  #include "trace.h"
>> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
>>       ftrace_trace_userstack(buffer, flags, pc);
>>  }
>>
>> +static inline void
>> +trace_generic_commit(struct trace_array *tr,
>> +            struct ring_buffer_event *event)
>
> Why is this marked inline? It is never called directly here.
>
>> +{
>> +     struct trace_entry *entry;
>> +     struct trace_export *export = tr->export;
>> +     unsigned int size = 0;
>> +
>> +     entry = ring_buffer_event_data(event);
>> +
>> +     trace_entry_size(size, entry->type);
>> +     if (!size)
>> +             return;
>> +
>> +     if (export->write)
>> +             export->write((char *)entry, size);
>> +}
>> +
>> +static inline void
>
> Same here.
>
>> +trace_rb_commit(struct trace_array *tr,
>> +            struct ring_buffer_event *event)
>> +{
>> +     __buffer_unlock_commit(tr->trace_buffer.buffer, event);
>> +}
>> +
>> +static DEFINE_MUTEX(trace_export_lock);
>> +
>> +static struct trace_export trace_export_rb __read_mostly = {
>> +     .name           = "rb",
>> +     .commit = trace_rb_commit,
>
> Make sure equal signs are lined up.
>
>> +     .next           = NULL,
>> +};
>> +static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb;
>
> trace_exports_list needs to be annotated as rcu, if you are going to
> dereference it via rcu_dereference. That was the kbuild warning you
> received.

Ok, will do.

But I guess 'ftrace_ops_list' in ftrace.c [1] also should be annotated as rcu?

[1] http://lxr.free-electrons.com/source/kernel/trace/ftrace.c#L460

>
>> +
>> +inline void
>> +trace_exports(struct trace_array *tr, struct ring_buffer_event *event)
>> +{
>> +     struct trace_export *export;
>> +
>> +     preempt_disable_notrace();
>> +
>> +     for (export = rcu_dereference_raw_notrace(trace_exports_list);
>> +          export && export->commit;
>> +          export = rcu_dereference_raw_notrace(export->next)) {
>> +             tr->export = export;
>> +             export->commit(tr, event);
>> +     }
>> +
>> +     preempt_enable_notrace();
>> +}
>
> I'm not too happy about the added overhead to normal function tracing
> (it will be noticeable), but I can fix that later.

I think I will try to revise here in the next revision.

>
>> +
>> +static void
>> +add_trace_export(struct trace_export **list, struct trace_export *export)
>> +{
>> +     export->next = *list;
>
> As export->next will most likely need to be marked __rcu as well, this
> may need an rcu_assign_pointer() too.
>
>> +     /*
>> +      * We are entering export into the list but another
>> +      * CPU might be walking that list. We need to make sure
>> +      * the export->next pointer is valid before another CPU sees
>> +      * the export pointer included into the list.
>> +      */
>> +     rcu_assign_pointer(*list, export);
>> +
>> +}
>> +
>> +static int
>> +rm_trace_export(struct trace_export **list, struct trace_export *export)
>> +{
>> +     struct trace_export **p;
>> +
>> +     for (p = list; *p != &trace_export_rb; p = &(*p)->next)
>> +             if (*p == export)
>> +                     break;
>> +
>> +     if (*p != export)
>> +             return -1;
>> +
>> +     *p = (*p)->next;
>
> I believe this requires an rcu_assign_pointer() as well.
>
>> +
>> +     return 0;
>> +}
>> +
>> +int register_trace_export(struct trace_export *export)
>> +{
>> +     if (!export->write) {
>> +             pr_warn("trace_export must have the write() call back.\n");
>
> Probably should be a "WARN_ON_ONCE()".

Yes, will revise.

>
>> +             return -1;
>> +     }
>> +
>> +     if (!export->name) {
>> +             pr_warn("trace_export must have a name.\n");
>> +             return -1;
>> +     }
>
> If name isn't used don't add it till it is. That is, this patch should
> not have "name" in the structure. It's confusing, because I don't see a
> purpose for it.

Ok will remove that.

>
>> +
>> +     mutex_lock(&trace_export_lock);
>> +
>> +     export->tr = trace_exports_list->tr;
>
> I don't see where tr is ever assigned.
>
>> +     export->commit = trace_generic_commit;
>
> Shouldn't the caller pass in the commit function too?

The trace_export::write() callback is for caller, commit function
mainly deal with traces, is it better to keep 'trace_generic_commit'
in trace.c, i.e don't expose it to callers?

>
>> +
>> +     add_trace_export(&trace_exports_list, export);
>> +
>> +     mutex_unlock(&trace_export_lock);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(register_trace_export);
>> +
>> +int unregister_trace_export(struct trace_export *export)
>> +{
>> +     int ret;
>> +
>> +     mutex_lock(&trace_export_lock);
>> +
>> +     ret = rm_trace_export(&trace_exports_list, export);
>> +
>> +     mutex_unlock(&trace_export_lock);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(unregister_trace_export);
>> +
>>  void
>>  trace_function(struct trace_array *tr,
>>              unsigned long ip, unsigned long parent_ip, unsigned long flags,
>> @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr,
>>       entry->parent_ip                = parent_ip;
>>
>>       if (!call_filter_check_discard(call, entry, buffer, event))
>
> How do you handle the discard? If the entry doesn't match the filter,
> it will try to discard the event. I don't see how the "trace_exports"
> handle that.

I directly used the entries which had already been filtered.
Humm.. sorry, actually you lost me here.

>
>
>> -             __buffer_unlock_commit(buffer, event);
>> +             trace_exports(tr, event);
>>  }
>>
>>  #ifdef CONFIG_STACKTRACE
>> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
>> index f783df4..a40f07c 100644
>> --- a/kernel/trace/trace.h
>> +++ b/kernel/trace/trace.h
>> @@ -260,6 +260,7 @@ struct trace_array {
>>       /* function tracing enabled */
>>       int                     function_enabled;
>>  #endif
>> +     struct trace_export     *export;
>>  };
>>
>>  enum {
>> @@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void)
>>               break;                                  \
>>       }
>>
>> +#undef IF_SIZE
>> +#define IF_SIZE(size, var, etype, id)                \
>> +             if (var == id) {                \
>> +                     size = (sizeof(etype)); \
>> +                     break;                  \
>> +             }
>> +
>>  /* Will cause compile errors if type is not found. */
>>  extern void __ftrace_bad_type(void);
>>
>> @@ -339,6 +347,29 @@ extern void __ftrace_bad_type(void);
>>       } while (0)
>>
>>  /*
>> + * The trace_entry_size return the size of specific trace type
>> + *
>> + * IF_SIZE(size, var);
>> + *
>> + * Where "var" is just the given trace type.
>> + */
>> +#define trace_entry_size(size, var)                                  \
>> +     do {                                                            \
>> +             IF_SIZE(size, var, struct ftrace_entry, TRACE_FN);      \
>> +             IF_SIZE(size, var, struct stack_entry, TRACE_STACK);    \
>> +             IF_SIZE(size, var, struct userstack_entry,              \
>> +                     TRACE_USER_STACK);                              \
>> +             IF_SIZE(size, var, struct print_entry, TRACE_PRINT);    \
>> +             IF_SIZE(size, var, struct bprint_entry, TRACE_BPRINT);  \
>> +             IF_SIZE(size, var, struct bputs_entry, TRACE_BPUTS);    \
>> +             IF_SIZE(size, var, struct trace_branch, TRACE_BRANCH);  \
>> +             IF_SIZE(size, var, struct ftrace_graph_ent_entry,       \
>> +                     TRACE_GRAPH_ENT);                               \
>> +             IF_SIZE(size, var, struct ftrace_graph_ret_entry,       \
>> +                     TRACE_GRAPH_RET);                               \
>
> I really really dislike this. This is a big if statement that all
> needs to go down one by one. Very inefficient!

Ok, will change to other implementation.


Thanks for your comments,
Chunyan

>
> -- Steve
>
>> +     } while (0)
>> +
>> +/*
>>   * An option specific to a tracer. This is a boolean value.
>>   * The bit is the bit index that sets its value on the
>>   * flags value in struct tracer_flags.
>

Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Steven Rostedt]

On Wed, 17 Aug 2016 20:48:07 +0800
Chunyan Zhang <zhang.chunyan@linaro.org> wrote:


> Ok, will do.
> 
> But I guess 'ftrace_ops_list' in ftrace.c [1] also should be annotated as rcu?
> 
> [1] http://lxr.free-electrons.com/source/kernel/trace/ftrace.c#L460

Hmm, perhaps it should. I wonder if sparse complains about this?




> >  
> >> +
> >> +     mutex_lock(&trace_export_lock);
> >> +
> >> +     export->tr = trace_exports_list->tr;  
> >
> > I don't see where tr is ever assigned.
> >  
> >> +     export->commit = trace_generic_commit;  
> >
> > Shouldn't the caller pass in the commit function too?  
> 
> The trace_export::write() callback is for caller, commit function
> mainly deal with traces, is it better to keep 'trace_generic_commit'
> in trace.c, i.e don't expose it to callers?

It's fine to be external if it's only declared in kernel/trace/trace.h.
I would think anything using a different "write" would require a
different "commit".

But maybe I'm misunderstanding your objective. See below.

> 
> >  
> >> +
> >> +     add_trace_export(&trace_exports_list, export);
> >> +
> >> +     mutex_unlock(&trace_export_lock);
> >> +
> >> +     return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(register_trace_export);
> >> +
> >> +int unregister_trace_export(struct trace_export *export)
> >> +{
> >> +     int ret;
> >> +
> >> +     mutex_lock(&trace_export_lock);
> >> +
> >> +     ret = rm_trace_export(&trace_exports_list, export);
> >> +
> >> +     mutex_unlock(&trace_export_lock);
> >> +
> >> +     return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(unregister_trace_export);
> >> +
> >>  void
> >>  trace_function(struct trace_array *tr,
> >>              unsigned long ip, unsigned long parent_ip, unsigned long flags,
> >> @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr,
> >>       entry->parent_ip                = parent_ip;
> >>
> >>       if (!call_filter_check_discard(call, entry, buffer, event))  
> >
> > How do you handle the discard? If the entry doesn't match the filter,
> > it will try to discard the event. I don't see how the "trace_exports"
> > handle that.  
> 
> I directly used the entries which had already been filtered.
> Humm.. sorry, actually you lost me here.

I'm assuming this entire patch is to have the events written to
something other than the ftrace ring buffer, correct?

Or is this just trying to hook into the tracing that is happening? That
is, this isn't replacing writing into the ftrace ring buffer, but it is
just adding a way to write to someplace in addition to the ftrace ring
buffer. Where you still write to the ftrace ring buffer, but then you
can add a hook to copy someplace else as well.

I was looking at this as a way that you are adding a replacement, not
only an addition to. If that's the case, I think there may be a easier
way to do this.

-- Steve

Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Chunyan Zhang]

Hi Fengguang,

On 15 August 2016 at 21:03, kbuild test robot <lkp@intel.com> wrote:
> Hi Chunyan,
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v4.8-rc2 next-20160815]
> [cannot apply to linux/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/tracing-add-a-possibility-of-exporting-function-trace-to-other-places-instead-of-ring-buffer-only/20160815-195631
> reproduce:
>         # apt-get install sparse


It may be better to use the latest version of sparse, i.e.

git clone git://git.kernel.org/pub/scm/devel/sparse/sparse.git

and then build the source code according to the direction on the page
https://kernelnewbies.org/Sparse

Otherwise, would get the errors like below if I install sparse using
apt-get install.

-------------------------------------------------------
 CHECK   arch/x86/purgatory/purgatory.c
No such file: asan-stack=1
scripts/Makefile.build:289: recipe for target
'arch/x86/purgatory/purgatory.o' failed
make[1]: *** [arch/x86/purgatory/purgatory.o] Error 1
arch/x86/Makefile:195: recipe for target 'archprepare' failed
make: *** [archprepare] Error 2
-------------------------------------------------------


Thanks,
Chunyan

>         make ARCH=x86_64 allmodconfig
>         make C=1 CF=-D__CHECK_ENDIAN__
>
> sparse warnings: (new ones prefixed by >>)
>
>    include/linux/compiler.h:230:8: sparse: attribute 'no_sanitize_address': unknown attribute
>>> kernel/trace/trace.c:2173:23: sparse: incompatible types in comparison expression (different address spaces)
>    kernel/trace/trace.c:2175:23: sparse: incompatible types in comparison expression (different address spaces)
>
> vim +2173 kernel/trace/trace.c
>
>   2157  static DEFINE_MUTEX(trace_export_lock);
>   2158
>   2159  static struct trace_export trace_export_rb __read_mostly = {
>   2160          .name           = "rb",
>   2161          .commit = trace_rb_commit,
>   2162          .next           = NULL,
>   2163  };
>   2164  static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb;
>   2165
>   2166  inline void
>   2167  trace_exports(struct trace_array *tr, struct ring_buffer_event *event)
>   2168  {
>   2169          struct trace_export *export;
>   2170
>   2171          preempt_disable_notrace();
>   2172
>> 2173          for (export = rcu_dereference_raw_notrace(trace_exports_list);
>   2174               export && export->commit;
>   2175               export = rcu_dereference_raw_notrace(export->next)) {
>   2176                  tr->export = export;
>   2177                  export->commit(tr, event);
>   2178          }
>   2179
>   2180          preempt_enable_notrace();
>   2181  }
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Chunyan Zhang]

Hi Steve,

Please correct me if I'm misunderstanding something.

On 17 August 2016 at 22:11, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 17 Aug 2016 20:48:07 +0800
> Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
>
>
>> Ok, will do.
>>
>> But I guess 'ftrace_ops_list' in ftrace.c [1] also should be annotated as rcu?
>>
>> [1] http://lxr.free-electrons.com/source/kernel/trace/ftrace.c#L460
>
> Hmm, perhaps it should. I wonder if sparse complains about this?
>

Yes, there are many this kind of errors in kernel/trace/ftrace.c and
kernel/trace/trace_events_filter.c
I can submit a patch to fix them.

>
>> >
>> >> +
>> >> +     mutex_lock(&trace_export_lock);
>> >> +
>> >> +     export->tr = trace_exports_list->tr;
>> >
>> > I don't see where tr is ever assigned.
>> >
>> >> +     export->commit = trace_generic_commit;
>> >
>> > Shouldn't the caller pass in the commit function too?
>>
>> The trace_export::write() callback is for caller, commit function
>> mainly deal with traces, is it better to keep 'trace_generic_commit'
>> in trace.c, i.e don't expose it to callers?
>
> It's fine to be external if it's only declared in kernel/trace/trace.h.

But we cannot assume that the trace_exports all are under
kernel/trace/, for example in this patchset 2/2, 'stm_ftrace' as a
trace_export was registered in stm subsystem but during its
initialization it cannot access 'trace_generic_commit' which is under
kernel/trace.

> I would think anything using a different "write" would require a
> different "commit".

Ok, I should make it more feasible rather than pointing to
'trace_generic_commit' directly when registering trace_export, that's
say, if the trace_export doesn't have its own commit function, point
directly to 'trace_generic_commit'.

>
> But maybe I'm misunderstanding your objective. See below.
>
>>
>> >
>> >> +
>> >> +     add_trace_export(&trace_exports_list, export);
>> >> +
>> >> +     mutex_unlock(&trace_export_lock);
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(register_trace_export);
>> >> +
>> >> +int unregister_trace_export(struct trace_export *export)
>> >> +{
>> >> +     int ret;
>> >> +
>> >> +     mutex_lock(&trace_export_lock);
>> >> +
>> >> +     ret = rm_trace_export(&trace_exports_list, export);
>> >> +
>> >> +     mutex_unlock(&trace_export_lock);
>> >> +
>> >> +     return ret;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(unregister_trace_export);
>> >> +
>> >>  void
>> >>  trace_function(struct trace_array *tr,
>> >>              unsigned long ip, unsigned long parent_ip, unsigned long flags,
>> >> @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr,
>> >>       entry->parent_ip                = parent_ip;
>> >>
>> >>       if (!call_filter_check_discard(call, entry, buffer, event))
>> >
>> > How do you handle the discard? If the entry doesn't match the filter,
>> > it will try to discard the event. I don't see how the "trace_exports"
>> > handle that.
>>
>> I directly used the entries which had already been filtered.
>> Humm.. sorry, actually you lost me here.
>
> I'm assuming this entire patch is to have the events written to
> something other than the ftrace ring buffer, correct?
>
> Or is this just trying to hook into the tracing that is happening? That
> is, this isn't replacing writing into the ftrace ring buffer, but it is
> just adding a way to write to someplace in addition to the ftrace ring
> buffer. Where you still write to the ftrace ring buffer, but then you
> can add a hook to copy someplace else as well.

Yes, this is what this patch is trying to implement.

>
> I was looking at this as a way that you are adding a replacement, not
> only an addition to. If that's the case, I think there may be a easier
> way to do this.

I want to know how it would be in the easier way you mentioned here.

I was trying to add a ftrace_ops before, but with that way, I have to
deal with a lot of trace or ring buffer stuff including the sort of
discard things like you mentioned, which the existed ftrace code does.
And if I choose to implement a new ftrace_ops, I'm only able to get
the function trace support for STM and have to do many things which
would be overlap with the current ftrace subsystem.

So in order to reuse the existed code and architecture, I chose to add
a trace_export interface for Ftrace subsytem, and in this way I'm
using in this patch, I will get all supports of traces which are dealt
with trace_function();

Another benefit of adding a trace_export is, if there will be other
subsystem would like to use the processed traces, it only needs to
register a trace_export and provides a .write() function call back or
together with a commit function, although from what I can see now
.write() is enough since my purpose was the processed traces I don't
need 'ring_buffer_event' so long as I had trace entries.


Thanks for your comments,
Chunyan

>
> -- Steve

Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Steven Rostedt]

On Thu, 18 Aug 2016 17:22:11 +0800
Chunyan Zhang <zhang.chunyan@linaro.org> wrote:


> > Or is this just trying to hook into the tracing that is happening? That
> > is, this isn't replacing writing into the ftrace ring buffer, but it is
> > just adding a way to write to someplace in addition to the ftrace ring
> > buffer. Where you still write to the ftrace ring buffer, but then you
> > can add a hook to copy someplace else as well.  
> 
> Yes, this is what this patch is trying to implement.
> 
> >
> > I was looking at this as a way that you are adding a replacement, not
> > only an addition to. If that's the case, I think there may be a easier
> > way to do this.  
> 
> I want to know how it would be in the easier way you mentioned here.
> 
> I was trying to add a ftrace_ops before, but with that way, I have to
> deal with a lot of trace or ring buffer stuff including the sort of
> discard things like you mentioned, which the existed ftrace code does.
> And if I choose to implement a new ftrace_ops, I'm only able to get
> the function trace support for STM and have to do many things which
> would be overlap with the current ftrace subsystem.

Adding your own ftrace_ops is a way for replacing, not just adding a
hook into.

> 
> So in order to reuse the existed code and architecture, I chose to add
> a trace_export interface for Ftrace subsytem, and in this way I'm
> using in this patch, I will get all supports of traces which are dealt
> with trace_function();

Actually, a trace_export() should only be called if there's been
something added. And that should be done with a static_key_false()
branch (which is dynamically enabled, and does not use a comparison
branch).

That is, something like this instead:

 	if (!call_filter_check_discard(call, entry, buffer, event)) {
		if (static_key_false(&ftrace_trace_exports_enabled))
			ftrace_exports(tr, event);
		__buffer_unlock_commit(buffer, event);
	}

Don't touch the current logic. Just have your code hook into the
ftrace_exports (note I use "ftrace_exports" and not trace_exports()
because it's the function tracer, which has stricter requirements than
events do. If you add a hook for tracepoints later, use trace_exports()
and have a different list for that).

> 
> Another benefit of adding a trace_export is, if there will be other
> subsystem would like to use the processed traces, it only needs to
> register a trace_export and provides a .write() function call back or
> together with a commit function, although from what I can see now
> .write() is enough since my purpose was the processed traces I don't
> need 'ring_buffer_event' so long as I had trace entries.

I'm saying if you don't mind the ring buffer being used along with
your own code (which seems to be what's happening), then just add a
call back to your code. Don't monkey with the current logic.

I think that will simplify things tremendously.

-- Steve

Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Chunyan Zhang]

On 19 August 2016 at 00:12, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 18 Aug 2016 17:22:11 +0800
> Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
>
>
>> > Or is this just trying to hook into the tracing that is happening? That
>> > is, this isn't replacing writing into the ftrace ring buffer, but it is
>> > just adding a way to write to someplace in addition to the ftrace ring
>> > buffer. Where you still write to the ftrace ring buffer, but then you
>> > can add a hook to copy someplace else as well.
>>
>> Yes, this is what this patch is trying to implement.
>>
>> >
>> > I was looking at this as a way that you are adding a replacement, not
>> > only an addition to. If that's the case, I think there may be a easier
>> > way to do this.
>>
>> I want to know how it would be in the easier way you mentioned here.
>>
>> I was trying to add a ftrace_ops before, but with that way, I have to
>> deal with a lot of trace or ring buffer stuff including the sort of
>> discard things like you mentioned, which the existed ftrace code does.
>> And if I choose to implement a new ftrace_ops, I'm only able to get
>> the function trace support for STM and have to do many things which
>> would be overlap with the current ftrace subsystem.
>
> Adding your own ftrace_ops is a way for replacing, not just adding a
> hook into.
>
>>
>> So in order to reuse the existed code and architecture, I chose to add
>> a trace_export interface for Ftrace subsytem, and in this way I'm
>> using in this patch, I will get all supports of traces which are dealt
>> with trace_function();
>
> Actually, a trace_export() should only be called if there's been
> something added. And that should be done with a static_key_false()
> branch (which is dynamically enabled, and does not use a comparison
> branch).
>
> That is, something like this instead:
>
>         if (!call_filter_check_discard(call, entry, buffer, event)) {
>                 if (static_key_false(&ftrace_trace_exports_enabled))
>                         ftrace_exports(tr, event);
>                 __buffer_unlock_commit(buffer, event);
>         }
>

Thanks for the sample code, I got it, will do like this.

> Don't touch the current logic. Just have your code hook into the
> ftrace_exports (note I use "ftrace_exports" and not trace_exports()
> because it's the function tracer, which has stricter requirements than
> events do. If you add a hook for tracepoints later, use trace_exports()
> and have a different list for that).
>
>>
>> Another benefit of adding a trace_export is, if there will be other
>> subsystem would like to use the processed traces, it only needs to
>> register a trace_export and provides a .write() function call back or
>> together with a commit function, although from what I can see now
>> .write() is enough since my purpose was the processed traces I don't
>> need 'ring_buffer_event' so long as I had trace entries.
>
> I'm saying if you don't mind the ring buffer being used along with
> your own code (which seems to be what's happening), then just add a
> call back to your code. Don't monkey with the current logic.
>
> I think that will simplify things tremendously.

Thanks for your comments and detailed explanation,
Chunyan

>
> -- Steve