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

[Chunyan Zhang]

On Tue, Aug 9, 2016 at 11:35 PM, Steven Rostedt  wrote:
> On Tue,  9 Aug 2016 14:32:39 +0800
> Chunyan Zhang  wrote:
>
>> Currently ring buffer is the only 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 
>> ---
>>  include/linux/trace.h |  31 +
>>  kernel/trace/trace.c  | 124 
>> +-
>>  kernel/trace/trace.h  |  31 +
>>  3 files changed, 185 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 000..bc7f503
>> --- /dev/null
>> +++ b/include/linux/trace.h
>> @@ -0,0 +1,31 @@
>> +#ifndef _LINUX_TRACE_H
>> +#define _LINUX_TRACE_H
>> +
>> +#include 
>> +struct trace_array;
>> +
>> +#ifdef CONFIG_TRACING
>> +/*
>> + * The trace export - an export of function traces.  Every ftrace_ops
>> + * has at least one export which would output function traces to ring
>> + * buffer.
>> + *
>> + * tr- the trace_array this export belongs to
>> + * commit- commit the traces to ring buffer and/or some other places
>> + * write - copy traces which have been delt with ->commit() to
>> + * the destination
>> + */
>> +struct trace_export {
>> + char name[16];
>> + struct trace_export *next;
>
> Should document above name and next. What's name used for? Is it

Sure, I will document them in the next revision.

Speaking to the 'name' here... I just think it will probably be useful
in the future, for example, if we need an userspace interface for
users to decide which trace_export should be used.

> visible to userspace? Add "next" just to be consistent as that's pretty
> obvious what it is for.
>
>> + struct trace_array  *tr;
>> + void (*commit)(struct trace_array *, struct ring_buffer_event *);
>> + void (*write)(const char *, unsigned int);
>> +};
>> +
>> +int register_trace_export(struct trace_export *export);
>> +int unregister_trace_export(struct trace_export *export);
>> +
>> +#endif   /* CONFIG_TRACING */
>> +
>> +#endif   /* _LINUX_TRACE_H */
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index dade4c9..67ae581 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -40,6 +40,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>
>>  #include "trace.h"
>> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct 
>> trace_array *tr,
>>   ftrace_trace_userstack(buffer, flags, pc);
>>  }
>>
>> +static inline void
>> +trace_generic_commit(struct trace_array *tr,
>> +struct ring_buffer_event *event)
>> +{
>> + struct trace_entry *entry;
>> + struct trace_export *export = tr->export;
>> + unsigned int size = 0;
>> +
>> + entry = ring_buffer_event_data(event);
>> +
>> + trace_entry_size(size, entry->type);
>> + if (!size)
>> + return;
>> +
>> + if (export->write)
>> + export->write((char *)entry, size);
>> +}
>> +
>> +static inline void
>> +trace_rb_commit(struct trace_array *tr,
>> +struct ring_buffer_event *event)
>> +{
>> + __buffer_unlock_commit(tr->trace_buffer.buffer, event);
>> +}
>> +
>> +static DEFINE_MUTEX(trace_export_lock);
>> +
>> +static struct trace_export trace_export_rb __read_mostly = {
>> + .name   = "rb",
>> + .commit = trace_rb_commit,
>> + .next   = NULL,
>> +};
>> +static struct trace_export *trace_fn_exports __read_mostly = 
>> _export_rb;
>> +
>> +inline void
>> +trace_function_exports(struct trace_array *tr,
>> +struct ring_buffer_event *event)
>> +{
>> + struct trace_export *export;
>> +
>> + mutex_lock(_export_lock);
>
> Wait! Are you calling a mutex from the function tracer? This will blow
> up easily. The function callbacks must be totally lockless!

Okay, I just wanted to protect the list from being changed while being used.
What do you think if I change to make adding/removing trace exports
from the list are only permitted when the trace isn't enabled?

>
>> +
>> + for (export = trace_fn_exports; export && export->commit;
>> +  export = export->next) {
>> + tr->export = export;
>> + export->commit(tr, event);
>> + }
>> +
>> + 

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

[Chunyan Zhang]

On Tue, Aug 9, 2016 at 11:35 PM, Steven Rostedt  wrote:
> On Tue,  9 Aug 2016 14:32:39 +0800
> Chunyan Zhang  wrote:
>
>> Currently ring buffer is the only 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 
>> ---
>>  include/linux/trace.h |  31 +
>>  kernel/trace/trace.c  | 124 
>> +-
>>  kernel/trace/trace.h  |  31 +
>>  3 files changed, 185 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 000..bc7f503
>> --- /dev/null
>> +++ b/include/linux/trace.h
>> @@ -0,0 +1,31 @@
>> +#ifndef _LINUX_TRACE_H
>> +#define _LINUX_TRACE_H
>> +
>> +#include 
>> +struct trace_array;
>> +
>> +#ifdef CONFIG_TRACING
>> +/*
>> + * The trace export - an export of function traces.  Every ftrace_ops
>> + * has at least one export which would output function traces to ring
>> + * buffer.
>> + *
>> + * tr- the trace_array this export belongs to
>> + * commit- commit the traces to ring buffer and/or some other places
>> + * write - copy traces which have been delt with ->commit() to
>> + * the destination
>> + */
>> +struct trace_export {
>> + char name[16];
>> + struct trace_export *next;
>
> Should document above name and next. What's name used for? Is it

Sure, I will document them in the next revision.

Speaking to the 'name' here... I just think it will probably be useful
in the future, for example, if we need an userspace interface for
users to decide which trace_export should be used.

> visible to userspace? Add "next" just to be consistent as that's pretty
> obvious what it is for.
>
>> + struct trace_array  *tr;
>> + void (*commit)(struct trace_array *, struct ring_buffer_event *);
>> + void (*write)(const char *, unsigned int);
>> +};
>> +
>> +int register_trace_export(struct trace_export *export);
>> +int unregister_trace_export(struct trace_export *export);
>> +
>> +#endif   /* CONFIG_TRACING */
>> +
>> +#endif   /* _LINUX_TRACE_H */
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index dade4c9..67ae581 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -40,6 +40,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>
>>  #include "trace.h"
>> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct 
>> trace_array *tr,
>>   ftrace_trace_userstack(buffer, flags, pc);
>>  }
>>
>> +static inline void
>> +trace_generic_commit(struct trace_array *tr,
>> +struct ring_buffer_event *event)
>> +{
>> + struct trace_entry *entry;
>> + struct trace_export *export = tr->export;
>> + unsigned int size = 0;
>> +
>> + entry = ring_buffer_event_data(event);
>> +
>> + trace_entry_size(size, entry->type);
>> + if (!size)
>> + return;
>> +
>> + if (export->write)
>> + export->write((char *)entry, size);
>> +}
>> +
>> +static inline void
>> +trace_rb_commit(struct trace_array *tr,
>> +struct ring_buffer_event *event)
>> +{
>> + __buffer_unlock_commit(tr->trace_buffer.buffer, event);
>> +}
>> +
>> +static DEFINE_MUTEX(trace_export_lock);
>> +
>> +static struct trace_export trace_export_rb __read_mostly = {
>> + .name   = "rb",
>> + .commit = trace_rb_commit,
>> + .next   = NULL,
>> +};
>> +static struct trace_export *trace_fn_exports __read_mostly = 
>> _export_rb;
>> +
>> +inline void
>> +trace_function_exports(struct trace_array *tr,
>> +struct ring_buffer_event *event)
>> +{
>> + struct trace_export *export;
>> +
>> + mutex_lock(_export_lock);
>
> Wait! Are you calling a mutex from the function tracer? This will blow
> up easily. The function callbacks must be totally lockless!

Okay, I just wanted to protect the list from being changed while being used.
What do you think if I change to make adding/removing trace exports
from the list are only permitted when the trace isn't enabled?

>
>> +
>> + for (export = trace_fn_exports; export && export->commit;
>> +  export = export->next) {
>> + tr->export = export;
>> + export->commit(tr, event);
>> + }
>> +
>> + mutex_unlock(_export_lock);
>> +}
>> +
>> +static void add_trace_fn_export(struct trace_export 

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

[Steven Rostedt]

On Tue,  9 Aug 2016 14:32:39 +0800
Chunyan Zhang  wrote:

> Currently ring buffer is the only 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 
> ---
>  include/linux/trace.h |  31 +
>  kernel/trace/trace.c  | 124 
> +-
>  kernel/trace/trace.h  |  31 +
>  3 files changed, 185 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 000..bc7f503
> --- /dev/null
> +++ b/include/linux/trace.h
> @@ -0,0 +1,31 @@
> +#ifndef _LINUX_TRACE_H
> +#define _LINUX_TRACE_H
> +
> +#include 
> +struct trace_array;
> +
> +#ifdef CONFIG_TRACING
> +/*
> + * The trace export - an export of function traces.  Every ftrace_ops
> + * has at least one export which would output function traces to ring
> + * buffer.
> + *
> + * tr- the trace_array this export belongs to
> + * commit- commit the traces to ring buffer and/or some other places
> + * write - copy traces which have been delt with ->commit() to
> + * the destination
> + */
> +struct trace_export {
> + char name[16];
> + struct trace_export *next;

Should document above name and next. What's name used for? Is it
visible to userspace? Add "next" just to be consistent as that's pretty
obvious what it is for.

> + struct trace_array  *tr;
> + void (*commit)(struct trace_array *, struct ring_buffer_event *);
> + void (*write)(const char *, unsigned int);
> +};
> +
> +int register_trace_export(struct trace_export *export);
> +int unregister_trace_export(struct trace_export *export);
> +
> +#endif   /* CONFIG_TRACING */
> +
> +#endif   /* _LINUX_TRACE_H */
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index dade4c9..67ae581 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -40,6 +40,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "trace.h"
> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct 
> trace_array *tr,
>   ftrace_trace_userstack(buffer, flags, pc);
>  }
>  
> +static inline void
> +trace_generic_commit(struct trace_array *tr,
> +struct ring_buffer_event *event)
> +{
> + struct trace_entry *entry;
> + struct trace_export *export = tr->export;
> + unsigned int size = 0;
> +
> + entry = ring_buffer_event_data(event);
> +
> + trace_entry_size(size, entry->type);
> + if (!size)
> + return;
> +
> + if (export->write)
> + export->write((char *)entry, size);
> +}
> +
> +static inline void
> +trace_rb_commit(struct trace_array *tr,
> +struct ring_buffer_event *event)
> +{
> + __buffer_unlock_commit(tr->trace_buffer.buffer, event);
> +}
> +
> +static DEFINE_MUTEX(trace_export_lock);
> +
> +static struct trace_export trace_export_rb __read_mostly = {
> + .name   = "rb",
> + .commit = trace_rb_commit,
> + .next   = NULL,
> +};
> +static struct trace_export *trace_fn_exports __read_mostly = 
> _export_rb;
> +
> +inline void
> +trace_function_exports(struct trace_array *tr,
> +struct ring_buffer_event *event)
> +{
> + struct trace_export *export;
> +
> + mutex_lock(_export_lock);

Wait! Are you calling a mutex from the function tracer? This will blow
up easily. The function callbacks must be totally lockless!

> +
> + for (export = trace_fn_exports; export && export->commit;
> +  export = export->next) {
> + tr->export = export;
> + export->commit(tr, event);
> + }
> +
> + mutex_unlock(_export_lock);
> +}
> +
> +static void add_trace_fn_export(struct trace_export **list,
> +   struct trace_export *export)
> +{
> + export->next = *list;
> + /*
> +  * We are entering export into the list but another
> +  * CPU might be walking that list. We need to make sure
> +  * the export->next pointer is valid before another CPU sees
> +  * the export pointer included into the list.
> +  */
> + rcu_assign_pointer(*list, export);
> +
> +}
> +
> +static int rm_trace_fn_export(struct trace_export **list,
> +   struct trace_export *export)
> +{
> + struct trace_export **p;
> +
> + for 

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

[Steven Rostedt]

On Tue,  9 Aug 2016 14:32:39 +0800
Chunyan Zhang  wrote:

> Currently ring buffer is the only 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 
> ---
>  include/linux/trace.h |  31 +
>  kernel/trace/trace.c  | 124 
> +-
>  kernel/trace/trace.h  |  31 +
>  3 files changed, 185 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 000..bc7f503
> --- /dev/null
> +++ b/include/linux/trace.h
> @@ -0,0 +1,31 @@
> +#ifndef _LINUX_TRACE_H
> +#define _LINUX_TRACE_H
> +
> +#include 
> +struct trace_array;
> +
> +#ifdef CONFIG_TRACING
> +/*
> + * The trace export - an export of function traces.  Every ftrace_ops
> + * has at least one export which would output function traces to ring
> + * buffer.
> + *
> + * tr- the trace_array this export belongs to
> + * commit- commit the traces to ring buffer and/or some other places
> + * write - copy traces which have been delt with ->commit() to
> + * the destination
> + */
> +struct trace_export {
> + char name[16];
> + struct trace_export *next;

Should document above name and next. What's name used for? Is it
visible to userspace? Add "next" just to be consistent as that's pretty
obvious what it is for.

> + struct trace_array  *tr;
> + void (*commit)(struct trace_array *, struct ring_buffer_event *);
> + void (*write)(const char *, unsigned int);
> +};
> +
> +int register_trace_export(struct trace_export *export);
> +int unregister_trace_export(struct trace_export *export);
> +
> +#endif   /* CONFIG_TRACING */
> +
> +#endif   /* _LINUX_TRACE_H */
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index dade4c9..67ae581 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -40,6 +40,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "trace.h"
> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct 
> trace_array *tr,
>   ftrace_trace_userstack(buffer, flags, pc);
>  }
>  
> +static inline void
> +trace_generic_commit(struct trace_array *tr,
> +struct ring_buffer_event *event)
> +{
> + struct trace_entry *entry;
> + struct trace_export *export = tr->export;
> + unsigned int size = 0;
> +
> + entry = ring_buffer_event_data(event);
> +
> + trace_entry_size(size, entry->type);
> + if (!size)
> + return;
> +
> + if (export->write)
> + export->write((char *)entry, size);
> +}
> +
> +static inline void
> +trace_rb_commit(struct trace_array *tr,
> +struct ring_buffer_event *event)
> +{
> + __buffer_unlock_commit(tr->trace_buffer.buffer, event);
> +}
> +
> +static DEFINE_MUTEX(trace_export_lock);
> +
> +static struct trace_export trace_export_rb __read_mostly = {
> + .name   = "rb",
> + .commit = trace_rb_commit,
> + .next   = NULL,
> +};
> +static struct trace_export *trace_fn_exports __read_mostly = 
> _export_rb;
> +
> +inline void
> +trace_function_exports(struct trace_array *tr,
> +struct ring_buffer_event *event)
> +{
> + struct trace_export *export;
> +
> + mutex_lock(_export_lock);

Wait! Are you calling a mutex from the function tracer? This will blow
up easily. The function callbacks must be totally lockless!

> +
> + for (export = trace_fn_exports; export && export->commit;
> +  export = export->next) {
> + tr->export = export;
> + export->commit(tr, event);
> + }
> +
> + mutex_unlock(_export_lock);
> +}
> +
> +static void add_trace_fn_export(struct trace_export **list,
> +   struct trace_export *export)
> +{
> + export->next = *list;
> + /*
> +  * We are entering export into the list but another
> +  * CPU might be walking that list. We need to make sure
> +  * the export->next pointer is valid before another CPU sees
> +  * the export pointer included into the list.
> +  */
> + rcu_assign_pointer(*list, export);
> +
> +}
> +
> +static int rm_trace_fn_export(struct trace_export **list,
> +   struct trace_export *export)
> +{
> + struct trace_export **p;
> +
> + for (p = list; *p != _export_rb; p = &(*p)->next)
> +

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

[Chunyan Zhang]

Removing maxime.coque...@st.com since it seems an unreachable address

On Tue, Aug 9, 2016 at 2:32 PM, Chunyan Zhang  wrote:
> Currently ring buffer is the only 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 
> ---
>  include/linux/trace.h |  31 +
>  kernel/trace/trace.c  | 124 
> +-
>  kernel/trace/trace.h  |  31 +
>  3 files changed, 185 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 000..bc7f503
> --- /dev/null
> +++ b/include/linux/trace.h
> @@ -0,0 +1,31 @@
> +#ifndef _LINUX_TRACE_H
> +#define _LINUX_TRACE_H
> +
> +#include 
> +struct trace_array;
> +
> +#ifdef CONFIG_TRACING
> +/*
> + * The trace export - an export of function traces.  Every ftrace_ops
> + * has at least one export which would output function traces to ring
> + * buffer.
> + *
> + * tr  - the trace_array this export belongs to
> + * commit  - commit the traces to ring buffer and/or some other places
> + * write   - copy traces which have been delt with ->commit() to
> + *   the destination
> + */
> +struct trace_export {
> +   char name[16];
> +   struct trace_export *next;
> +   struct trace_array  *tr;
> +   void (*commit)(struct trace_array *, struct ring_buffer_event *);
> +   void (*write)(const char *, unsigned int);
> +};
> +
> +int register_trace_export(struct trace_export *export);
> +int unregister_trace_export(struct trace_export *export);
> +
> +#endif /* CONFIG_TRACING */
> +
> +#endif /* _LINUX_TRACE_H */
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index dade4c9..67ae581 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -40,6 +40,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include "trace.h"
> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct 
> trace_array *tr,
> ftrace_trace_userstack(buffer, flags, pc);
>  }
>
> +static inline void
> +trace_generic_commit(struct trace_array *tr,
> +  struct ring_buffer_event *event)
> +{
> +   struct trace_entry *entry;
> +   struct trace_export *export = tr->export;
> +   unsigned int size = 0;
> +
> +   entry = ring_buffer_event_data(event);
> +
> +   trace_entry_size(size, entry->type);
> +   if (!size)
> +   return;
> +
> +   if (export->write)
> +   export->write((char *)entry, size);
> +}
> +
> +static inline void
> +trace_rb_commit(struct trace_array *tr,
> +  struct ring_buffer_event *event)
> +{
> +   __buffer_unlock_commit(tr->trace_buffer.buffer, event);
> +}
> +
> +static DEFINE_MUTEX(trace_export_lock);
> +
> +static struct trace_export trace_export_rb __read_mostly = {
> +   .name   = "rb",
> +   .commit = trace_rb_commit,
> +   .next   = NULL,
> +};
> +static struct trace_export *trace_fn_exports __read_mostly = 
> _export_rb;
> +
> +inline void
> +trace_function_exports(struct trace_array *tr,
> +  struct ring_buffer_event *event)
> +{
> +   struct trace_export *export;
> +
> +   mutex_lock(_export_lock);
> +
> +   for (export = trace_fn_exports; export && export->commit;
> +export = export->next) {
> +   tr->export = export;
> +   export->commit(tr, event);
> +   }
> +
> +   mutex_unlock(_export_lock);
> +}
> +
> +static void add_trace_fn_export(struct trace_export **list,
> + struct trace_export *export)
> +{
> +   export->next = *list;
> +   /*
> +* We are entering export into the list but another
> +* CPU might be walking that list. We need to make sure
> +* the export->next pointer is valid before another CPU sees
> +* the export pointer included into the list.
> +*/
> +   rcu_assign_pointer(*list, export);
> +
> +}
> +
> +static int rm_trace_fn_export(struct trace_export **list,
> + struct trace_export *export)
> +{
> +   struct trace_export **p;
> +
> +   for (p = list; *p != _export_rb; p = &(*p)->next)
> +   if (*p == export)
> +   break;
> +
> +   if (*p != export)
> +   

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

[Chunyan Zhang]

Removing maxime.coque...@st.com since it seems an unreachable address

On Tue, Aug 9, 2016 at 2:32 PM, Chunyan Zhang  wrote:
> Currently ring buffer is the only 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 
> ---
>  include/linux/trace.h |  31 +
>  kernel/trace/trace.c  | 124 
> +-
>  kernel/trace/trace.h  |  31 +
>  3 files changed, 185 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 000..bc7f503
> --- /dev/null
> +++ b/include/linux/trace.h
> @@ -0,0 +1,31 @@
> +#ifndef _LINUX_TRACE_H
> +#define _LINUX_TRACE_H
> +
> +#include 
> +struct trace_array;
> +
> +#ifdef CONFIG_TRACING
> +/*
> + * The trace export - an export of function traces.  Every ftrace_ops
> + * has at least one export which would output function traces to ring
> + * buffer.
> + *
> + * tr  - the trace_array this export belongs to
> + * commit  - commit the traces to ring buffer and/or some other places
> + * write   - copy traces which have been delt with ->commit() to
> + *   the destination
> + */
> +struct trace_export {
> +   char name[16];
> +   struct trace_export *next;
> +   struct trace_array  *tr;
> +   void (*commit)(struct trace_array *, struct ring_buffer_event *);
> +   void (*write)(const char *, unsigned int);
> +};
> +
> +int register_trace_export(struct trace_export *export);
> +int unregister_trace_export(struct trace_export *export);
> +
> +#endif /* CONFIG_TRACING */
> +
> +#endif /* _LINUX_TRACE_H */
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index dade4c9..67ae581 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -40,6 +40,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include "trace.h"
> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct 
> trace_array *tr,
> ftrace_trace_userstack(buffer, flags, pc);
>  }
>
> +static inline void
> +trace_generic_commit(struct trace_array *tr,
> +  struct ring_buffer_event *event)
> +{
> +   struct trace_entry *entry;
> +   struct trace_export *export = tr->export;
> +   unsigned int size = 0;
> +
> +   entry = ring_buffer_event_data(event);
> +
> +   trace_entry_size(size, entry->type);
> +   if (!size)
> +   return;
> +
> +   if (export->write)
> +   export->write((char *)entry, size);
> +}
> +
> +static inline void
> +trace_rb_commit(struct trace_array *tr,
> +  struct ring_buffer_event *event)
> +{
> +   __buffer_unlock_commit(tr->trace_buffer.buffer, event);
> +}
> +
> +static DEFINE_MUTEX(trace_export_lock);
> +
> +static struct trace_export trace_export_rb __read_mostly = {
> +   .name   = "rb",
> +   .commit = trace_rb_commit,
> +   .next   = NULL,
> +};
> +static struct trace_export *trace_fn_exports __read_mostly = 
> _export_rb;
> +
> +inline void
> +trace_function_exports(struct trace_array *tr,
> +  struct ring_buffer_event *event)
> +{
> +   struct trace_export *export;
> +
> +   mutex_lock(_export_lock);
> +
> +   for (export = trace_fn_exports; export && export->commit;
> +export = export->next) {
> +   tr->export = export;
> +   export->commit(tr, event);
> +   }
> +
> +   mutex_unlock(_export_lock);
> +}
> +
> +static void add_trace_fn_export(struct trace_export **list,
> + struct trace_export *export)
> +{
> +   export->next = *list;
> +   /*
> +* We are entering export into the list but another
> +* CPU might be walking that list. We need to make sure
> +* the export->next pointer is valid before another CPU sees
> +* the export pointer included into the list.
> +*/
> +   rcu_assign_pointer(*list, export);
> +
> +}
> +
> +static int rm_trace_fn_export(struct trace_export **list,
> + struct trace_export *export)
> +{
> +   struct trace_export **p;
> +
> +   for (p = list; *p != _export_rb; p = &(*p)->next)
> +   if (*p == export)
> +   break;
> +
> +   if (*p != export)
> +   return -1;
> +
> +   *p = (*p)->next;
> +
> +