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

[PATCH V7 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 function tracing
information produced by Ftrace and I'm taking the Function trace
(trace type is TRACE_FN) as the example in this patchset, but other
types of traces also can be supported.

Logging information generated by the Ftrace subsystem to STM and gathered
in the sink device 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
other architecture wishing to do the same.

Comments 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 v6:
* Rebased on v4.9-rc1;
* Removed unused the declaration and header file including from trace.h
  which was added in patch 1 of this series;
* Revised a bit the comments in trace.h .

Changes from v5:
* Addressed comments from Steven Rostedt:
  - Removed .commit() from trace_export;
  - Changed to directly call trace_process_export() instead of trace_export::commit();
  - Used 'ring_buffer_event_length(event)' instead to get the trace size;
  - Removed trace_export pointer from trace_array structure.
* Revised commit message a little to make the description more accurate.

Changes from v4:
* Addressed comments from Steven Rostedt:
  - Removed 'inline' annotations from the function which is called via function pointer;
  - Removed useless components from structure trace_export;
  - Added '__rcu' annotation to the RCU variables;
  - Used 'rcu_assign_pointer' to do every RCU assignment;
  - Added WARN_ON_ONCE() when the .write() is not assigned;
  - In order to reduce the overhead caused by adding this feature, this revision used an
    global array instead of a big "if statement" to get the size of trace entry according
    to the trace type.
  - In order to keep the current logic unchanged, made ftrace_exports() only being called if
    there's something have been added, i.e. if trace_export to stm_ftrace has been added in
    this patchset.

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              |  88 +++++++++++++++++++
 include/linux/stm.h                         |   4 +-
 include/linux/trace.h                       |  28 ++++++
 kernel/trace/trace.c                        | 132 +++++++++++++++++++++++++++-
 10 files changed, 275 insertions(+), 12 deletions(-)
 create mode 100644 drivers/hwtracing/stm/ftrace.c
 create mode 100644 include/linux/trace.h

-- 
2.7.4

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

[Chunyan Zhang]

Currently Function traces can be only exported to ring buffer, this
patch added trace_export concept which can process traces and export
them to a registered destination as an addition to the current only
one output of Ftrace - i.e. ring buffer.

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 .write() function for writing traces to storage.

With this patch, only Function trace (trace type is TRACE_FN)
is supported.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 include/linux/trace.h |  28 +++++++++++
 kernel/trace/trace.c  | 132 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 159 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..eb1c5b8
--- /dev/null
+++ b/include/linux/trace.h
@@ -0,0 +1,28 @@
+#ifndef _LINUX_TRACE_H
+#define _LINUX_TRACE_H
+
+#ifdef CONFIG_TRACING
+/*
+ * The trace export - an export of Ftrace output. The trace_export
+ * can process traces and export them to a registered destination as
+ * an addition to the current only output of Ftrace - i.e. ring buffer.
+ *
+ * If you want traces to be sent to some other place rather than ring
+ * buffer only, just need to register a new trace_export and implement
+ * its own .write() function for writing traces to the storage.
+ *
+ * next		- pointer to the next trace_export
+ * write	- copy traces which have been delt with ->commit() to
+ *		  the destination
+ */
+struct trace_export {
+	struct trace_export __rcu	*next;
+	void (*write)(const char *, unsigned int);
+};
+
+int register_ftrace_export(struct trace_export *export);
+int unregister_ftrace_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 8696ce6..db94ec1 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,132 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
 	ftrace_trace_userstack(buffer, flags, pc);
 }
 
