IP blocks allowing a variety of trace sources to log debugging
information to a pre-defined area have been introduced on a couple of
architecture [1][2]. These system trace blocks (also known as STM)
typically follow the MIPI STPv2 protocol [3] and provide a system wide
logging facility to any device, running a kernel or not, with access
to the block's log entry port(s). Since each trace message has a
timestamp, it is possible to correlate events happening in the entire
system rather than being confined to the logging facility of a single
entity.
This patchset is trying to use STM IP blocks to store some function
tracing information produced by Ftrace and I'm taking the Function trace
(TRACE_FN) as the example in this patchset, but other types of traces
also can be supported.
Logging information generated by the function trace subsystem
and gathered in the coresight sink can be used in conjunction with
trace data from other board components, also collected in the same
trace sink. This example is using ARM coresight STM but the same would
apply to any architecture wishing to do the same.
Comments and advice would be greatly appreciated.
Thanks,
Chunyan
[1]. https://lwn.net/Articles/674746/
[2]. http://lxr.free-electrons.com/source/drivers/hwtracing/intel_th/
[3]. http://mipi.org/specifications/debug#STP
Changes from v3:
* Addressed comments from Steven Rostedt:
- Added comments for the element 'name' and 'next' of trace_export;
- Changed to use RCU functions instead of mutex in function callback;
- Moved the check for name to register trace_export function;
* Renamed 'trace_function_exports' to 'trace_exports' since its
implementation is not only for function_trace; The similar changes on
'add_trace_export' and 'rm_trace_export'.
Changes from v2:
* Rebased on v4.8-rc1.
* Added trace_export class, and make traces can be exported to not only
ring buffer but also other area such as STM.
* Made stm_ftrace as an trace_export.
* More detailed changes are described in change log of each patch.
Changes from v1:
* Addressed comments from Alexander Shishkin:
- Modified some ambiguous change logs.
- Decoupled stm_ftrace and trace_output interface to STM.
- Changed the file name from stm_ftrace.c to stm/ftrace.c.
- Implemented link/unlink hooks for stm_ftrace.
* Removed useless header file include from stm/ftrace.c
* Added Acked-by from Steven Rostedt on 4/4.
Chunyan Zhang (3):
tracing: add a possibility of exporting function trace to other places
instead of ring buffer only
stm class: ftrace: Add ftrace-export-over-stm driver
stm: Mark the functions of writing buffer with notrace
drivers/hwtracing/coresight/coresight-stm.c | 2 +-
drivers/hwtracing/intel_th/sth.c | 11 ++-
drivers/hwtracing/stm/Kconfig | 11 +++
drivers/hwtracing/stm/Makefile | 2 +
drivers/hwtracing/stm/core.c | 7 +-
drivers/hwtracing/stm/dummy_stm.c | 2 +-
drivers/hwtracing/stm/ftrace.c | 87 +++++++++++++++++++
include/linux/stm.h | 4 +-
include/linux/trace.h | 33 ++++++++
kernel/trace/trace.c | 124 +++++++++++++++++++++++++++-
kernel/trace/trace.h | 31 +++++++
11 files changed, 302 insertions(+), 12 deletions(-)
create mode 100644 drivers/hwtracing/stm/ftrace.c
create mode 100644 include/linux/trace.h
--
2.7.4
Currently ring buffer is the only one output of Function traces, this patch
added trace_export concept which would process the traces and export
traces to a registered destination which can be ring buffer or some other
storage, in this way if we want Function traces to be sent to other
destination rather than ring buffer only, we just need to register a new
trace_export and implement its own .commit() callback or just use
'trace_generic_commit()' which this patch also added and hooks up its
own .write() functio for writing traces to the storage.
Currently, only Function trace (TRACE_FN) is supported.
Signed-off-by: Chunyan Zhang <redacted>
---
include/linux/trace.h | 33 ++++++++++++++
kernel/trace/trace.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++-
kernel/trace/trace.h | 31 +++++++++++++
3 files changed, 187 insertions(+), 1 deletion(-)
create mode 100644 include/linux/trace.h
@@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,ftrace_trace_userstack(buffer,flags,pc);}+staticinlinevoid+trace_generic_commit(structtrace_array*tr,+structring_buffer_event*event)+{+structtrace_entry*entry;+structtrace_export*export=tr->export;+unsignedintsize=0;++entry=ring_buffer_event_data(event);++trace_entry_size(size,entry->type);+if(!size)+return;++if(export->write)+export->write((char*)entry,size);+}++staticinlinevoid+trace_rb_commit(structtrace_array*tr,+structring_buffer_event*event)+{+__buffer_unlock_commit(tr->trace_buffer.buffer,event);+}++staticDEFINE_MUTEX(trace_export_lock);++staticstructtrace_exporttrace_export_rb__read_mostly={+.name="rb",+.commit=trace_rb_commit,+.next=NULL,+};+staticstructtrace_export*trace_exports_list__read_mostly=&trace_export_rb;++inlinevoid+trace_exports(structtrace_array*tr,structring_buffer_event*event)+{+structtrace_export*export;++preempt_disable_notrace();++for(export=rcu_dereference_raw_notrace(trace_exports_list);+export&&export->commit;+export=rcu_dereference_raw_notrace(export->next)){+tr->export=export;+export->commit(tr,event);+}++preempt_enable_notrace();+}++staticvoid+add_trace_export(structtrace_export**list,structtrace_export*export)+{+export->next=*list;+/*+*Weareenteringexportintothelistbutanother+*CPUmightbewalkingthatlist.Weneedtomakesure+*theexport->nextpointerisvalidbeforeanotherCPUsees+*theexportpointerincludedintothelist.+*/+rcu_assign_pointer(*list,export);++}++staticint+rm_trace_export(structtrace_export**list,structtrace_export*export)+{+structtrace_export**p;++for(p=list;*p!=&trace_export_rb;p=&(*p)->next)+if(*p==export)+break;++if(*p!=export)+return-1;++*p=(*p)->next;++return0;+}++intregister_trace_export(structtrace_export*export)+{+if(!export->write){+pr_warn("trace_export must have the write() call back.\n");+return-1;+}++if(!export->name){+pr_warn("trace_export must have a name.\n");+return-1;+}++mutex_lock(&trace_export_lock);++export->tr=trace_exports_list->tr;+export->commit=trace_generic_commit;++add_trace_export(&trace_exports_list,export);++mutex_unlock(&trace_export_lock);++return0;+}+EXPORT_SYMBOL_GPL(register_trace_export);++intunregister_trace_export(structtrace_export*export)+{+intret;++mutex_lock(&trace_export_lock);++ret=rm_trace_export(&trace_exports_list,export);++mutex_unlock(&trace_export_lock);++returnret;+}+EXPORT_SYMBOL_GPL(unregister_trace_export);+voidtrace_function(structtrace_array*tr,unsignedlongip,unsignedlongparent_ip,unsignedlongflags,
@@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void)break;\}+#undef IF_SIZE+#define IF_SIZE(size, var, etype, id) \+if(var==id){\+size=(sizeof(etype));\+break;\+}+/* Will cause compile errors if type is not found. */externvoid__ftrace_bad_type(void);
It may be better to use the latest version of sparse, i.e.
git clone git://git.kernel.org/pub/scm/devel/sparse/sparse.git
and then build the source code according to the direction on the page
https://kernelnewbies.org/Sparse
Otherwise, would get the errors like below if I install sparse using
apt-get install.
-------------------------------------------------------
CHECK arch/x86/purgatory/purgatory.c
No such file: asan-stack=1
scripts/Makefile.build:289: recipe for target
'arch/x86/purgatory/purgatory.o' failed
make[1]: *** [arch/x86/purgatory/purgatory.o] Error 1
arch/x86/Makefile:195: recipe for target 'archprepare' failed
make: *** [archprepare] Error 2
-------------------------------------------------------
Thanks,
Chunyan
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
include/linux/compiler.h:230:8: sparse: attribute 'no_sanitize_address': unknown attribute
quoted
quoted
kernel/trace/trace.c:2173:23: sparse: incompatible types in comparison expression (different address spaces)
As export->next will most likely need to be marked __rcu as well, this
may need an rcu_assign_pointer() too.
+ /*
+ * We are entering export into the list but another
+ * CPU might be walking that list. We need to make sure
+ * the export->next pointer is valid before another CPU sees
+ * the export pointer included into the list.
+ */
+ rcu_assign_pointer(*list, export);
+
+}
+
+static int
+rm_trace_export(struct trace_export **list, struct trace_export *export)
+{
+ struct trace_export **p;
+
+ for (p = list; *p != &trace_export_rb; p = &(*p)->next)
+ if (*p == export)
+ break;
+
+ if (*p != export)
+ return -1;
+
+ *p = (*p)->next;
I believe this requires an rcu_assign_pointer() as well.
+
+ return 0;
+}
+
+int register_trace_export(struct trace_export *export)
+{
+ if (!export->write) {
+ pr_warn("trace_export must have the write() call back.\n");
Probably should be a "WARN_ON_ONCE()".
+ return -1;
+ }
+
+ if (!export->name) {
+ pr_warn("trace_export must have a name.\n");
+ return -1;
+ }
If name isn't used don't add it till it is. That is, this patch should
not have "name" in the structure. It's confusing, because I don't see a
purpose for it.
How do you handle the discard? If the entry doesn't match the filter,
it will try to discard the event. I don't see how the "trace_exports"
handle that.
@@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void)break;\}+#undef IF_SIZE+#define IF_SIZE(size, var, etype, id) \+if(var==id){\+size=(sizeof(etype));\+break;\+}+/* Will cause compile errors if type is not found. */externvoid__ftrace_bad_type(void);
I really really dislike this. This is a big if statement that all
needs to go down one by one. Very inefficient!
-- Steve
+ } while (0)
+
+/*
* An option specific to a tracer. This is a boolean value.
* The bit is the bit index that sets its value on the
* flags value in struct tracer_flags.
As export->next will most likely need to be marked __rcu as well, this
may need an rcu_assign_pointer() too.
quoted
+ /*
+ * We are entering export into the list but another
+ * CPU might be walking that list. We need to make sure
+ * the export->next pointer is valid before another CPU sees
+ * the export pointer included into the list.
+ */
+ rcu_assign_pointer(*list, export);
+
+}
+
+static int
+rm_trace_export(struct trace_export **list, struct trace_export *export)
+{
+ struct trace_export **p;
+
+ for (p = list; *p != &trace_export_rb; p = &(*p)->next)
+ if (*p == export)
+ break;
+
+ if (*p != export)
+ return -1;
+
+ *p = (*p)->next;
I believe this requires an rcu_assign_pointer() as well.
quoted
+
+ return 0;
+}
+
+int register_trace_export(struct trace_export *export)
+{
+ if (!export->write) {
+ pr_warn("trace_export must have the write() call back.\n");
Probably should be a "WARN_ON_ONCE()".
Yes, will revise.
quoted
+ return -1;
+ }
+
+ if (!export->name) {
+ pr_warn("trace_export must have a name.\n");
+ return -1;
+ }
If name isn't used don't add it till it is. That is, this patch should
not have "name" in the structure. It's confusing, because I don't see a
purpose for it.
Shouldn't the caller pass in the commit function too?
The trace_export::write() callback is for caller, commit function
mainly deal with traces, is it better to keep 'trace_generic_commit'
in trace.c, i.e don't expose it to callers?
How do you handle the discard? If the entry doesn't match the filter,
it will try to discard the event. I don't see how the "trace_exports"
handle that.
I directly used the entries which had already been filtered.
Humm.. sorry, actually you lost me here.
@@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void)break;\}+#undef IF_SIZE+#define IF_SIZE(size, var, etype, id) \+if(var==id){\+size=(sizeof(etype));\+break;\+}+/* Will cause compile errors if type is not found. */externvoid__ftrace_bad_type(void);
I really really dislike this. This is a big if statement that all
needs to go down one by one. Very inefficient!
Ok, will change to other implementation.
Thanks for your comments,
Chunyan
-- Steve
quoted
+ } while (0)
+
+/*
* An option specific to a tracer. This is a boolean value.
* The bit is the bit index that sets its value on the
* flags value in struct tracer_flags.
Shouldn't the caller pass in the commit function too?
The trace_export::write() callback is for caller, commit function
mainly deal with traces, is it better to keep 'trace_generic_commit'
in trace.c, i.e don't expose it to callers?
It's fine to be external if it's only declared in kernel/trace/trace.h.
I would think anything using a different "write" would require a
different "commit".
But maybe I'm misunderstanding your objective. See below.
How do you handle the discard? If the entry doesn't match the filter,
it will try to discard the event. I don't see how the "trace_exports"
handle that.
I directly used the entries which had already been filtered.
Humm.. sorry, actually you lost me here.
I'm assuming this entire patch is to have the events written to
something other than the ftrace ring buffer, correct?
Or is this just trying to hook into the tracing that is happening? That
is, this isn't replacing writing into the ftrace ring buffer, but it is
just adding a way to write to someplace in addition to the ftrace ring
buffer. Where you still write to the ftrace ring buffer, but then you
can add a hook to copy someplace else as well.
I was looking at this as a way that you are adding a replacement, not
only an addition to. If that's the case, I think there may be a easier
way to do this.
-- Steve
Shouldn't the caller pass in the commit function too?
The trace_export::write() callback is for caller, commit function
mainly deal with traces, is it better to keep 'trace_generic_commit'
in trace.c, i.e don't expose it to callers?
It's fine to be external if it's only declared in kernel/trace/trace.h.
But we cannot assume that the trace_exports all are under
kernel/trace/, for example in this patchset 2/2, 'stm_ftrace' as a
trace_export was registered in stm subsystem but during its
initialization it cannot access 'trace_generic_commit' which is under
kernel/trace.
I would think anything using a different "write" would require a
different "commit".
Ok, I should make it more feasible rather than pointing to
'trace_generic_commit' directly when registering trace_export, that's
say, if the trace_export doesn't have its own commit function, point
directly to 'trace_generic_commit'.
But maybe I'm misunderstanding your objective. See below.
How do you handle the discard? If the entry doesn't match the filter,
it will try to discard the event. I don't see how the "trace_exports"
handle that.
I directly used the entries which had already been filtered.
Humm.. sorry, actually you lost me here.
I'm assuming this entire patch is to have the events written to
something other than the ftrace ring buffer, correct?
Or is this just trying to hook into the tracing that is happening? That
is, this isn't replacing writing into the ftrace ring buffer, but it is
just adding a way to write to someplace in addition to the ftrace ring
buffer. Where you still write to the ftrace ring buffer, but then you
can add a hook to copy someplace else as well.
Yes, this is what this patch is trying to implement.
I was looking at this as a way that you are adding a replacement, not
only an addition to. If that's the case, I think there may be a easier
way to do this.
I want to know how it would be in the easier way you mentioned here.
I was trying to add a ftrace_ops before, but with that way, I have to
deal with a lot of trace or ring buffer stuff including the sort of
discard things like you mentioned, which the existed ftrace code does.
And if I choose to implement a new ftrace_ops, I'm only able to get
the function trace support for STM and have to do many things which
would be overlap with the current ftrace subsystem.
So in order to reuse the existed code and architecture, I chose to add
a trace_export interface for Ftrace subsytem, and in this way I'm
using in this patch, I will get all supports of traces which are dealt
with trace_function();
Another benefit of adding a trace_export is, if there will be other
subsystem would like to use the processed traces, it only needs to
register a trace_export and provides a .write() function call back or
together with a commit function, although from what I can see now
.write() is enough since my purpose was the processed traces I don't
need 'ring_buffer_event' so long as I had trace entries.
Thanks for your comments,
Chunyan
From: Steven Rostedt <rostedt@goodmis.org> Date: 2016-08-19 00:53:30
On Thu, 18 Aug 2016 17:22:11 +0800
Chunyan Zhang [off-list ref] wrote:
quoted
Or is this just trying to hook into the tracing that is happening? That
is, this isn't replacing writing into the ftrace ring buffer, but it is
just adding a way to write to someplace in addition to the ftrace ring
buffer. Where you still write to the ftrace ring buffer, but then you
can add a hook to copy someplace else as well.
Yes, this is what this patch is trying to implement.
quoted
I was looking at this as a way that you are adding a replacement, not
only an addition to. If that's the case, I think there may be a easier
way to do this.
I want to know how it would be in the easier way you mentioned here.
I was trying to add a ftrace_ops before, but with that way, I have to
deal with a lot of trace or ring buffer stuff including the sort of
discard things like you mentioned, which the existed ftrace code does.
And if I choose to implement a new ftrace_ops, I'm only able to get
the function trace support for STM and have to do many things which
would be overlap with the current ftrace subsystem.
Adding your own ftrace_ops is a way for replacing, not just adding a
hook into.
So in order to reuse the existed code and architecture, I chose to add
a trace_export interface for Ftrace subsytem, and in this way I'm
using in this patch, I will get all supports of traces which are dealt
with trace_function();
Actually, a trace_export() should only be called if there's been
something added. And that should be done with a static_key_false()
branch (which is dynamically enabled, and does not use a comparison
branch).
That is, something like this instead:
if (!call_filter_check_discard(call, entry, buffer, event)) {
if (static_key_false(&ftrace_trace_exports_enabled))
ftrace_exports(tr, event);
__buffer_unlock_commit(buffer, event);
}
Don't touch the current logic. Just have your code hook into the
ftrace_exports (note I use "ftrace_exports" and not trace_exports()
because it's the function tracer, which has stricter requirements than
events do. If you add a hook for tracepoints later, use trace_exports()
and have a different list for that).
Another benefit of adding a trace_export is, if there will be other
subsystem would like to use the processed traces, it only needs to
register a trace_export and provides a .write() function call back or
together with a commit function, although from what I can see now
.write() is enough since my purpose was the processed traces I don't
need 'ring_buffer_event' so long as I had trace entries.
I'm saying if you don't mind the ring buffer being used along with
your own code (which seems to be what's happening), then just add a
call back to your code. Don't monkey with the current logic.
I think that will simplify things tremendously.
-- Steve
On 19 August 2016 at 00:12, Steven Rostedt [off-list ref] wrote:
On Thu, 18 Aug 2016 17:22:11 +0800
Chunyan Zhang [off-list ref] wrote:
quoted
quoted
Or is this just trying to hook into the tracing that is happening? That
is, this isn't replacing writing into the ftrace ring buffer, but it is
just adding a way to write to someplace in addition to the ftrace ring
buffer. Where you still write to the ftrace ring buffer, but then you
can add a hook to copy someplace else as well.
Yes, this is what this patch is trying to implement.
quoted
I was looking at this as a way that you are adding a replacement, not
only an addition to. If that's the case, I think there may be a easier
way to do this.
I want to know how it would be in the easier way you mentioned here.
I was trying to add a ftrace_ops before, but with that way, I have to
deal with a lot of trace or ring buffer stuff including the sort of
discard things like you mentioned, which the existed ftrace code does.
And if I choose to implement a new ftrace_ops, I'm only able to get
the function trace support for STM and have to do many things which
would be overlap with the current ftrace subsystem.
Adding your own ftrace_ops is a way for replacing, not just adding a
hook into.
quoted
So in order to reuse the existed code and architecture, I chose to add
a trace_export interface for Ftrace subsytem, and in this way I'm
using in this patch, I will get all supports of traces which are dealt
with trace_function();
Actually, a trace_export() should only be called if there's been
something added. And that should be done with a static_key_false()
branch (which is dynamically enabled, and does not use a comparison
branch).
That is, something like this instead:
if (!call_filter_check_discard(call, entry, buffer, event)) {
if (static_key_false(&ftrace_trace_exports_enabled))
ftrace_exports(tr, event);
__buffer_unlock_commit(buffer, event);
}
Thanks for the sample code, I got it, will do like this.
Don't touch the current logic. Just have your code hook into the
ftrace_exports (note I use "ftrace_exports" and not trace_exports()
because it's the function tracer, which has stricter requirements than
events do. If you add a hook for tracepoints later, use trace_exports()
and have a different list for that).
quoted
Another benefit of adding a trace_export is, if there will be other
subsystem would like to use the processed traces, it only needs to
register a trace_export and provides a .write() function call back or
together with a commit function, although from what I can see now
.write() is enough since my purpose was the processed traces I don't
need 'ring_buffer_event' so long as I had trace entries.
I'm saying if you don't mind the ring buffer being used along with
your own code (which seems to be what's happening), then just add a
call back to your code. Don't monkey with the current logic.
I think that will simplify things tremendously.
Thanks for your comments and detailed explanation,
Chunyan
This patch adds a driver that models itself as an stm_source and
registers itself as a trace_export. Once the stm and stm_source
have been linked via sysfs, everything that is passed to the
interface from Ftrace subsystem will endup in the STM trace engine.
Signed-off-by: Chunyan Zhang <redacted>
---
drivers/hwtracing/stm/Kconfig | 11 ++++++
drivers/hwtracing/stm/Makefile | 2 +
drivers/hwtracing/stm/ftrace.c | 87 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 100 insertions(+)
create mode 100644 drivers/hwtracing/stm/ftrace.c
@@ -39,4 +39,15 @@ config STM_SOURCE_HEARTBEATIfyouwanttosendheartbeatmessagesoverSTMdevices,sayY.+configSTM_SOURCE_FTRACE+tristate"Copy the output from kernel Ftrace to STM engine"+depends onTRACING+help+ThisoptioncanbeusedtocopytheoutputfromkernelFtrace+toSTMengine.Enablingthisoptionwillintroduceaslight+timingeffect.++IfyouwanttosendkernelFtracemessagesoverSTMdevices,+sayY.+endif
If CONFIG_STM_FTRACE is selected, Function trace data can be writen
to sink via STM, all functions that related to writing data packets
to STM should be marked 'notrace' to avoid being traced by Ftrace,
otherwise the program would stall into an endless loop.
Signed-off-by: Chunyan Zhang <redacted>
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(-)