[RFC PATCH 0/4] Integration of function trace with System Trace IP blocks

[RFC PATCH 0/4] 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 an RFC aimed at generating ideas on the best way to
use STM IP blocks to collect function tracing information produced by
Ftrace.  That way 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
[4]. http://lxr.free-electrons.com/source/include/linux/stm.h

Chunyan Zhang (4):
  STM Ftrace: Adding generic buffer interface driver
  trace: Introduce an output interface from ftrace to STM
  trace: Duplicate the output of the function trace logs to STM
  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               |  9 +++++
 drivers/hwtracing/stm/Makefile              |  2 ++
 drivers/hwtracing/stm/core.c                |  7 ++--
 drivers/hwtracing/stm/dummy_stm.c           |  2 +-
 drivers/hwtracing/stm/stm_ftrace.c          | 54 +++++++++++++++++++++++++++++
 include/linux/stm.h                         |  4 +--
 kernel/trace/Makefile                       |  1 +
 kernel/trace/trace.c                        |  5 ++-
 kernel/trace/trace_output_stm.c             | 27 +++++++++++++++
 kernel/trace/trace_output_stm.h             | 14 ++++++++
 12 files changed, 126 insertions(+), 12 deletions(-)
 create mode 100644 drivers/hwtracing/stm/stm_ftrace.c
 create mode 100644 kernel/trace/trace_output_stm.c
 create mode 100644 kernel/trace/trace_output_stm.h

-- 
1.9.1

[RFC PATCH 1/4] STM Ftrace: Adding generic buffer interface driver

[Chunyan Zhang]

This patch adds a driver that models itself as an stm_source and
who's sole purpose is to export an interface to the rest of the
kernel.  Once the stm and stm_source have been linked via sysfs,
everything that is passed to the interface will endup in the STM
trace engine.

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

diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
index 847a39b..a36633a 100644
--- a/drivers/hwtracing/stm/Kconfig
+++ b/drivers/hwtracing/stm/Kconfig
@@ -39,4 +39,13 @@ config STM_SOURCE_HEARTBEAT
 	  If you want to send heartbeat messages over STM devices,
 	  say Y.
 
+config STM_FTRACE
+	tristate "Redirect/copy the output from kernel Ftrace to STM engine"
+	help
+	  This option can be used to redirect or 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..5eef041 100644
--- a/drivers/hwtracing/stm/Makefile
+++ b/drivers/hwtracing/stm/Makefile
@@ -9,3 +9,5 @@ obj-$(CONFIG_STM_SOURCE_HEARTBEAT)	+= stm_heartbeat.o
 
 stm_console-y		:= console.o
 stm_heartbeat-y		:= heartbeat.o
+
+obj-$(CONFIG_STM_FTRACE)	+= stm_ftrace.o
diff --git a/drivers/hwtracing/stm/stm_ftrace.c b/drivers/hwtracing/stm/stm_ftrace.c
new file mode 100644
index 0000000..0b4eb70
--- /dev/null
+++ b/drivers/hwtracing/stm/stm_ftrace.c
@@ -0,0 +1,54 @@
+/*
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/console.h>
+#include <linux/stm.h>
+
+static struct stm_source_data stm_ftrace_data = {
+	.name		= "stm_ftrace",
+	.nr_chans	= 1,
+};
+
+/**
+ * stm_ftrace_write() - write data to STM via 'stm_ftrace' source
+ * @buf:	buffer containing the data packet
+ * @len:	length of the data packet
+ * @chan:	offset above the start channel number allocated to 'stm_ftrace'
+ */
+void notrace stm_ftrace_write(const char *buf, unsigned int len,
+			      unsigned int chan)
+{
+	stm_source_write(&stm_ftrace_data, chan, buf, len);
+}
+EXPORT_SYMBOL_GPL(stm_ftrace_write);
+
+static int __init stm_ftrace_init(void)
+{
+	return stm_source_register_device(NULL, &stm_ftrace_data);
+}
+
+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>");
-- 
1.9.1

Re: [RFC PATCH 1/4] STM Ftrace: Adding generic buffer interface driver

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> This patch adds a driver that models itself as an stm_source and
> who's sole purpose is to export an interface to the rest of the
> kernel.  Once the stm and stm_source have been linked via sysfs,
> everything that is passed to the interface will endup in the STM
> trace engine.

STM core already provides this exact interface to the rest of the
kernel. You need something that ftrace will call into to export its
traces.