+static void
+trace_process_export(struct trace_export *export,
+	       struct ring_buffer_event *event)
+{
+	struct trace_entry *entry;
+	unsigned int size = 0;
+
+	entry = ring_buffer_event_data(event);
+
+	size = ring_buffer_event_length(event);
+
+	if (export->write)
+		export->write((char *)entry, size);
+}
+
+static DEFINE_MUTEX(ftrace_export_lock);
+
+static struct trace_export __rcu *ftrace_exports_list __read_mostly;
+
+static DEFINE_STATIC_KEY_FALSE(ftrace_exports_enabled);
+
+static inline void ftrace_exports_enable(void)
+{
+	static_branch_enable(&ftrace_exports_enabled);
+}
+
+static inline void ftrace_exports_disable(void)
+{
+	static_branch_disable(&ftrace_exports_enabled);
+}
+
+void ftrace_exports(struct ring_buffer_event *event)
+{
+	struct trace_export *export;
+
+	preempt_disable_notrace();
+
+	export = rcu_dereference_raw_notrace(ftrace_exports_list);
+	while (export) {
+		trace_process_export(export, event);
+		export = rcu_dereference_raw_notrace(export->next);
+	}
+
+	preempt_enable_notrace();
+}
+
+static inline void
+add_trace_export(struct trace_export **list, struct trace_export *export)
+{
+	rcu_assign_pointer(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 inline int
+rm_trace_export(struct trace_export **list, struct trace_export *export)
+{
+	struct trace_export **p;
+
+	for (p = list; *p != NULL; p = &(*p)->next)
+		if (*p == export)
+			break;
+
+	if (*p != export)
+		return -1;
+
+	rcu_assign_pointer(*p, (*p)->next);
+
+	return 0;
+}
+
+static inline void
+add_ftrace_export(struct trace_export **list, struct trace_export *export)
+{
+	if (*list == NULL)
+		ftrace_exports_enable();
+
+	add_trace_export(list, export);
+}
+
+static inline int
+rm_ftrace_export(struct trace_export **list, struct trace_export *export)
+{
+	int ret;
+
+	ret = rm_trace_export(list, export);
+	if (*list == NULL)
+		ftrace_exports_disable();
+
+	return ret;
+}
+
+int register_ftrace_export(struct trace_export *export)
+{
+	if (WARN_ON_ONCE(!export->write))
+		return -1;
+
+	mutex_lock(&ftrace_export_lock);
+
+	add_ftrace_export(&ftrace_exports_list, export);
+
+	mutex_unlock(&ftrace_export_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(register_ftrace_export);
+
+int unregister_ftrace_export(struct trace_export *export)
+{
+	int ret;
+
+	mutex_lock(&ftrace_export_lock);
+
+	ret = rm_ftrace_export(&ftrace_exports_list, export);
+
+	mutex_unlock(&ftrace_export_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(unregister_ftrace_export);
+
 void
 trace_function(struct trace_array *tr,
 	       unsigned long ip, unsigned long parent_ip, unsigned long flags,
@@ -2146,8 +2273,11 @@ trace_function(struct trace_array *tr,
 	entry->ip			= ip;
 	entry->parent_ip		= parent_ip;
 
-	if (!call_filter_check_discard(call, entry, buffer, event))
+	if (!call_filter_check_discard(call, entry, buffer, event)) {
+		if (static_branch_unlikely(&ftrace_exports_enabled))
+			ftrace_exports(event);
 		__buffer_unlock_commit(buffer, event);
+	}
 }
 
 #ifdef CONFIG_STACKTRACE
-- 
2.7.4

[PATCH V7 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 called
stm_ftrace. Once the stm device and stm_ftrace have been linked via
sysfs, the driver registers itself as a trace_export and everything
passed to the interface from Ftrace subsystem will end up 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 | 88 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 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..1a114c8f
--- /dev/null
+++ b/drivers/hwtracing/stm/ftrace.c
@@ -0,0 +1,88 @@
+/*
+ * 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.
+ *
+ * STM Ftrace will be registered as a trace_export.
+ */
+
+#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);
+
+	sf->ftrace.write = stm_ftrace_write;
+	sf->ftrace.next = NULL;
+
+	return register_ftrace_export(&sf->ftrace);
+}
+
+static void stm_ftrace_unlink(struct stm_source_data *data)
+{
+	struct stm_ftrace *sf = container_of(data, struct stm_ftrace, data);
+
+	unregister_ftrace_export(&sf->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 V7 3/3] stm: Mark the functions of writing buffer with notrace

[Chunyan Zhang]

If CONFIG_STM_SOURCE_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 49e0f1b..b7543bd 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -406,7 +406,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 V7 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Steven Rostedt]

On Tue, 18 Oct 2016 16:08:58 +0800
Chunyan Zhang <zhang.chunyan@linaro.org> wrote:

> Currently Function traces can be only exported to ring buffer, this
> patch added trace_export concept which can process traces and export
> them to a registered destination as an addition to the current only
> one output of Ftrace - i.e. ring buffer.
> 
> 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 .write() function for writing traces to storage.
> 
> With this patch, only Function trace (trace type is TRACE_FN)
> is supported.

This is getting better, but I still have some nits.

> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  include/linux/trace.h |  28 +++++++++++
>  kernel/trace/trace.c  | 132 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 159 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..eb1c5b8
> --- /dev/null
> +++ b/include/linux/trace.h
> @@ -0,0 +1,28 @@
> +#ifndef _LINUX_TRACE_H
> +#define _LINUX_TRACE_H
> +
> +#ifdef CONFIG_TRACING
> +/*
> + * The trace export - an export of Ftrace output. The trace_export
> + * can process traces and export them to a registered destination as
> + * an addition to the current only output of Ftrace - i.e. ring buffer.
> + *
> + * If you want traces to be sent to some other place rather than ring
> + * buffer only, just need to register a new trace_export and implement
> + * its own .write() function for writing traces to the storage.
> + *
> + * next		- pointer to the next trace_export
> + * write	- copy traces which have been delt with ->commit() to
> + *		  the destination
> + */
> +struct trace_export {
> +	struct trace_export __rcu	*next;
> +	void (*write)(const char *, unsigned int);

Why const char*? Why not const void *? This will never be a string.


> +};
> +
> +int register_ftrace_export(struct trace_export *export);
> +int unregister_ftrace_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 8696ce6..db94ec1 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,132 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
>  	ftrace_trace_userstack(buffer, flags, pc);
>  }
>  
> +static void
> +trace_process_export(struct trace_export *export,
> +	       struct ring_buffer_event *event)
> +{
> +	struct trace_entry *entry;
> +	unsigned int size = 0;
> +
> +	entry = ring_buffer_event_data(event);
> +
> +	size = ring_buffer_event_length(event);
> +
> +	if (export->write)
> +		export->write((char *)entry, size);

Is there ever going to be a time where export->write wont be set?

And if there is, this can be racy. As in


	CPU 0:			CPU 1:
	------			------
	if (export->write)

				export->write = NULL;

	export->write(entry, size);

	BOOM!


-- Steve

> +}
> +
> +static DEFINE_MUTEX(ftrace_export_lock);
> +
> +static struct trace_export __rcu *ftrace_exports_list __read_mostly;
> +
> +static DEFINE_STATIC_KEY_FALSE(ftrace_exports_enabled);
> +
> +static inline void ftrace_exports_enable(void)
> +{
> +	static_branch_enable(&ftrace_exports_enabled);
> +}
> +
> +static inline void ftrace_exports_disable(void)
> +{
> +	static_branch_disable(&ftrace_exports_enabled);
> +}
> +
> +void ftrace_exports(struct ring_buffer_event *event)
> +{
> +	struct trace_export *export;
> +
> +	preempt_disable_notrace();
> +
> +	export = rcu_dereference_raw_notrace(ftrace_exports_list);
> +	while (export) {
> +		trace_process_export(export, event);
> +		export = rcu_dereference_raw_notrace(export->next);
> +	}
> +
> +	preempt_enable_notrace();
> +}
> +
> +static inline void
> +add_trace_export(struct trace_export **list, struct trace_export *export)
> +{
> +	rcu_assign_pointer(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 inline int
> +rm_trace_export(struct trace_export **list, struct trace_export *export)
> +{
> +	struct trace_export **p;
> +
> +	for (p = list; *p != NULL; p = &(*p)->next)
> +		if (*p == export)
> +			break;
> +
> +	if (*p != export)
> +		return -1;
> +
> +	rcu_assign_pointer(*p, (*p)->next);
> +
> +	return 0;
> +}
> +
> +static inline void
> +add_ftrace_export(struct trace_export **list, struct trace_export *export)
> +{
> +	if (*list == NULL)
> +		ftrace_exports_enable();
> +
> +	add_trace_export(list, export);
> +}
> +
> +static inline int
> +rm_ftrace_export(struct trace_export **list, struct trace_export *export)
> +{
> +	int ret;
> +
> +	ret = rm_trace_export(list, export);
> +	if (*list == NULL)
> +		ftrace_exports_disable();
> +
> +	return ret;
> +}
> +
> +int register_ftrace_export(struct trace_export *export)
> +{
> +	if (WARN_ON_ONCE(!export->write))
> +		return -1;
> +
> +	mutex_lock(&ftrace_export_lock);
> +
> +	add_ftrace_export(&ftrace_exports_list, export);
> +
> +	mutex_unlock(&ftrace_export_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_ftrace_export);
> +
> +int unregister_ftrace_export(struct trace_export *export)
> +{
> +	int ret;
> +
> +	mutex_lock(&ftrace_export_lock);
> +
> +	ret = rm_ftrace_export(&ftrace_exports_list, export);
> +
> +	mutex_unlock(&ftrace_export_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(unregister_ftrace_export);
> +
>  void
>  trace_function(struct trace_array *tr,
>  	       unsigned long ip, unsigned long parent_ip, unsigned long flags,
> @@ -2146,8 +2273,11 @@ trace_function(struct trace_array *tr,
>  	entry->ip			= ip;
>  	entry->parent_ip		= parent_ip;
>  
> -	if (!call_filter_check_discard(call, entry, buffer, event))
> +	if (!call_filter_check_discard(call, entry, buffer, event)) {
> +		if (static_branch_unlikely(&ftrace_exports_enabled))
> +			ftrace_exports(event);
>  		__buffer_unlock_commit(buffer, event);
> +	}
>  }
>  
>  #ifdef CONFIG_STACKTRACE

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

[Steven Rostedt]

On Tue, 18 Oct 2016 16:08:59 +0800
Chunyan Zhang <zhang.chunyan@linaro.org> wrote:

> This patch adds a driver that models itself as an stm_source called
> stm_ftrace. Once the stm device and stm_ftrace have been linked via
> sysfs, the driver registers itself as a trace_export and everything
> passed to the interface from Ftrace subsystem will end up 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 | 88 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 101 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

I think it should depend on FUNCTION_TRACER

> +	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..1a114c8f
> --- /dev/null
> +++ b/drivers/hwtracing/stm/ftrace.c
> @@ -0,0 +1,88 @@
> +/*
> + * 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.
> + *
> + * STM Ftrace will be registered as a trace_export.
> + */
> +
> +#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);
> +
> +	sf->ftrace.write = stm_ftrace_write;
> +	sf->ftrace.next = NULL;

Why setting this to NULL? register_ftrace_export() should not require
nor depend on that.

-- Steve

> +
> +	return register_ftrace_export(&sf->ftrace);
> +}
> +
> +static void stm_ftrace_unlink(struct stm_source_data *data)
> +{
> +	struct stm_ftrace *sf = container_of(data, struct stm_ftrace, data);
> +
> +	unregister_ftrace_export(&sf->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>");

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

[Chunyan Zhang]

On 18 October 2016 at 23:44, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 18 Oct 2016 16:08:58 +0800
> Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
>
>> Currently Function traces can be only exported to ring buffer, this
>> patch added trace_export concept which can process traces and export
>> them to a registered destination as an addition to the current only
>> one output of Ftrace - i.e. ring buffer.
>>
>> 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 .write() function for writing traces to storage.
>>
>> With this patch, only Function trace (trace type is TRACE_FN)
>> is supported.
>
> This is getting better, but I still have some nits.
>

Thanks.

>>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  include/linux/trace.h |  28 +++++++++++
>>  kernel/trace/trace.c  | 132 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 159 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..eb1c5b8
>> --- /dev/null
>> +++ b/include/linux/trace.h
>> @@ -0,0 +1,28 @@
>> +#ifndef _LINUX_TRACE_H
>> +#define _LINUX_TRACE_H
>> +
>> +#ifdef CONFIG_TRACING
>> +/*
>> + * The trace export - an export of Ftrace output. The trace_export
>> + * can process traces and export them to a registered destination as
>> + * an addition to the current only output of Ftrace - i.e. ring buffer.
>> + *
>> + * If you want traces to be sent to some other place rather than ring
>> + * buffer only, just need to register a new trace_export and implement
>> + * its own .write() function for writing traces to the storage.
>> + *
>> + * next              - pointer to the next trace_export
>> + * write     - copy traces which have been delt with ->commit() to
>> + *             the destination
>> + */
>> +struct trace_export {
>> +     struct trace_export __rcu       *next;
>> +     void (*write)(const char *, unsigned int);
>
> Why const char*? Why not const void *? This will never be a string.
>

Will revise this.

>
>> +};
>> +
>> +int register_ftrace_export(struct trace_export *export);
>> +int unregister_ftrace_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 8696ce6..db94ec1 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,132 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
>>       ftrace_trace_userstack(buffer, flags, pc);
>>  }
>>
>> +static void
>> +trace_process_export(struct trace_export *export,
>> +            struct ring_buffer_event *event)
>> +{
>> +     struct trace_entry *entry;
>> +     unsigned int size = 0;
>> +
>> +     entry = ring_buffer_event_data(event);
>> +
>> +     size = ring_buffer_event_length(event);
>> +
>> +     if (export->write)
>> +             export->write((char *)entry, size);
>
> Is there ever going to be a time where export->write wont be set?

There hasn't been since only one trace_export (i.e. stm_ftrace) was
added in this patch-set , I just wanted to make sure the write() has
been set before registering trace_export like what I added in 2/3 of
this series.

>
> And if there is, this can be racy. As in
>
>
>         CPU 0:                  CPU 1:
>         ------                  ------
>         if (export->write)
>
>                                 export->write = NULL;

Is there going to be this kind of use case? Why some one needs to
change export->write() rather than register a new trace_export?

I probably haven't understood your point thoroughly, please correct me
if my guess was wrong.


Thanks for the review,
Chunyan

>
>         export->write(entry, size);
>
>         BOOM!
>
>
> -- Steve
>
>> +}
>> +
>> +static DEFINE_MUTEX(ftrace_export_lock);
>> +
>> +static struct trace_export __rcu *ftrace_exports_list __read_mostly;
>> +
>> +static DEFINE_STATIC_KEY_FALSE(ftrace_exports_enabled);
>> +
>> +static inline void ftrace_exports_enable(void)
>> +{
>> +     static_branch_enable(&ftrace_exports_enabled);
>> +}
>> +
>> +static inline void ftrace_exports_disable(void)
>> +{
>> +     static_branch_disable(&ftrace_exports_enabled);
>> +}
>> +
>> +void ftrace_exports(struct ring_buffer_event *event)
>> +{
>> +     struct trace_export *export;
>> +
>> +     preempt_disable_notrace();
>> +
>> +     export = rcu_dereference_raw_notrace(ftrace_exports_list);
>> +     while (export) {
>> +             trace_process_export(export, event);
>> +             export = rcu_dereference_raw_notrace(export->next);
>> +     }
>> +
>> +     preempt_enable_notrace();
>> +}
>> +
>> +static inline void
>> +add_trace_export(struct trace_export **list, struct trace_export *export)
>> +{
>> +     rcu_assign_pointer(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 inline int
>> +rm_trace_export(struct trace_export **list, struct trace_export *export)
>> +{
>> +     struct trace_export **p;
>> +
>> +     for (p = list; *p != NULL; p = &(*p)->next)
>> +             if (*p == export)
>> +                     break;
>> +
>> +     if (*p != export)
>> +             return -1;
>> +
>> +     rcu_assign_pointer(*p, (*p)->next);
>> +
>> +     return 0;
>> +}
>> +
>> +static inline void
>> +add_ftrace_export(struct trace_export **list, struct trace_export *export)
>> +{
>> +     if (*list == NULL)
>> +             ftrace_exports_enable();
>> +
>> +     add_trace_export(list, export);
>> +}
>> +
>> +static inline int
>> +rm_ftrace_export(struct trace_export **list, struct trace_export *export)
>> +{
>> +     int ret;
>> +
>> +     ret = rm_trace_export(list, export);
>> +     if (*list == NULL)
>> +             ftrace_exports_disable();
>> +
>> +     return ret;
>> +}
>> +
>> +int register_ftrace_export(struct trace_export *export)
>> +{
>> +     if (WARN_ON_ONCE(!export->write))
>> +             return -1;
>> +
>> +     mutex_lock(&ftrace_export_lock);
>> +
>> +     add_ftrace_export(&ftrace_exports_list, export);
>> +
>> +     mutex_unlock(&ftrace_export_lock);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(register_ftrace_export);
>> +
>> +int unregister_ftrace_export(struct trace_export *export)
>> +{
>> +     int ret;
>> +
>> +     mutex_lock(&ftrace_export_lock);
>> +
>> +     ret = rm_ftrace_export(&ftrace_exports_list, export);
>> +
>> +     mutex_unlock(&ftrace_export_lock);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(unregister_ftrace_export);
>> +
>>  void
>>  trace_function(struct trace_array *tr,
>>              unsigned long ip, unsigned long parent_ip, unsigned long flags,
>> @@ -2146,8 +2273,11 @@ trace_function(struct trace_array *tr,
>>       entry->ip                       = ip;
>>       entry->parent_ip                = parent_ip;
>>
>> -     if (!call_filter_check_discard(call, entry, buffer, event))
>> +     if (!call_filter_check_discard(call, entry, buffer, event)) {
>> +             if (static_branch_unlikely(&ftrace_exports_enabled))
>> +                     ftrace_exports(event);
>>               __buffer_unlock_commit(buffer, event);
>> +     }
>>  }
>>
>>  #ifdef CONFIG_STACKTRACE
>

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

[Chunyan Zhang]

On 19 October 2016 at 00:29, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 18 Oct 2016 16:08:59 +0800
> Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
>
>> This patch adds a driver that models itself as an stm_source called
>> stm_ftrace. Once the stm device and stm_ftrace have been linked via
>> sysfs, the driver registers itself as a trace_export and everything
>> passed to the interface from Ftrace subsystem will end up 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 | 88 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 101 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
>
> I think it should depend on FUNCTION_TRACER

I think we should support other type of tracer in the future, but
you're right it only should depend on FUNCTION_TRACER for now, I will
revise this.

>
>> +     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..1a114c8f
>> --- /dev/null
>> +++ b/drivers/hwtracing/stm/ftrace.c
>> @@ -0,0 +1,88 @@
>> +/*
>> + * 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.
>> + *
>> + * STM Ftrace will be registered as a trace_export.
>> + */
>> +
>> +#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);
>> +
>> +     sf->ftrace.write = stm_ftrace_write;
>> +     sf->ftrace.next = NULL;
>
> Why setting this to NULL? register_ftrace_export() should not require
> nor depend on that.

Right, it's not required indeed, will remove it.

Thanks,
Chunyan

>
> -- Steve
>
>> +
>> +     return register_ftrace_export(&sf->ftrace);
>> +}
>> +
>> +static void stm_ftrace_unlink(struct stm_source_data *data)
>> +{
>> +     struct stm_ftrace *sf = container_of(data, struct stm_ftrace, data);
>> +
>> +     unregister_ftrace_export(&sf->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>");
>

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

[Chunyan Zhang]

Hi Steve,

Resend this since the subject was lost in the prior one due to unknown reason.

On 21 October 2016 at 20:13, Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
> On 18 October 2016 at 23:44, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Tue, 18 Oct 2016 16:08:58 +0800
>> Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
>>
>>> Currently Function traces can be only exported to ring buffer, this
>>> patch added trace_export concept which can process traces and export
>>> them to a registered destination as an addition to the current only
>>> one output of Ftrace - i.e. ring buffer.
>>>
>>> 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 .write() function for writing traces to storage.
>>>
>>> With this patch, only Function trace (trace type is TRACE_FN)
>>> is supported.
>>
>> This is getting better, but I still have some nits.
>>
>
> Thanks.
>
>>>
>>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>>> ---
>>>  include/linux/trace.h |  28 +++++++++++
>>>  kernel/trace/trace.c  | 132 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 159 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..eb1c5b8
>>> --- /dev/null
>>> +++ b/include/linux/trace.h
>>> @@ -0,0 +1,28 @@
>>> +#ifndef _LINUX_TRACE_H
>>> +#define _LINUX_TRACE_H
>>> +
>>> +#ifdef CONFIG_TRACING
>>> +/*
>>> + * The trace export - an export of Ftrace output. The trace_export
>>> + * can process traces and export them to a registered destination as
>>> + * an addition to the current only output of Ftrace - i.e. ring buffer.
>>> + *
>>> + * If you want traces to be sent to some other place rather than ring
>>> + * buffer only, just need to register a new trace_export and implement
>>> + * its own .write() function for writing traces to the storage.
>>> + *
>>> + * next              - pointer to the next trace_export
>>> + * write     - copy traces which have been delt with ->commit() to
>>> + *             the destination
>>> + */
>>> +struct trace_export {
>>> +     struct trace_export __rcu       *next;
>>> +     void (*write)(const char *, unsigned int);
>>
>> Why const char*? Why not const void *? This will never be a string.
>>
>
> Will revise this.
>
>>
>>> +};
>>> +
>>> +int register_ftrace_export(struct trace_export *export);
>>> +int unregister_ftrace_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 8696ce6..db94ec1 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,132 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
>>>       ftrace_trace_userstack(buffer, flags, pc);
>>>  }
>>>
>>> +static void
>>> +trace_process_export(struct trace_export *export,
>>> +            struct ring_buffer_event *event)
>>> +{
>>> +     struct trace_entry *entry;
>>> +     unsigned int size = 0;
>>> +
>>> +     entry = ring_buffer_event_data(event);
>>> +
>>> +     size = ring_buffer_event_length(event);
>>> +
>>> +     if (export->write)
>>> +             export->write((char *)entry, size);
>>
>> Is there ever going to be a time where export->write wont be set?
>
> There hasn't been since only one trace_export (i.e. stm_ftrace) was
> added in this patch-set , I just wanted to make sure the write() has
> been set before registering trace_export like what I added in 2/3 of
> this series.
>
>>
>> And if there is, this can be racy. As in
>>
>>
>>         CPU 0:                  CPU 1:
>>         ------                  ------
>>         if (export->write)
>>
>>                                 export->write = NULL;
>
> Is there going to be this kind of use case? Why some one needs to
> change export->write() rather than register a new trace_export?
>
> I probably haven't understood your point thoroughly, please correct me
> if my guess was wrong.
>

Any further comments? :)

Thanks,
Chunyan

>
> Thanks for the review,
> Chunyan
>
>>
>>         export->write(entry, size);
>>
>>         BOOM!
>>
>>
>> -- Steve
>>
>>> +}
>>> +
>>> +static DEFINE_MUTEX(ftrace_export_lock);
>>> +
>>> +static struct trace_export __rcu *ftrace_exports_list __read_mostly;
>>> +
>>> +static DEFINE_STATIC_KEY_FALSE(ftrace_exports_enabled);
>>> +
>>> +static inline void ftrace_exports_enable(void)
>>> +{
>>> +     static_branch_enable(&ftrace_exports_enabled);
>>> +}
>>> +
>>> +static inline void ftrace_exports_disable(void)
>>> +{
>>> +     static_branch_disable(&ftrace_exports_enabled);
>>> +}
>>> +
>>> +void ftrace_exports(struct ring_buffer_event *event)
>>> +{
>>> +     struct trace_export *export;
>>> +
>>> +     preempt_disable_notrace();
>>> +
>>> +     export = rcu_dereference_raw_notrace(ftrace_exports_list);
>>> +     while (export) {
>>> +             trace_process_export(export, event);
>>> +             export = rcu_dereference_raw_notrace(export->next);
>>> +     }
>>> +
>>> +     preempt_enable_notrace();
>>> +}
>>> +
>>> +static inline void
>>> +add_trace_export(struct trace_export **list, struct trace_export *export)
>>> +{
>>> +     rcu_assign_pointer(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 inline int
>>> +rm_trace_export(struct trace_export **list, struct trace_export *export)
>>> +{
>>> +     struct trace_export **p;
>>> +
>>> +     for (p = list; *p != NULL; p = &(*p)->next)
>>> +             if (*p == export)
>>> +                     break;
>>> +
>>> +     if (*p != export)
>>> +             return -1;
>>> +
>>> +     rcu_assign_pointer(*p, (*p)->next);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static inline void
>>> +add_ftrace_export(struct trace_export **list, struct trace_export *export)
>>> +{
>>> +     if (*list == NULL)
>>> +             ftrace_exports_enable();
>>> +
>>> +     add_trace_export(list, export);
>>> +}
>>> +
>>> +static inline int
>>> +rm_ftrace_export(struct trace_export **list, struct trace_export *export)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = rm_trace_export(list, export);
>>> +     if (*list == NULL)
>>> +             ftrace_exports_disable();
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +int register_ftrace_export(struct trace_export *export)
>>> +{
>>> +     if (WARN_ON_ONCE(!export->write))
>>> +             return -1;
>>> +
>>> +     mutex_lock(&ftrace_export_lock);
>>> +
>>> +     add_ftrace_export(&ftrace_exports_list, export);
>>> +
>>> +     mutex_unlock(&ftrace_export_lock);
>>> +
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(register_ftrace_export);
>>> +
>>> +int unregister_ftrace_export(struct trace_export *export)
>>> +{
>>> +     int ret;
>>> +
>>> +     mutex_lock(&ftrace_export_lock);
>>> +
>>> +     ret = rm_ftrace_export(&ftrace_exports_list, export);
>>> +
>>> +     mutex_unlock(&ftrace_export_lock);
>>> +
>>> +     return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(unregister_ftrace_export);
>>> +
>>>  void
>>>  trace_function(struct trace_array *tr,
>>>              unsigned long ip, unsigned long parent_ip, unsigned long flags,
>>> @@ -2146,8 +2273,11 @@ trace_function(struct trace_array *tr,
>>>       entry->ip                       = ip;
>>>       entry->parent_ip                = parent_ip;
>>>
>>> -     if (!call_filter_check_discard(call, entry, buffer, event))
>>> +     if (!call_filter_check_discard(call, entry, buffer, event)) {
>>> +             if (static_branch_unlikely(&ftrace_exports_enabled))
>>> +                     ftrace_exports(event);
>>>               __buffer_unlock_commit(buffer, event);
>>> +     }
>>>  }
>>>
>>>  #ifdef CONFIG_STACKTRACE
>>

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

[Steven Rostedt]

On Fri, 21 Oct 2016 20:13:13 +0800
Chunyan Zhang <zhang.chunyan@linaro.org> wrote:

> On 18 October 2016 at 23:44, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Tue, 18 Oct 2016 16:08:58 +0800
> > Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
> >  
> >> Currently Function traces can be only exported to ring buffer, this
> >> patch added trace_export concept which can process traces and export
> >> them to a registered destination as an addition to the current only
> >> one output of Ftrace - i.e. ring buffer.
> >>
> >> 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 .write() function for writing traces to storage.
> >>
> >> With this patch, only Function trace (trace type is TRACE_FN)
> >> is supported.  
> >
> > This is getting better, but I still have some nits.
> >  
> 
> Thanks.
> 
> >>
> >> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> >> ---
> >>  include/linux/trace.h |  28 +++++++++++
> >>  kernel/trace/trace.c  | 132 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 159 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..eb1c5b8
> >> --- /dev/null
> >> +++ b/include/linux/trace.h
> >> @@ -0,0 +1,28 @@
> >> +#ifndef _LINUX_TRACE_H
> >> +#define _LINUX_TRACE_H
> >> +
> >> +#ifdef CONFIG_TRACING
> >> +/*
> >> + * The trace export - an export of Ftrace output. The trace_export
> >> + * can process traces and export them to a registered destination as
> >> + * an addition to the current only output of Ftrace - i.e. ring buffer.
> >> + *
> >> + * If you want traces to be sent to some other place rather than ring
> >> + * buffer only, just need to register a new trace_export and implement
> >> + * its own .write() function for writing traces to the storage.
> >> + *
> >> + * next              - pointer to the next trace_export
> >> + * write     - copy traces which have been delt with ->commit() to
> >> + *             the destination
> >> + */
> >> +struct trace_export {
> >> +     struct trace_export __rcu       *next;
> >> +     void (*write)(const char *, unsigned int);  
> >
> > Why const char*? Why not const void *? This will never be a string.
> >  
> 
> Will revise this.
> 
> >  
> >> +};
> >> +
> >> +int register_ftrace_export(struct trace_export *export);
> >> +int unregister_ftrace_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 8696ce6..db94ec1 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,132 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
> >>       ftrace_trace_userstack(buffer, flags, pc);
> >>  }
> >>
> >> +static void
> >> +trace_process_export(struct trace_export *export,
> >> +            struct ring_buffer_event *event)
> >> +{
> >> +     struct trace_entry *entry;
> >> +     unsigned int size = 0;
> >> +
> >> +     entry = ring_buffer_event_data(event);
> >> +
> >> +     size = ring_buffer_event_length(event);
> >> +
> >> +     if (export->write)
> >> +             export->write((char *)entry, size);  
> >
> > Is there ever going to be a time where export->write wont be set?  
> 
> There hasn't been since only one trace_export (i.e. stm_ftrace) was
> added in this patch-set , I just wanted to make sure the write() has
> been set before registering trace_export like what I added in 2/3 of
> this series.
> 
> >
> > And if there is, this can be racy. As in
> >
> >
> >         CPU 0:                  CPU 1:
> >         ------                  ------
> >         if (export->write)
> >
> >                                 export->write = NULL;  
> 
> Is there going to be this kind of use case? Why some one needs to
> change export->write() rather than register a new trace_export?

Then why have a

	if (export->write)


Is there every going to be a case where export will not have a write
function?

-- Steve

> 
> I probably haven't understood your point thoroughly, please correct me
> if my guess was wrong.
> 
> 
> Thanks for the review,
> Chunyan
> 
> >
> >         export->write(entry, size);
> >
> >         BOOM!
> >
> >
> > -- Steve

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

[Chunyan Zhang]

On 14 November 2016 at 23:59, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 21 Oct 2016 20:13:13 +0800
> Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
>
>> On 18 October 2016 at 23:44, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > On Tue, 18 Oct 2016 16:08:58 +0800
>> > Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
>> >
>> >> Currently Function traces can be only exported to ring buffer, this
>> >> patch added trace_export concept which can process traces and export
>> >> them to a registered destination as an addition to the current only
>> >> one output of Ftrace - i.e. ring buffer.
>> >>
>> >> 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 .write() function for writing traces to storage.
>> >>
>> >> With this patch, only Function trace (trace type is TRACE_FN)
>> >> is supported.
>> >
>> > This is getting better, but I still have some nits.
>> >
>>
>> Thanks.
>>
>> >>
>> >> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> >> ---
>> >>  include/linux/trace.h |  28 +++++++++++
>> >>  kernel/trace/trace.c  | 132 +++++++++++++++++++++++++++++++++++++++++++++++++-
>> >>  2 files changed, 159 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..eb1c5b8
>> >> --- /dev/null
>> >> +++ b/include/linux/trace.h
>> >> @@ -0,0 +1,28 @@
>> >> +#ifndef _LINUX_TRACE_H
>> >> +#define _LINUX_TRACE_H
>> >> +
>> >> +#ifdef CONFIG_TRACING
>> >> +/*
>> >> + * The trace export - an export of Ftrace output. The trace_export
>> >> + * can process traces and export them to a registered destination as
>> >> + * an addition to the current only output of Ftrace - i.e. ring buffer.
>> >> + *
>> >> + * If you want traces to be sent to some other place rather than ring
>> >> + * buffer only, just need to register a new trace_export and implement
>> >> + * its own .write() function for writing traces to the storage.
>> >> + *
>> >> + * next              - pointer to the next trace_export
>> >> + * write     - copy traces which have been delt with ->commit() to
>> >> + *             the destination
>> >> + */
>> >> +struct trace_export {
>> >> +     struct trace_export __rcu       *next;
>> >> +     void (*write)(const char *, unsigned int);
>> >
>> > Why const char*? Why not const void *? This will never be a string.
>> >
>>
>> Will revise this.
>>
>> >
>> >> +};
>> >> +
>> >> +int register_ftrace_export(struct trace_export *export);
>> >> +int unregister_ftrace_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 8696ce6..db94ec1 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,132 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
>> >>       ftrace_trace_userstack(buffer, flags, pc);
>> >>  }
>> >>
>> >> +static void
>> >> +trace_process_export(struct trace_export *export,
>> >> +            struct ring_buffer_event *event)
>> >> +{
>> >> +     struct trace_entry *entry;
>> >> +     unsigned int size = 0;
>> >> +
>> >> +     entry = ring_buffer_event_data(event);
>> >> +
>> >> +     size = ring_buffer_event_length(event);
>> >> +
>> >> +     if (export->write)
>> >> +             export->write((char *)entry, size);
>> >
>> > Is there ever going to be a time where export->write wont be set?
>>
>> There hasn't been since only one trace_export (i.e. stm_ftrace) was
>> added in this patch-set , I just wanted to make sure the write() has
>> been set before registering trace_export like what I added in 2/3 of
>> this series.
>>
>> >
>> > And if there is, this can be racy. As in
>> >
>> >
>> >         CPU 0:                  CPU 1:
>> >         ------                  ------
>> >         if (export->write)
>> >
>> >                                 export->write = NULL;
>>
>> Is there going to be this kind of use case? Why some one needs to
>> change export->write() rather than register a new trace_export?
>
> Then why have a
>
>         if (export->write)
>
>
> Is there every going to be a case where export will not have a write
> function?

There shouldn't be.

I can move this if statement to the register_ftrace_export() to ensure
users won't wrongly use it, that's saying the write() of trace_export
has been set before being registered to 'ftrace_exports_list'.

Thanks,
Chunyan

>
> -- Steve
>
>>
>> I probably haven't understood your point thoroughly, please correct me
>> if my guess was wrong.
>>
>>
>> Thanks for the review,
>> Chunyan
>>
>> >
>> >         export->write(entry, size);
>> >
>> >         BOOM!
>> >
>> >
>> > -- Steve

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

[Steven Rostedt]

On Tue, 15 Nov 2016 16:14:29 +0800
Chunyan Zhang <zhang.chunyan@linaro.org> wrote:

>
> > Then why have a
> >
> >         if (export->write)
> >
> >
> > Is there every going to be a case where export will not have a write
> > function?  
> 
> There shouldn't be.
> 
> I can move this if statement to the register_ftrace_export() to ensure
> users won't wrongly use it, that's saying the write() of trace_export
> has been set before being registered to 'ftrace_exports_list'.
> 

Looks like it's already there:

+int register_ftrace_export(struct trace_export *export)
+{
+	if (WARN_ON_ONCE(!export->write))
+		return -1;


-- Steve