>
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/hwtracing/stm/Kconfig      |  9 +++++++
>  drivers/hwtracing/stm/Makefile     |  2 ++
>  drivers/hwtracing/stm/stm_ftrace.c | 54 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
>  create mode 100644 drivers/hwtracing/stm/stm_ftrace.c
>
> diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
> index 847a39b..a36633a 100644
> --- a/drivers/hwtracing/stm/Kconfig
> +++ b/drivers/hwtracing/stm/Kconfig
> @@ -39,4 +39,13 @@ config STM_SOURCE_HEARTBEAT
>  	  If you want to send heartbeat messages over STM devices,
>  	  say Y.
>  
> +config STM_FTRACE
> +	tristate "Redirect/copy the output from kernel Ftrace to STM engine"
> +	help
> +	  This option can be used to redirect or copy the output from kernel Ftrace
> +	  to STM engine. Enabling this option will introduce a slight timing effect.

This creates an impression that STM_FTRACE will somehow make events
bypass the normal ftrace ring buffer.

> +
> +	  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..5eef041 100644
> --- a/drivers/hwtracing/stm/Makefile
> +++ b/drivers/hwtracing/stm/Makefile
> @@ -9,3 +9,5 @@ obj-$(CONFIG_STM_SOURCE_HEARTBEAT)	+= stm_heartbeat.o
>  
>  stm_console-y		:= console.o
>  stm_heartbeat-y		:= heartbeat.o
> +
> +obj-$(CONFIG_STM_FTRACE)	+= stm_ftrace.o

We don't have any other source files prefixed with "stm_" in
drivers/hwtracing/stm, because the directory is already "stm".

> diff --git a/drivers/hwtracing/stm/stm_ftrace.c b/drivers/hwtracing/stm/stm_ftrace.c
> new file mode 100644
> index 0000000..0b4eb70
> --- /dev/null
> +++ b/drivers/hwtracing/stm/stm_ftrace.c
> @@ -0,0 +1,54 @@
> +/*
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/console.h>
> +#include <linux/stm.h>
> +
> +static struct stm_source_data stm_ftrace_data = {
> +	.name		= "stm_ftrace",
> +	.nr_chans	= 1,

Unless you want to allocate one channel per cpu core, which might or
might not be more efficient, but it would be nice to see at least a
comment about it.

> +};
> +
> +/**
> + * stm_ftrace_write() - write data to STM via 'stm_ftrace' source
> + * @buf:	buffer containing the data packet
> + * @len:	length of the data packet
> + * @chan:	offset above the start channel number allocated to 'stm_ftrace'
> + */
> +void notrace stm_ftrace_write(const char *buf, unsigned int len,
> +			      unsigned int chan)
> +{
> +	stm_source_write(&stm_ftrace_data, chan, buf, len);
> +}
> +EXPORT_SYMBOL_GPL(stm_ftrace_write);

An extra wrapper around stm_source_write().

> +
> +static int __init stm_ftrace_init(void)
> +{
> +	return stm_source_register_device(NULL, &stm_ftrace_data);
> +}
> +
> +static void __exit stm_ftrace_exit(void)
> +{
> +	stm_source_unregister_device(&stm_ftrace_data);
> +}

So basically when ftrace is compiled in, it will pull in stm core
through this.

Regards,
--
Alex

Re: [RFC PATCH 1/4] STM Ftrace: Adding generic buffer interface driver

[Chunyan Zhang]

On Tue, Jun 7, 2016 at 6:25 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> This patch adds a driver that models itself as an stm_source and
>> who's sole purpose is to export an interface to the rest of the
>> kernel.  Once the stm and stm_source have been linked via sysfs,
>> everything that is passed to the interface will endup in the STM
>> trace engine.
>
> STM core already provides this exact interface to the rest of the

Can you point out 'this exact interface' to me?

> kernel. You need something that ftrace will call into to export its
> traces.
>
>>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  drivers/hwtracing/stm/Kconfig      |  9 +++++++
>>  drivers/hwtracing/stm/Makefile     |  2 ++
>>  drivers/hwtracing/stm/stm_ftrace.c | 54 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 65 insertions(+)
>>  create mode 100644 drivers/hwtracing/stm/stm_ftrace.c
>>
>> diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
>> index 847a39b..a36633a 100644
>> --- a/drivers/hwtracing/stm/Kconfig
>> +++ b/drivers/hwtracing/stm/Kconfig
>> @@ -39,4 +39,13 @@ config STM_SOURCE_HEARTBEAT
>>         If you want to send heartbeat messages over STM devices,
>>         say Y.
>>
>> +config STM_FTRACE
>> +     tristate "Redirect/copy the output from kernel Ftrace to STM engine"
>> +     help
>> +       This option can be used to redirect or copy the output from kernel Ftrace
>> +       to STM engine. Enabling this option will introduce a slight timing effect.
>
> This creates an impression that STM_FTRACE will somehow make events
> bypass the normal ftrace ring buffer.

Ok, this name can be adjusted, do you have a better one for me :)

>
>> +
>> +       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..5eef041 100644
>> --- a/drivers/hwtracing/stm/Makefile
>> +++ b/drivers/hwtracing/stm/Makefile
>> @@ -9,3 +9,5 @@ obj-$(CONFIG_STM_SOURCE_HEARTBEAT)    += stm_heartbeat.o
>>
>>  stm_console-y                := console.o
>>  stm_heartbeat-y              := heartbeat.o
>> +
>> +obj-$(CONFIG_STM_FTRACE)     += stm_ftrace.o
>
> We don't have any other source files prefixed with "stm_" in
> drivers/hwtracing/stm, because the directory is already "stm".

Ok, I will remove 'stm_' prefix.

>
>> diff --git a/drivers/hwtracing/stm/stm_ftrace.c b/drivers/hwtracing/stm/stm_ftrace.c
>> new file mode 100644
>> index 0000000..0b4eb70
>> --- /dev/null
>> +++ b/drivers/hwtracing/stm/stm_ftrace.c
>> @@ -0,0 +1,54 @@
>> +/*
>> + * 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/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/console.h>
>> +#include <linux/stm.h>
>> +
>> +static struct stm_source_data stm_ftrace_data = {
>> +     .name           = "stm_ftrace",
>> +     .nr_chans       = 1,
>
> Unless you want to allocate one channel per cpu core, which might or
> might not be more efficient, but it would be nice to see at least a
> comment about it.
>
>> +};
>> +
>> +/**
>> + * stm_ftrace_write() - write data to STM via 'stm_ftrace' source
>> + * @buf:     buffer containing the data packet
>> + * @len:     length of the data packet
>> + * @chan:    offset above the start channel number allocated to 'stm_ftrace'
>> + */
>> +void notrace stm_ftrace_write(const char *buf, unsigned int len,
>> +                           unsigned int chan)
>> +{
>> +     stm_source_write(&stm_ftrace_data, chan, buf, len);
>> +}
>> +EXPORT_SYMBOL_GPL(stm_ftrace_write);
>
> An extra wrapper around stm_source_write().

Yes, I think it's not good to expose the stm_source to ftrace_stm_func().

>
>> +
>> +static int __init stm_ftrace_init(void)
>> +{
>> +     return stm_source_register_device(NULL, &stm_ftrace_data);
>> +}
>> +
>> +static void __exit stm_ftrace_exit(void)
>> +{
>> +     stm_source_unregister_device(&stm_ftrace_data);
>> +}
>
> So basically when ftrace is compiled in, it will pull in stm core
> through this.

Sorry I cannot get you here.  Could you please explain you concern further?

Thanks,
Chunyan

>
> Regards,
> --
> Alex

Re: [RFC PATCH 1/4] STM Ftrace: Adding generic buffer interface driver

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> On Tue, Jun 7, 2016 at 6:25 PM, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>
>>> This patch adds a driver that models itself as an stm_source and
>>> who's sole purpose is to export an interface to the rest of the
>>> kernel.  Once the stm and stm_source have been linked via sysfs,
>>> everything that is passed to the interface will endup in the STM
>>> trace engine.
>>
>> STM core already provides this exact interface to the rest of the
>
> Can you point out 'this exact interface' to me?

Well, you're saying that this stm_source exports an interface to send
data to STM for the rest of the kernel. Whereas, stm_source already is
that interface.

>>> +config STM_FTRACE
>>> +     tristate "Redirect/copy the output from kernel Ftrace to STM engine"
>>> +     help
>>> +       This option can be used to redirect or copy the output from kernel Ftrace
>>> +       to STM engine. Enabling this option will introduce a slight timing effect.
>>
>> This creates an impression that STM_FTRACE will somehow make events
>> bypass the normal ftrace ring buffer.
>
> Ok, this name can be adjusted, do you have a better one for me :)

What I mean is: from the description it sounds like there is an option
to bypass ftrace ring buffer, but I don't think that's the case at the
moment. I'm also not sure if it's practical at all to do.

>>> +/**
>>> + * stm_ftrace_write() - write data to STM via 'stm_ftrace' source
>>> + * @buf:     buffer containing the data packet
>>> + * @len:     length of the data packet
>>> + * @chan:    offset above the start channel number allocated to 'stm_ftrace'
>>> + */
>>> +void notrace stm_ftrace_write(const char *buf, unsigned int len,
>>> +                           unsigned int chan)
>>> +{
>>> +     stm_source_write(&stm_ftrace_data, chan, buf, len);
>>> +}
>>> +EXPORT_SYMBOL_GPL(stm_ftrace_write);
>>
>> An extra wrapper around stm_source_write().
>
> Yes, I think it's not good to expose the stm_source to ftrace_stm_func().

I understand, but wrapping it into an intermediary function doesn't
really solve it either.

>> So basically when ftrace is compiled in, it will pull in stm core
>> through this.
>
> Sorry I cannot get you here.  Could you please explain you concern further?

Well, if you plug the stm_source driver into the ftrace core (via a
wrapper or directly), you will end up with a link dependency. In other
words, stm_source and by association stm_core will have to be statically
linked.

Look at the way stm_console is done, for example: it registers with both
stm_source class and the console layer dynamically, so that it can be
dynamically loaded/unloaded.

Regards,
--
Alex

Re: [RFC PATCH 1/4] STM Ftrace: Adding generic buffer interface driver

[Chunyan Zhang]

On Wed, Jun 8, 2016 at 8:13 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> On Tue, Jun 7, 2016 at 6:25 PM, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>>
>>>> This patch adds a driver that models itself as an stm_source and
>>>> who's sole purpose is to export an interface to the rest of the
>>>> kernel.  Once the stm and stm_source have been linked via sysfs,
>>>> everything that is passed to the interface will endup in the STM
>>>> trace engine.
>>>
>>> STM core already provides this exact interface to the rest of the
>>
>> Can you point out 'this exact interface' to me?
>
> Well, you're saying that this stm_source exports an interface to send
> data to STM for the rest of the kernel. Whereas, stm_source already is
> that interface.

Ok, got it now.  I'll revise this change log in the next version.

>
>>>> +config STM_FTRACE
>>>> +     tristate "Redirect/copy the output from kernel Ftrace to STM engine"
>>>> +     help
>>>> +       This option can be used to redirect or copy the output from kernel Ftrace
>>>> +       to STM engine. Enabling this option will introduce a slight timing effect.
>>>
>>> This creates an impression that STM_FTRACE will somehow make events
>>> bypass the normal ftrace ring buffer.
>>
>> Ok, this name can be adjusted, do you have a better one for me :)
>
> What I mean is: from the description it sounds like there is an option
> to bypass ftrace ring buffer, but I don't think that's the case at the
> moment. I'm also not sure if it's practical at all to do.
>
>>>> +/**
>>>> + * stm_ftrace_write() - write data to STM via 'stm_ftrace' source
>>>> + * @buf:     buffer containing the data packet
>>>> + * @len:     length of the data packet
>>>> + * @chan:    offset above the start channel number allocated to 'stm_ftrace'
>>>> + */
>>>> +void notrace stm_ftrace_write(const char *buf, unsigned int len,
>>>> +                           unsigned int chan)
>>>> +{
>>>> +     stm_source_write(&stm_ftrace_data, chan, buf, len);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(stm_ftrace_write);
>>>
>>> An extra wrapper around stm_source_write().
>>
>> Yes, I think it's not good to expose the stm_source to ftrace_stm_func().
>
> I understand, but wrapping it into an intermediary function doesn't
> really solve it either.
>
>>> So basically when ftrace is compiled in, it will pull in stm core
>>> through this.
>>
>> Sorry I cannot get you here.  Could you please explain you concern further?
>
> Well, if you plug the stm_source driver into the ftrace core (via a
> wrapper or directly), you will end up with a link dependency. In other
> words, stm_source and by association stm_core will have to be statically
> linked.
>
> Look at the way stm_console is done, for example: it registers with both
> stm_source class and the console layer dynamically, so that it can be
> dynamically loaded/unloaded.

Yes, I just looked at stm_console implementation, it is an good
example. I may finally got your point.
Declaring stm_ftrace as a struct, then Ftrace only needs an instance
of stm_ftrace from its own side.  That way, ftrace subsystem and
stm_ftrace are more independent of each other, and stm_ftrace can be
dynamically loaded/unloaded like you suggested.

I will revise this patchset.

Thanks for your comments,
Chunyan

>
> Regards,
> --
> Alex

[RFC PATCH 2/4] trace: Introduce an output interface from ftrace to STM

[Chunyan Zhang]

This patch is introducing a new function to print Ftrace messages
to STM buffer when the traces happen.  In order to reduce the
effect on timing overhead as much as possible, only the current
function and its parent ip address will be recorded into STM in
this patch.  This idea was first introduced by Philippe Langlais
at ST-Microelectronics a long time ago[1].

[1] http://thread.gmane.org/gmane.linux.kernel/1114274/focus=1114275

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 kernel/trace/Makefile           |  1 +
 kernel/trace/trace_output_stm.c | 27 +++++++++++++++++++++++++++
 kernel/trace/trace_output_stm.h | 14 ++++++++++++++
 3 files changed, 42 insertions(+)
 create mode 100644 kernel/trace/trace_output_stm.c
 create mode 100644 kernel/trace/trace_output_stm.h

diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index 979e7bf..445afa8 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -69,4 +69,5 @@ obj-$(CONFIG_UPROBE_EVENT) += trace_uprobe.o
 
 obj-$(CONFIG_TRACEPOINT_BENCHMARK) += trace_benchmark.o
 
+obj-$(CONFIG_STM_FTRACE) += trace_output_stm.o
 libftrace-y := ftrace.o
diff --git a/kernel/trace/trace_output_stm.c b/kernel/trace/trace_output_stm.c
new file mode 100644
index 0000000..f6a3c01
--- /dev/null
+++ b/kernel/trace/trace_output_stm.c
@@ -0,0 +1,27 @@
+/*
+ * Output interface from Ftrace to STM buffer
+ * 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 "trace_output_stm.h"
+
+/* Offset above the start channel number */
+#define STM_FTRACE_CHAN 0
+
+void ftrace_stm_func(unsigned long ip, unsigned long parent_ip)
+{
+	unsigned long ip_array[2] = {ip, parent_ip};
+
+	stm_ftrace_write((char *)ip_array, sizeof(unsigned long) * 2,
+			 STM_FTRACE_CHAN);
+}
+EXPORT_SYMBOL_GPL(ftrace_stm_func);
diff --git a/kernel/trace/trace_output_stm.h b/kernel/trace/trace_output_stm.h
new file mode 100644
index 0000000..fc3f989
--- /dev/null
+++ b/kernel/trace/trace_output_stm.h
@@ -0,0 +1,14 @@
+#ifndef __TRACE_OUTPUT_STM_H
+#define __TRACE_OUTPUT_STM_H
+
+#include <linux/module.h>
+
+#ifdef CONFIG_STM_FTRACE
+extern void stm_ftrace_write(const char *buf, unsigned int len,
+			     unsigned int chan);
+extern void ftrace_stm_func(unsigned long ip, unsigned long parent_ip);
+#else
+static inline void ftrace_stm_func(unsigned long ip, unsigned long parent_ip) {}
+#endif
+
+#endif /* __TRACE_OUTPUT_STM_H */
-- 
1.9.1

Re: [RFC PATCH 2/4] trace: Introduce an output interface from ftrace to STM

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> This patch is introducing a new function to print Ftrace messages
> to STM buffer when the traces happen.  In order to reduce the
> effect on timing overhead as much as possible, only the current
> function and its parent ip address will be recorded into STM in
> this patch.  This idea was first introduced by Philippe Langlais
> at ST-Microelectronics a long time ago[1].

So why is this useful? The value of trace points is in their payload.

> +#define STM_FTRACE_CHAN 0

This is why we have the stm master/channel allocation policy, which
should already assign a channel to your stm_source device when it is
linked to an stm device.

Also, why is this a separate compilation unit from the stm_ftrace.o?

> +
> +void ftrace_stm_func(unsigned long ip, unsigned long parent_ip)
> +{
> +	unsigned long ip_array[2] = {ip, parent_ip};
> +
> +	stm_ftrace_write((char *)ip_array, sizeof(unsigned long) * 2,
> +			 STM_FTRACE_CHAN);
> +}
> +EXPORT_SYMBOL_GPL(ftrace_stm_func);
> diff --git a/kernel/trace/trace_output_stm.h b/kernel/trace/trace_output_stm.h
> new file mode 100644
> index 0000000..fc3f989
> --- /dev/null
> +++ b/kernel/trace/trace_output_stm.h
> @@ -0,0 +1,14 @@
> +#ifndef __TRACE_OUTPUT_STM_H
> +#define __TRACE_OUTPUT_STM_H
> +
> +#include <linux/module.h>
> +
> +#ifdef CONFIG_STM_FTRACE
> +extern void stm_ftrace_write(const char *buf, unsigned int len,
> +			     unsigned int chan);
> +extern void ftrace_stm_func(unsigned long ip, unsigned long parent_ip);
> +#else
> +static inline void ftrace_stm_func(unsigned long ip, unsigned long parent_ip) {}
> +#endif
> +
> +#endif /* __TRACE_OUTPUT_STM_H */
> -- 
> 1.9.1

Re: [RFC PATCH 2/4] trace: Introduce an output interface from ftrace to STM

[Chunyan Zhang]

On Tue, Jun 7, 2016 at 6:04 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> This patch is introducing a new function to print Ftrace messages
>> to STM buffer when the traces happen.  In order to reduce the
>> effect on timing overhead as much as possible, only the current
>> function and its parent ip address will be recorded into STM in
>> this patch.  This idea was first introduced by Philippe Langlais
>> at ST-Microelectronics a long time ago[1].
>
> So why is this useful? The value of trace points is in their payload.

Sorry, didn't get you, what did you mean by "The value of trace points" here?

>
>> +#define STM_FTRACE_CHAN 0
>
> This is why we have the stm master/channel allocation policy, which
> should already assign a channel to your stm_source device when it is
> linked to an stm device.

If I understand correctly, STM core can assign many channels for one
stm_source, the number we want to be allocated is specified by
'stm_source_data.nr_chans', for this patchset it's 1 for the time
being, STM_FTRACE_CHAN is the index in the range of channels allocated
to this stm_source by STM core.

>
> Also, why is this a separate compilation unit from the stm_ftrace.o?

My idea was stm_ftrace.c is part of STM driver, this patch is a
process of traces from Ftrace subsystem.  These two parts are small so
far, since this serial only added function trace exporting to STM, but
I want to add more functionalities to export more kinds of traces
which Ftrace subsystem supports to STM in the feature, my thought is
dividing into two parts like this serial did is convenient to add
other feature to each part.

>
>> +
>> +void ftrace_stm_func(unsigned long ip, unsigned long parent_ip)
>> +{
>> +     unsigned long ip_array[2] = {ip, parent_ip};
>> +
>> +     stm_ftrace_write((char *)ip_array, sizeof(unsigned long) * 2,
>> +                      STM_FTRACE_CHAN);
>> +}
>> +EXPORT_SYMBOL_GPL(ftrace_stm_func);
>> diff --git a/kernel/trace/trace_output_stm.h b/kernel/trace/trace_output_stm.h
>> new file mode 100644
>> index 0000000..fc3f989
>> --- /dev/null
>> +++ b/kernel/trace/trace_output_stm.h
>> @@ -0,0 +1,14 @@
>> +#ifndef __TRACE_OUTPUT_STM_H
>> +#define __TRACE_OUTPUT_STM_H
>> +
>> +#include <linux/module.h>
>> +
>> +#ifdef CONFIG_STM_FTRACE
>> +extern void stm_ftrace_write(const char *buf, unsigned int len,
>> +                          unsigned int chan);
>> +extern void ftrace_stm_func(unsigned long ip, unsigned long parent_ip);
>> +#else
>> +static inline void ftrace_stm_func(unsigned long ip, unsigned long parent_ip) {}
>> +#endif
>> +
>> +#endif /* __TRACE_OUTPUT_STM_H */
>> --
>> 1.9.1

Re: [RFC PATCH 2/4] trace: Introduce an output interface from ftrace to STM

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> On Tue, Jun 7, 2016 at 6:04 PM, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>
>>> This patch is introducing a new function to print Ftrace messages
>>> to STM buffer when the traces happen.  In order to reduce the
>>> effect on timing overhead as much as possible, only the current
>>> function and its parent ip address will be recorded into STM in
>>> this patch.  This idea was first introduced by Philippe Langlais
>>> at ST-Microelectronics a long time ago[1].
>>
>> So why is this useful? The value of trace points is in their payload.
>
> Sorry, didn't get you, what did you mean by "The value of trace points" here?

I suggest that you read about ftrace usage and in particular the types
of records that end up in its ring buffer.

>
>>
>>> +#define STM_FTRACE_CHAN 0
>>
>> This is why we have the stm master/channel allocation policy, which
>> should already assign a channel to your stm_source device when it is
>> linked to an stm device.
>
> If I understand correctly, STM core can assign many channels for one
> stm_source, the number we want to be allocated is specified by
> 'stm_source_data.nr_chans', for this patchset it's 1 for the time
> being, STM_FTRACE_CHAN is the index in the range of channels allocated
> to this stm_source by STM core.

Yes, I misread the code, apologies.

>> Also, why is this a separate compilation unit from the stm_ftrace.o?
>
> My idea was stm_ftrace.c is part of STM driver, this patch is a
> process of traces from Ftrace subsystem.  These two parts are small so
> far, since this serial only added function trace exporting to STM, but
> I want to add more functionalities to export more kinds of traces
> which Ftrace subsystem supports to STM in the feature, my thought is
> dividing into two parts like this serial did is convenient to add
> other feature to each part.

Leaving aside the question of different ftrace functionalities for a
moment, what is the benefit of splitting stm_ftrace like this? The only
purpose of stm_ftrace is to take data from ftrace and send it down to an
stm device, using necessary intefaces on both ends and performing
additional framing if required.

If would make sense if the ftrace part gets a notion of "output" or
"sink" interface, which stm_ftrace can declare itself to be an
implementation of.

Regards,
--
Alex

Re: [RFC PATCH 2/4] trace: Introduce an output interface from ftrace to STM

[Chunyan Zhang]

On Tue, Jun 7, 2016 at 6:04 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> This patch is introducing a new function to print Ftrace messages
>> to STM buffer when the traces happen.  In order to reduce the
>> effect on timing overhead as much as possible, only the current
>> function and its parent ip address will be recorded into STM in
>> this patch.  This idea was first introduced by Philippe Langlais
>> at ST-Microelectronics a long time ago[1].
>
> So why is this useful? The value of trace points is in their payload.

>From what I understand, we have no way to log as much information as
Ftrace has done if we want to control the overhead of writing STM in a
limited/acceptable range.  Exporting information to STM is much more
time-consuming than writing ring buffer, the thought was simply that
export as much useful information as possible while taking a limited
bytes.

Thanks,
Chunyan

>
>> +#define STM_FTRACE_CHAN 0
>
> This is why we have the stm master/channel allocation policy, which
> should already assign a channel to your stm_source device when it is
> linked to an stm device.
>
> Also, why is this a separate compilation unit from the stm_ftrace.o?
>
>> +
>> +void ftrace_stm_func(unsigned long ip, unsigned long parent_ip)
>> +{
>> +     unsigned long ip_array[2] = {ip, parent_ip};
>> +
>> +     stm_ftrace_write((char *)ip_array, sizeof(unsigned long) * 2,
>> +                      STM_FTRACE_CHAN);
>> +}
>> +EXPORT_SYMBOL_GPL(ftrace_stm_func);
>> diff --git a/kernel/trace/trace_output_stm.h b/kernel/trace/trace_output_stm.h
>> new file mode 100644
>> index 0000000..fc3f989
>> --- /dev/null
>> +++ b/kernel/trace/trace_output_stm.h
>> @@ -0,0 +1,14 @@
>> +#ifndef __TRACE_OUTPUT_STM_H
>> +#define __TRACE_OUTPUT_STM_H
>> +
>> +#include <linux/module.h>
>> +
>> +#ifdef CONFIG_STM_FTRACE
>> +extern void stm_ftrace_write(const char *buf, unsigned int len,
>> +                          unsigned int chan);
>> +extern void ftrace_stm_func(unsigned long ip, unsigned long parent_ip);
>> +#else
>> +static inline void ftrace_stm_func(unsigned long ip, unsigned long parent_ip) {}
>> +#endif
>> +
>> +#endif /* __TRACE_OUTPUT_STM_H */
>> --
>> 1.9.1

[RFC PATCH 3/4] trace: Duplicate the output of the function trace logs to STM

[Chunyan Zhang]

This patch adds an output from Ftrace to STM.  That being said,
Function trace messages would also be duplicated to STM buffer when
being stored into ring buffer.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 kernel/trace/trace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8a4bd6b..d613053 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -44,6 +44,7 @@
 
 #include "trace.h"
 #include "trace_output.h"
+#include "trace_output_stm.h"
 
 /*
  * On boot up, the ring buffer is set to the minimum size, so that
@@ -1884,8 +1885,10 @@ 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)) {
 		__buffer_unlock_commit(buffer, event);
+		ftrace_stm_func(ip, parent_ip);
+	}
 }
 
 #ifdef CONFIG_STACKTRACE
-- 
1.9.1

Re: [RFC PATCH 3/4] trace: Duplicate the output of the function trace logs to STM

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> This patch adds an output from Ftrace to STM.

But does it?

> That being said,
> Function trace messages would also be duplicated to STM buffer when
> being stored into ring buffer.

Not sure what you mean here. What's "STM buffer"?

>
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  kernel/trace/trace.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 8a4bd6b..d613053 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -44,6 +44,7 @@
>  
>  #include "trace.h"
>  #include "trace_output.h"
> +#include "trace_output_stm.h"
>  
>  /*
>   * On boot up, the ring buffer is set to the minimum size, so that
> @@ -1884,8 +1885,10 @@ 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)) {
>  		__buffer_unlock_commit(buffer, event);
> +		ftrace_stm_func(ip, parent_ip);
> +	}

So this logs instruction pointers, not the actual events. Not much is
duplicated like the message suggests, but it also doesn't seem very
useful.

Regards,
--
Alex

Re: [RFC PATCH 3/4] trace: Duplicate the output of the function trace logs to STM

[Chunyan Zhang]

On Tue, Jun 7, 2016 at 6:00 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> This patch adds an output from Ftrace to STM.
>
> But does it?
>
>> That being said,
>> Function trace messages would also be duplicated to STM buffer when
>> being stored into ring buffer.
>
> Not sure what you mean here. What's "STM buffer"?

Sorry if this is ambiguously expression, I mean the buffer which
stores the trace data from STM, such as ETB/TMC for CoreSight.

>
>>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  kernel/trace/trace.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 8a4bd6b..d613053 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -44,6 +44,7 @@
>>
>>  #include "trace.h"
>>  #include "trace_output.h"
>> +#include "trace_output_stm.h"
>>
>>  /*
>>   * On boot up, the ring buffer is set to the minimum size, so that
>> @@ -1884,8 +1885,10 @@ 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)) {
>>               __buffer_unlock_commit(buffer, event);
>> +             ftrace_stm_func(ip, parent_ip);
>> +     }
>
> So this logs instruction pointers, not the actual events. Not much is
> duplicated like the message suggests, but it also doesn't seem very
> useful.

Yes, only knowing function pointers may not be very useful, if doing
in this way is acceptable I intend to add more interfaces to export
more information to STM in the feature, I will appreciate if you can
tell me what information you think is _absolutely_ necessary.

Thanks,
Chunyan

>
> Regards,
> --
> Alex

[RFC PATCH 4/4] stm: Mark the functions of writing buffer with notrace

[Chunyan Zhang]

If CONFIG_STM_FTRACE is selected, Function trace data would be
writen to STM buffer, all  functions that related to writing data
packets to STM buffer 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>
---
 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 ff31108..3139b01 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -424,7 +424,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;
@@ -1069,8 +1069,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_ */
-- 
1.9.1

Re: [RFC PATCH 4/4] stm: Mark the functions of writing buffer with notrace

[Steven Rostedt]

On Wed,  1 Jun 2016 19:18:59 +0800
Chunyan Zhang <zhang.chunyan@linaro.org> wrote:

> If CONFIG_STM_FTRACE is selected, Function trace data would be
> writen to STM buffer, all  functions that related to writing data
> packets to STM buffer 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>
> ---
>  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(-)

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

Re: [RFC PATCH 0/4] Integration of function trace with System Trace IP blocks

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> This patchset is an RFC aimed at generating ideas on the best way to
> use STM IP blocks to collect function tracing information produced by
> Ftrace.  That way 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.

I'd say, traces are only useful if you can make sense of them. This
patchset basically sends out addresses, which only makes sense if the
decoding side has vmlinux of the kernel under tracing. But even if they
do, other context information is still missing, such as the cpu ids of
these events, without which you can't really tell what's been going
on. You can, of course, still use this data for coverage analysis, but
there are easier ways of doing that already.

So I'd say that first you need to export at least as much as is written
to the ftrace ring buffer. And perhaps, to avoid inventing yet another
binary protocol, it would have to be exactly what is written to the
ftrace ring buffer.

Then you need to think whether you want to export binary ftrace data or
ascii-formatted strings:
  * binary data is way less overhead;
  * ascii data is self-contained;
  * binary data requires the exact running kernel's binaries to decode
  and the ability to read them (say, if you wanted to read the traces on
  some other OS that doesn't have native support for ELF binaries);
  every time you recompile the target kernel, you'll have to copy it
  over to the debug host;
  * ascii data can be looked at independently.

That's off the top of my head.

Regards,
--
Alex

Re: [RFC PATCH 0/4] Integration of function trace with System Trace IP blocks

[Chunyan Zhang]

Hi Alex,

Thanks for your comments on every patch of this serial.

On Tue, Jun 7, 2016 at 6:50 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> This patchset is an RFC aimed at generating ideas on the best way to
>> use STM IP blocks to collect function tracing information produced by
>> Ftrace.  That way 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.
>
> I'd say, traces are only useful if you can make sense of them. This
> patchset basically sends out addresses, which only makes sense if the
> decoding side has vmlinux of the kernel under tracing. But even if they

I guess it shouldn't hard to be implemented.

> do, other context information is still missing, such as the cpu ids of

On ARM Platform, STM master IDs are related with CPU cores, so long as
we know which master the traces come from, we know the CPU id it
happened on.  Doesn't Intel STM have this kind of similar mechanism?

> these events, without which you can't really tell what's been going
> on. You can, of course, still use this data for coverage analysis, but
> there are easier ways of doing that already.
>
> So I'd say that first you need to export at least as much as is written
> to the ftrace ring buffer. And perhaps, to avoid inventing yet another
> binary protocol, it would have to be exactly what is written to the
> ftrace ring buffer.

In order to minimize overhead introduced by exporting message to STM,
we perhaps cannot record as much information as is exported to Ftrace
ring buffer.  The pointers of function and its parent function may not
be enough though, we can see what else is required.  But we may not
need such as timestamp, cpuid, since CoreSight has its own timestamp
and I guess Intel STM is the same;  About the cpuid for Intel STM, can
we have some way to know that, for example using different channel to
identify that?

>
> Then you need to think whether you want to export binary ftrace data or
> ascii-formatted strings:
>   * binary data is way less overhead;

Right, that's why I changed to this way, I sent out one patchset
before in which ascii-formatted strings was recorded to STM, but
adding that process introduced too much overhead to be accepted for
Ftrace subsystem, I dropped that solution.

>   * ascii data is self-contained;
>   * binary data requires the exact running kernel's binaries to decode
>   and the ability to read them (say, if you wanted to read the traces on
>   some other OS that doesn't have native support for ELF binaries);
>   every time you recompile the target kernel, you'll have to copy it
>   over to the debug host;

Yes. I think if we want to use this feature, we have to make vmlinux
as a part of the decoding library.

>   * ascii data can be looked at independently.

Above all are my superficial opinions, please correct me if I'm
missing something, and will appreciate if you can give more
suggestions/opinions.

Thanks,
Chunyan

>
> That's off the top of my head.
>
> Regards,
> --
> Alex