| Message ID | 1471261803-1186-2-git-send-email-zhang.chunyan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Chunyan,
[auto build test WARNING on linus/master]
[also build test WARNING on v4.8-rc2 next-20160815]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chunyan-Zhang/tracing-add-a-possibility-of-exporting-function-trace-to-other-places-instead-of-ring-buffer-only/20160815-195631
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
include/linux/compiler.h:230:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> kernel/trace/trace.c:2173:23: sparse: incompatible types in comparison expression (different address spaces)
kernel/trace/trace.c:2175:23: sparse: incompatible types in comparison expression (different address spaces)
vim +2173 kernel/trace/trace.c
2157 static DEFINE_MUTEX(trace_export_lock);
2158
2159 static struct trace_export trace_export_rb __read_mostly = {
2160 .name = "rb",
2161 .commit = trace_rb_commit,
2162 .next = NULL,
2163 };
2164 static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb;
2165
2166 inline void
2167 trace_exports(struct trace_array *tr, struct ring_buffer_event *event)
2168 {
2169 struct trace_export *export;
2170
2171 preempt_disable_notrace();
2172
> 2173 for (export = rcu_dereference_raw_notrace(trace_exports_list);
2174 export && export->commit;
2175 export = rcu_dereference_raw_notrace(export->next)) {
2176 tr->export = export;
2177 export->commit(tr, event);
2178 }
2179
2180 preempt_enable_notrace();
2181 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, 15 Aug 2016 19:50:01 +0800 Chunyan Zhang <zhang.chunyan@linaro.org> wrote: > +int register_trace_export(struct trace_export *export); > +int unregister_trace_export(struct trace_export *export); > + > +#endif /* CONFIG_TRACING */ > + > +#endif /* _LINUX_TRACE_H */ > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index dade4c9..0247ac2 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -40,6 +40,7 @@ > #include <linux/poll.h> > #include <linux/nmi.h> > #include <linux/fs.h> > +#include <linux/trace.h> > #include <linux/sched/rt.h> > > #include "trace.h" > @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr, > ftrace_trace_userstack(buffer, flags, pc); > } > > +static inline void > +trace_generic_commit(struct trace_array *tr, > + struct ring_buffer_event *event) Why is this marked inline? It is never called directly here. > +{ > + struct trace_entry *entry; > + struct trace_export *export = tr->export; > + unsigned int size = 0; > + > + entry = ring_buffer_event_data(event); > + > + trace_entry_size(size, entry->type); > + if (!size) > + return; > + > + if (export->write) > + export->write((char *)entry, size); > +} > + > +static inline void Same here. > +trace_rb_commit(struct trace_array *tr, > + struct ring_buffer_event *event) > +{ > + __buffer_unlock_commit(tr->trace_buffer.buffer, event); > +} > + > +static DEFINE_MUTEX(trace_export_lock); > + > +static struct trace_export trace_export_rb __read_mostly = { > + .name = "rb", > + .commit = trace_rb_commit, Make sure equal signs are lined up. > + .next = NULL, > +}; > +static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb; trace_exports_list needs to be annotated as rcu, if you are going to dereference it via rcu_dereference. That was the kbuild warning you received. > + > +inline void > +trace_exports(struct trace_array *tr, struct ring_buffer_event *event) > +{ > + struct trace_export *export; > + > + preempt_disable_notrace(); > + > + for (export = rcu_dereference_raw_notrace(trace_exports_list); > + export && export->commit; > + export = rcu_dereference_raw_notrace(export->next)) { > + tr->export = export; > + export->commit(tr, event); > + } > + > + preempt_enable_notrace(); > +} I'm not too happy about the added overhead to normal function tracing (it will be noticeable), but I can fix that later. > + > +static void > +add_trace_export(struct trace_export **list, struct trace_export *export) > +{ > + export->next = *list; As export->next will most likely need to be marked __rcu as well, this may need an rcu_assign_pointer() too. > + /* > + * We are entering export into the list but another > + * CPU might be walking that list. We need to make sure > + * the export->next pointer is valid before another CPU sees > + * the export pointer included into the list. > + */ > + rcu_assign_pointer(*list, export); > + > +} > + > +static int > +rm_trace_export(struct trace_export **list, struct trace_export *export) > +{ > + struct trace_export **p; > + > + for (p = list; *p != &trace_export_rb; p = &(*p)->next) > + if (*p == export) > + break; > + > + if (*p != export) > + return -1; > + > + *p = (*p)->next; I believe this requires an rcu_assign_pointer() as well. > + > + return 0; > +} > + > +int register_trace_export(struct trace_export *export) > +{ > + if (!export->write) { > + pr_warn("trace_export must have the write() call back.\n"); Probably should be a "WARN_ON_ONCE()". > + return -1; > + } > + > + if (!export->name) { > + pr_warn("trace_export must have a name.\n"); > + return -1; > + } If name isn't used don't add it till it is. That is, this patch should not have "name" in the structure. It's confusing, because I don't see a purpose for it. > + > + mutex_lock(&trace_export_lock); > + > + export->tr = trace_exports_list->tr; I don't see where tr is ever assigned. > + export->commit = trace_generic_commit; Shouldn't the caller pass in the commit function too? > + > + add_trace_export(&trace_exports_list, export); > + > + mutex_unlock(&trace_export_lock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(register_trace_export); > + > +int unregister_trace_export(struct trace_export *export) > +{ > + int ret; > + > + mutex_lock(&trace_export_lock); > + > + ret = rm_trace_export(&trace_exports_list, export); > + > + mutex_unlock(&trace_export_lock); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(unregister_trace_export); > + > void > trace_function(struct trace_array *tr, > unsigned long ip, unsigned long parent_ip, unsigned long flags, > @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr, > entry->parent_ip = parent_ip; > > if (!call_filter_check_discard(call, entry, buffer, event)) How do you handle the discard? If the entry doesn't match the filter, it will try to discard the event. I don't see how the "trace_exports" handle that. > - __buffer_unlock_commit(buffer, event); > + trace_exports(tr, event); > } > > #ifdef CONFIG_STACKTRACE > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index f783df4..a40f07c 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -260,6 +260,7 @@ struct trace_array { > /* function tracing enabled */ > int function_enabled; > #endif > + struct trace_export *export; > }; > > enum { > @@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void) > break; \ > } > > +#undef IF_SIZE > +#define IF_SIZE(size, var, etype, id) \ > + if (var == id) { \ > + size = (sizeof(etype)); \ > + break; \ > + } > + > /* Will cause compile errors if type is not found. */ > extern void __ftrace_bad_type(void); > > @@ -339,6 +347,29 @@ extern void __ftrace_bad_type(void); > } while (0) > > /* > + * The trace_entry_size return the size of specific trace type > + * > + * IF_SIZE(size, var); > + * > + * Where "var" is just the given trace type. > + */ > +#define trace_entry_size(size, var) \ > + do { \ > + IF_SIZE(size, var, struct ftrace_entry, TRACE_FN); \ > + IF_SIZE(size, var, struct stack_entry, TRACE_STACK); \ > + IF_SIZE(size, var, struct userstack_entry, \ > + TRACE_USER_STACK); \ > + IF_SIZE(size, var, struct print_entry, TRACE_PRINT); \ > + IF_SIZE(size, var, struct bprint_entry, TRACE_BPRINT); \ > + IF_SIZE(size, var, struct bputs_entry, TRACE_BPUTS); \ > + IF_SIZE(size, var, struct trace_branch, TRACE_BRANCH); \ > + IF_SIZE(size, var, struct ftrace_graph_ent_entry, \ > + TRACE_GRAPH_ENT); \ > + IF_SIZE(size, var, struct ftrace_graph_ret_entry, \ > + TRACE_GRAPH_RET); \ I really really dislike this. This is a big if statement that all needs to go down one by one. Very inefficient! -- Steve > + } while (0) > + > +/* > * An option specific to a tracer. This is a boolean value. > * The bit is the bit index that sets its value on the > * flags value in struct tracer_flags.
Hello Steve, Please correct me if I'm missing something in my following reply. On 15 August 2016 at 22:28, Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 15 Aug 2016 19:50:01 +0800 > Chunyan Zhang <zhang.chunyan@linaro.org> wrote: > >> +int register_trace_export(struct trace_export *export); >> +int unregister_trace_export(struct trace_export *export); >> + >> +#endif /* CONFIG_TRACING */ >> + >> +#endif /* _LINUX_TRACE_H */ >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c >> index dade4c9..0247ac2 100644 >> --- a/kernel/trace/trace.c >> +++ b/kernel/trace/trace.c >> @@ -40,6 +40,7 @@ >> #include <linux/poll.h> >> #include <linux/nmi.h> >> #include <linux/fs.h> >> +#include <linux/trace.h> >> #include <linux/sched/rt.h> >> >> #include "trace.h" >> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr, >> ftrace_trace_userstack(buffer, flags, pc); >> } >> >> +static inline void >> +trace_generic_commit(struct trace_array *tr, >> + struct ring_buffer_event *event) > > Why is this marked inline? It is never called directly here. > >> +{ >> + struct trace_entry *entry; >> + struct trace_export *export = tr->export; >> + unsigned int size = 0; >> + >> + entry = ring_buffer_event_data(event); >> + >> + trace_entry_size(size, entry->type); >> + if (!size) >> + return; >> + >> + if (export->write) >> + export->write((char *)entry, size); >> +} >> + >> +static inline void > > Same here. > >> +trace_rb_commit(struct trace_array *tr, >> + struct ring_buffer_event *event) >> +{ >> + __buffer_unlock_commit(tr->trace_buffer.buffer, event); >> +} >> + >> +static DEFINE_MUTEX(trace_export_lock); >> + >> +static struct trace_export trace_export_rb __read_mostly = { >> + .name = "rb", >> + .commit = trace_rb_commit, > > Make sure equal signs are lined up. > >> + .next = NULL, >> +}; >> +static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb; > > trace_exports_list needs to be annotated as rcu, if you are going to > dereference it via rcu_dereference. That was the kbuild warning you > received. Ok, will do. But I guess 'ftrace_ops_list' in ftrace.c [1] also should be annotated as rcu? [1] http://lxr.free-electrons.com/source/kernel/trace/ftrace.c#L460 > >> + >> +inline void >> +trace_exports(struct trace_array *tr, struct ring_buffer_event *event) >> +{ >> + struct trace_export *export; >> + >> + preempt_disable_notrace(); >> + >> + for (export = rcu_dereference_raw_notrace(trace_exports_list); >> + export && export->commit; >> + export = rcu_dereference_raw_notrace(export->next)) { >> + tr->export = export; >> + export->commit(tr, event); >> + } >> + >> + preempt_enable_notrace(); >> +} > > I'm not too happy about the added overhead to normal function tracing > (it will be noticeable), but I can fix that later. I think I will try to revise here in the next revision. > >> + >> +static void >> +add_trace_export(struct trace_export **list, struct trace_export *export) >> +{ >> + export->next = *list; > > As export->next will most likely need to be marked __rcu as well, this > may need an rcu_assign_pointer() too. > >> + /* >> + * We are entering export into the list but another >> + * CPU might be walking that list. We need to make sure >> + * the export->next pointer is valid before another CPU sees >> + * the export pointer included into the list. >> + */ >> + rcu_assign_pointer(*list, export); >> + >> +} >> + >> +static int >> +rm_trace_export(struct trace_export **list, struct trace_export *export) >> +{ >> + struct trace_export **p; >> + >> + for (p = list; *p != &trace_export_rb; p = &(*p)->next) >> + if (*p == export) >> + break; >> + >> + if (*p != export) >> + return -1; >> + >> + *p = (*p)->next; > > I believe this requires an rcu_assign_pointer() as well. > >> + >> + return 0; >> +} >> + >> +int register_trace_export(struct trace_export *export) >> +{ >> + if (!export->write) { >> + pr_warn("trace_export must have the write() call back.\n"); > > Probably should be a "WARN_ON_ONCE()". Yes, will revise. > >> + return -1; >> + } >> + >> + if (!export->name) { >> + pr_warn("trace_export must have a name.\n"); >> + return -1; >> + } > > If name isn't used don't add it till it is. That is, this patch should > not have "name" in the structure. It's confusing, because I don't see a > purpose for it. Ok will remove that. > >> + >> + mutex_lock(&trace_export_lock); >> + >> + export->tr = trace_exports_list->tr; > > I don't see where tr is ever assigned. > >> + export->commit = trace_generic_commit; > > Shouldn't the caller pass in the commit function too? The trace_export::write() callback is for caller, commit function mainly deal with traces, is it better to keep 'trace_generic_commit' in trace.c, i.e don't expose it to callers? > >> + >> + add_trace_export(&trace_exports_list, export); >> + >> + mutex_unlock(&trace_export_lock); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(register_trace_export); >> + >> +int unregister_trace_export(struct trace_export *export) >> +{ >> + int ret; >> + >> + mutex_lock(&trace_export_lock); >> + >> + ret = rm_trace_export(&trace_exports_list, export); >> + >> + mutex_unlock(&trace_export_lock); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(unregister_trace_export); >> + >> void >> trace_function(struct trace_array *tr, >> unsigned long ip, unsigned long parent_ip, unsigned long flags, >> @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr, >> entry->parent_ip = parent_ip; >> >> if (!call_filter_check_discard(call, entry, buffer, event)) > > How do you handle the discard? If the entry doesn't match the filter, > it will try to discard the event. I don't see how the "trace_exports" > handle that. I directly used the entries which had already been filtered. Humm.. sorry, actually you lost me here. > > >> - __buffer_unlock_commit(buffer, event); >> + trace_exports(tr, event); >> } >> >> #ifdef CONFIG_STACKTRACE >> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h >> index f783df4..a40f07c 100644 >> --- a/kernel/trace/trace.h >> +++ b/kernel/trace/trace.h >> @@ -260,6 +260,7 @@ struct trace_array { >> /* function tracing enabled */ >> int function_enabled; >> #endif >> + struct trace_export *export; >> }; >> >> enum { >> @@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void) >> break; \ >> } >> >> +#undef IF_SIZE >> +#define IF_SIZE(size, var, etype, id) \ >> + if (var == id) { \ >> + size = (sizeof(etype)); \ >> + break; \ >> + } >> + >> /* Will cause compile errors if type is not found. */ >> extern void __ftrace_bad_type(void); >> >> @@ -339,6 +347,29 @@ extern void __ftrace_bad_type(void); >> } while (0) >> >> /* >> + * The trace_entry_size return the size of specific trace type >> + * >> + * IF_SIZE(size, var); >> + * >> + * Where "var" is just the given trace type. >> + */ >> +#define trace_entry_size(size, var) \ >> + do { \ >> + IF_SIZE(size, var, struct ftrace_entry, TRACE_FN); \ >> + IF_SIZE(size, var, struct stack_entry, TRACE_STACK); \ >> + IF_SIZE(size, var, struct userstack_entry, \ >> + TRACE_USER_STACK); \ >> + IF_SIZE(size, var, struct print_entry, TRACE_PRINT); \ >> + IF_SIZE(size, var, struct bprint_entry, TRACE_BPRINT); \ >> + IF_SIZE(size, var, struct bputs_entry, TRACE_BPUTS); \ >> + IF_SIZE(size, var, struct trace_branch, TRACE_BRANCH); \ >> + IF_SIZE(size, var, struct ftrace_graph_ent_entry, \ >> + TRACE_GRAPH_ENT); \ >> + IF_SIZE(size, var, struct ftrace_graph_ret_entry, \ >> + TRACE_GRAPH_RET); \ > > I really really dislike this. This is a big if statement that all > needs to go down one by one. Very inefficient! Ok, will change to other implementation. Thanks for your comments, Chunyan > > -- Steve > >> + } while (0) >> + >> +/* >> * An option specific to a tracer. This is a boolean value. >> * The bit is the bit index that sets its value on the >> * flags value in struct tracer_flags. >
On Wed, 17 Aug 2016 20:48:07 +0800 Chunyan Zhang <zhang.chunyan@linaro.org> wrote: > Ok, will do. > > But I guess 'ftrace_ops_list' in ftrace.c [1] also should be annotated as rcu? > > [1] http://lxr.free-electrons.com/source/kernel/trace/ftrace.c#L460 Hmm, perhaps it should. I wonder if sparse complains about this? > > > >> + > >> + mutex_lock(&trace_export_lock); > >> + > >> + export->tr = trace_exports_list->tr; > > > > I don't see where tr is ever assigned. > > > >> + export->commit = trace_generic_commit; > > > > Shouldn't the caller pass in the commit function too? > > The trace_export::write() callback is for caller, commit function > mainly deal with traces, is it better to keep 'trace_generic_commit' > in trace.c, i.e don't expose it to callers? It's fine to be external if it's only declared in kernel/trace/trace.h. I would think anything using a different "write" would require a different "commit". But maybe I'm misunderstanding your objective. See below. > > > > >> + > >> + add_trace_export(&trace_exports_list, export); > >> + > >> + mutex_unlock(&trace_export_lock); > >> + > >> + return 0; > >> +} > >> +EXPORT_SYMBOL_GPL(register_trace_export); > >> + > >> +int unregister_trace_export(struct trace_export *export) > >> +{ > >> + int ret; > >> + > >> + mutex_lock(&trace_export_lock); > >> + > >> + ret = rm_trace_export(&trace_exports_list, export); > >> + > >> + mutex_unlock(&trace_export_lock); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(unregister_trace_export); > >> + > >> void > >> trace_function(struct trace_array *tr, > >> unsigned long ip, unsigned long parent_ip, unsigned long flags, > >> @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr, > >> entry->parent_ip = parent_ip; > >> > >> if (!call_filter_check_discard(call, entry, buffer, event)) > > > > How do you handle the discard? If the entry doesn't match the filter, > > it will try to discard the event. I don't see how the "trace_exports" > > handle that. > > I directly used the entries which had already been filtered. > Humm.. sorry, actually you lost me here. I'm assuming this entire patch is to have the events written to something other than the ftrace ring buffer, correct? Or is this just trying to hook into the tracing that is happening? That is, this isn't replacing writing into the ftrace ring buffer, but it is just adding a way to write to someplace in addition to the ftrace ring buffer. Where you still write to the ftrace ring buffer, but then you can add a hook to copy someplace else as well. I was looking at this as a way that you are adding a replacement, not only an addition to. If that's the case, I think there may be a easier way to do this. -- Steve
Hi Fengguang, On 15 August 2016 at 21:03, kbuild test robot <lkp@intel.com> wrote: > Hi Chunyan, > > [auto build test WARNING on linus/master] > [also build test WARNING on v4.8-rc2 next-20160815] > [cannot apply to linux/master] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Chunyan-Zhang/tracing-add-a-possibility-of-exporting-function-trace-to-other-places-instead-of-ring-buffer-only/20160815-195631 > reproduce: > # apt-get install sparse It may be better to use the latest version of sparse, i.e. git clone git://git.kernel.org/pub/scm/devel/sparse/sparse.git and then build the source code according to the direction on the page https://kernelnewbies.org/Sparse Otherwise, would get the errors like below if I install sparse using apt-get install. ------------------------------------------------------- CHECK arch/x86/purgatory/purgatory.c No such file: asan-stack=1 scripts/Makefile.build:289: recipe for target 'arch/x86/purgatory/purgatory.o' failed make[1]: *** [arch/x86/purgatory/purgatory.o] Error 1 arch/x86/Makefile:195: recipe for target 'archprepare' failed make: *** [archprepare] Error 2 ------------------------------------------------------- Thanks, Chunyan > make ARCH=x86_64 allmodconfig > make C=1 CF=-D__CHECK_ENDIAN__ > > sparse warnings: (new ones prefixed by >>) > > include/linux/compiler.h:230:8: sparse: attribute 'no_sanitize_address': unknown attribute >>> kernel/trace/trace.c:2173:23: sparse: incompatible types in comparison expression (different address spaces) > kernel/trace/trace.c:2175:23: sparse: incompatible types in comparison expression (different address spaces) > > vim +2173 kernel/trace/trace.c > > 2157 static DEFINE_MUTEX(trace_export_lock); > 2158 > 2159 static struct trace_export trace_export_rb __read_mostly = { > 2160 .name = "rb", > 2161 .commit = trace_rb_commit, > 2162 .next = NULL, > 2163 }; > 2164 static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb; > 2165 > 2166 inline void > 2167 trace_exports(struct trace_array *tr, struct ring_buffer_event *event) > 2168 { > 2169 struct trace_export *export; > 2170 > 2171 preempt_disable_notrace(); > 2172 >> 2173 for (export = rcu_dereference_raw_notrace(trace_exports_list); > 2174 export && export->commit; > 2175 export = rcu_dereference_raw_notrace(export->next)) { > 2176 tr->export = export; > 2177 export->commit(tr, event); > 2178 } > 2179 > 2180 preempt_enable_notrace(); > 2181 } > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Steve, Please correct me if I'm misunderstanding something. On 17 August 2016 at 22:11, Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 17 Aug 2016 20:48:07 +0800 > Chunyan Zhang <zhang.chunyan@linaro.org> wrote: > > >> Ok, will do. >> >> But I guess 'ftrace_ops_list' in ftrace.c [1] also should be annotated as rcu? >> >> [1] http://lxr.free-electrons.com/source/kernel/trace/ftrace.c#L460 > > Hmm, perhaps it should. I wonder if sparse complains about this? > Yes, there are many this kind of errors in kernel/trace/ftrace.c and kernel/trace/trace_events_filter.c I can submit a patch to fix them. > >> > >> >> + >> >> + mutex_lock(&trace_export_lock); >> >> + >> >> + export->tr = trace_exports_list->tr; >> > >> > I don't see where tr is ever assigned. >> > >> >> + export->commit = trace_generic_commit; >> > >> > Shouldn't the caller pass in the commit function too? >> >> The trace_export::write() callback is for caller, commit function >> mainly deal with traces, is it better to keep 'trace_generic_commit' >> in trace.c, i.e don't expose it to callers? > > It's fine to be external if it's only declared in kernel/trace/trace.h. But we cannot assume that the trace_exports all are under kernel/trace/, for example in this patchset 2/2, 'stm_ftrace' as a trace_export was registered in stm subsystem but during its initialization it cannot access 'trace_generic_commit' which is under kernel/trace. > I would think anything using a different "write" would require a > different "commit". Ok, I should make it more feasible rather than pointing to 'trace_generic_commit' directly when registering trace_export, that's say, if the trace_export doesn't have its own commit function, point directly to 'trace_generic_commit'. > > But maybe I'm misunderstanding your objective. See below. > >> >> > >> >> + >> >> + add_trace_export(&trace_exports_list, export); >> >> + >> >> + mutex_unlock(&trace_export_lock); >> >> + >> >> + return 0; >> >> +} >> >> +EXPORT_SYMBOL_GPL(register_trace_export); >> >> + >> >> +int unregister_trace_export(struct trace_export *export) >> >> +{ >> >> + int ret; >> >> + >> >> + mutex_lock(&trace_export_lock); >> >> + >> >> + ret = rm_trace_export(&trace_exports_list, export); >> >> + >> >> + mutex_unlock(&trace_export_lock); >> >> + >> >> + return ret; >> >> +} >> >> +EXPORT_SYMBOL_GPL(unregister_trace_export); >> >> + >> >> void >> >> trace_function(struct trace_array *tr, >> >> unsigned long ip, unsigned long parent_ip, unsigned long flags, >> >> @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr, >> >> entry->parent_ip = parent_ip; >> >> >> >> if (!call_filter_check_discard(call, entry, buffer, event)) >> > >> > How do you handle the discard? If the entry doesn't match the filter, >> > it will try to discard the event. I don't see how the "trace_exports" >> > handle that. >> >> I directly used the entries which had already been filtered. >> Humm.. sorry, actually you lost me here. > > I'm assuming this entire patch is to have the events written to > something other than the ftrace ring buffer, correct? > > Or is this just trying to hook into the tracing that is happening? That > is, this isn't replacing writing into the ftrace ring buffer, but it is > just adding a way to write to someplace in addition to the ftrace ring > buffer. Where you still write to the ftrace ring buffer, but then you > can add a hook to copy someplace else as well. Yes, this is what this patch is trying to implement. > > I was looking at this as a way that you are adding a replacement, not > only an addition to. If that's the case, I think there may be a easier > way to do this. I want to know how it would be in the easier way you mentioned here. I was trying to add a ftrace_ops before, but with that way, I have to deal with a lot of trace or ring buffer stuff including the sort of discard things like you mentioned, which the existed ftrace code does. And if I choose to implement a new ftrace_ops, I'm only able to get the function trace support for STM and have to do many things which would be overlap with the current ftrace subsystem. So in order to reuse the existed code and architecture, I chose to add a trace_export interface for Ftrace subsytem, and in this way I'm using in this patch, I will get all supports of traces which are dealt with trace_function(); Another benefit of adding a trace_export is, if there will be other subsystem would like to use the processed traces, it only needs to register a trace_export and provides a .write() function call back or together with a commit function, although from what I can see now .write() is enough since my purpose was the processed traces I don't need 'ring_buffer_event' so long as I had trace entries. Thanks for your comments, Chunyan > > -- Steve
On Thu, 18 Aug 2016 17:22:11 +0800 Chunyan Zhang <zhang.chunyan@linaro.org> wrote: > > Or is this just trying to hook into the tracing that is happening? That > > is, this isn't replacing writing into the ftrace ring buffer, but it is > > just adding a way to write to someplace in addition to the ftrace ring > > buffer. Where you still write to the ftrace ring buffer, but then you > > can add a hook to copy someplace else as well. > > Yes, this is what this patch is trying to implement. > > > > > I was looking at this as a way that you are adding a replacement, not > > only an addition to. If that's the case, I think there may be a easier > > way to do this. > > I want to know how it would be in the easier way you mentioned here. > > I was trying to add a ftrace_ops before, but with that way, I have to > deal with a lot of trace or ring buffer stuff including the sort of > discard things like you mentioned, which the existed ftrace code does. > And if I choose to implement a new ftrace_ops, I'm only able to get > the function trace support for STM and have to do many things which > would be overlap with the current ftrace subsystem. Adding your own ftrace_ops is a way for replacing, not just adding a hook into. > > So in order to reuse the existed code and architecture, I chose to add > a trace_export interface for Ftrace subsytem, and in this way I'm > using in this patch, I will get all supports of traces which are dealt > with trace_function(); Actually, a trace_export() should only be called if there's been something added. And that should be done with a static_key_false() branch (which is dynamically enabled, and does not use a comparison branch). That is, something like this instead: if (!call_filter_check_discard(call, entry, buffer, event)) { if (static_key_false(&ftrace_trace_exports_enabled)) ftrace_exports(tr, event); __buffer_unlock_commit(buffer, event); } Don't touch the current logic. Just have your code hook into the ftrace_exports (note I use "ftrace_exports" and not trace_exports() because it's the function tracer, which has stricter requirements than events do. If you add a hook for tracepoints later, use trace_exports() and have a different list for that). > > Another benefit of adding a trace_export is, if there will be other > subsystem would like to use the processed traces, it only needs to > register a trace_export and provides a .write() function call back or > together with a commit function, although from what I can see now > .write() is enough since my purpose was the processed traces I don't > need 'ring_buffer_event' so long as I had trace entries. I'm saying if you don't mind the ring buffer being used along with your own code (which seems to be what's happening), then just add a call back to your code. Don't monkey with the current logic. I think that will simplify things tremendously. -- Steve
On 19 August 2016 at 00:12, Steven Rostedt <rostedt@goodmis.org> wrote: > On Thu, 18 Aug 2016 17:22:11 +0800 > Chunyan Zhang <zhang.chunyan@linaro.org> wrote: > > >> > Or is this just trying to hook into the tracing that is happening? That >> > is, this isn't replacing writing into the ftrace ring buffer, but it is >> > just adding a way to write to someplace in addition to the ftrace ring >> > buffer. Where you still write to the ftrace ring buffer, but then you >> > can add a hook to copy someplace else as well. >> >> Yes, this is what this patch is trying to implement. >> >> > >> > I was looking at this as a way that you are adding a replacement, not >> > only an addition to. If that's the case, I think there may be a easier >> > way to do this. >> >> I want to know how it would be in the easier way you mentioned here. >> >> I was trying to add a ftrace_ops before, but with that way, I have to >> deal with a lot of trace or ring buffer stuff including the sort of >> discard things like you mentioned, which the existed ftrace code does. >> And if I choose to implement a new ftrace_ops, I'm only able to get >> the function trace support for STM and have to do many things which >> would be overlap with the current ftrace subsystem. > > Adding your own ftrace_ops is a way for replacing, not just adding a > hook into. > >> >> So in order to reuse the existed code and architecture, I chose to add >> a trace_export interface for Ftrace subsytem, and in this way I'm >> using in this patch, I will get all supports of traces which are dealt >> with trace_function(); > > Actually, a trace_export() should only be called if there's been > something added. And that should be done with a static_key_false() > branch (which is dynamically enabled, and does not use a comparison > branch). > > That is, something like this instead: > > if (!call_filter_check_discard(call, entry, buffer, event)) { > if (static_key_false(&ftrace_trace_exports_enabled)) > ftrace_exports(tr, event); > __buffer_unlock_commit(buffer, event); > } > Thanks for the sample code, I got it, will do like this. > Don't touch the current logic. Just have your code hook into the > ftrace_exports (note I use "ftrace_exports" and not trace_exports() > because it's the function tracer, which has stricter requirements than > events do. If you add a hook for tracepoints later, use trace_exports() > and have a different list for that). > >> >> Another benefit of adding a trace_export is, if there will be other >> subsystem would like to use the processed traces, it only needs to >> register a trace_export and provides a .write() function call back or >> together with a commit function, although from what I can see now >> .write() is enough since my purpose was the processed traces I don't >> need 'ring_buffer_event' so long as I had trace entries. > > I'm saying if you don't mind the ring buffer being used along with > your own code (which seems to be what's happening), then just add a > call back to your code. Don't monkey with the current logic. > > I think that will simplify things tremendously. Thanks for your comments and detailed explanation, Chunyan > > -- Steve
diff --git a/include/linux/trace.h b/include/linux/trace.h new file mode 100644 index 0000000..4d4f0e1 --- /dev/null +++ b/include/linux/trace.h @@ -0,0 +1,33 @@ +#ifndef _LINUX_TRACE_H +#define _LINUX_TRACE_H + +#include <linux/ring_buffer.h> +struct trace_array; + +#ifdef CONFIG_TRACING +/* + * The trace export - an export of function traces. Every ftrace_ops + * has at least one export which would output function traces to ring + * buffer. + * + * name - the name of this export + * next - pointer to the next trace_export + * tr - the trace_array this export belongs to + * commit - commit the traces to ring buffer and/or some other places + * write - copy traces which have been delt with ->commit() to + * the destination + */ +struct trace_export { + char name[16]; + struct trace_export *next; + struct trace_array *tr; + void (*commit)(struct trace_array *, struct ring_buffer_event *); + void (*write)(const char *, unsigned int); +}; + +int register_trace_export(struct trace_export *export); +int unregister_trace_export(struct trace_export *export); + +#endif /* CONFIG_TRACING */ + +#endif /* _LINUX_TRACE_H */ diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index dade4c9..0247ac2 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -40,6 +40,7 @@ #include <linux/poll.h> #include <linux/nmi.h> #include <linux/fs.h> +#include <linux/trace.h> #include <linux/sched/rt.h> #include "trace.h" @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr, ftrace_trace_userstack(buffer, flags, pc); } +static inline void +trace_generic_commit(struct trace_array *tr, + struct ring_buffer_event *event) +{ + struct trace_entry *entry; + struct trace_export *export = tr->export; + unsigned int size = 0; + + entry = ring_buffer_event_data(event); + + trace_entry_size(size, entry->type); + if (!size) + return; + + if (export->write) + export->write((char *)entry, size); +} + +static inline void +trace_rb_commit(struct trace_array *tr, + struct ring_buffer_event *event) +{ + __buffer_unlock_commit(tr->trace_buffer.buffer, event); +} + +static DEFINE_MUTEX(trace_export_lock); + +static struct trace_export trace_export_rb __read_mostly = { + .name = "rb", + .commit = trace_rb_commit, + .next = NULL, +}; +static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb; + +inline void +trace_exports(struct trace_array *tr, struct ring_buffer_event *event) +{ + struct trace_export *export; + + preempt_disable_notrace(); + + for (export = rcu_dereference_raw_notrace(trace_exports_list); + export && export->commit; + export = rcu_dereference_raw_notrace(export->next)) { + tr->export = export; + export->commit(tr, event); + } + + preempt_enable_notrace(); +} + +static void +add_trace_export(struct trace_export **list, struct trace_export *export) +{ + export->next = *list; + /* + * We are entering export into the list but another + * CPU might be walking that list. We need to make sure + * the export->next pointer is valid before another CPU sees + * the export pointer included into the list. + */ + rcu_assign_pointer(*list, export); + +} + +static int +rm_trace_export(struct trace_export **list, struct trace_export *export) +{ + struct trace_export **p; + + for (p = list; *p != &trace_export_rb; p = &(*p)->next) + if (*p == export) + break; + + if (*p != export) + return -1; + + *p = (*p)->next; + + return 0; +} + +int register_trace_export(struct trace_export *export) +{ + if (!export->write) { + pr_warn("trace_export must have the write() call back.\n"); + return -1; + } + + if (!export->name) { + pr_warn("trace_export must have a name.\n"); + return -1; + } + + mutex_lock(&trace_export_lock); + + export->tr = trace_exports_list->tr; + export->commit = trace_generic_commit; + + add_trace_export(&trace_exports_list, export); + + mutex_unlock(&trace_export_lock); + + return 0; +} +EXPORT_SYMBOL_GPL(register_trace_export); + +int unregister_trace_export(struct trace_export *export) +{ + int ret; + + mutex_lock(&trace_export_lock); + + ret = rm_trace_export(&trace_exports_list, export); + + mutex_unlock(&trace_export_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(unregister_trace_export); + void trace_function(struct trace_array *tr, unsigned long ip, unsigned long parent_ip, unsigned long flags, @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr, entry->parent_ip = parent_ip; if (!call_filter_check_discard(call, entry, buffer, event)) - __buffer_unlock_commit(buffer, event); + trace_exports(tr, event); } #ifdef CONFIG_STACKTRACE diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index f783df4..a40f07c 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -260,6 +260,7 @@ struct trace_array { /* function tracing enabled */ int function_enabled; #endif + struct trace_export *export; }; enum { @@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void) break; \ } +#undef IF_SIZE +#define IF_SIZE(size, var, etype, id) \ + if (var == id) { \ + size = (sizeof(etype)); \ + break; \ + } + /* Will cause compile errors if type is not found. */ extern void __ftrace_bad_type(void); @@ -339,6 +347,29 @@ extern void __ftrace_bad_type(void); } while (0) /* + * The trace_entry_size return the size of specific trace type + * + * IF_SIZE(size, var); + * + * Where "var" is just the given trace type. + */ +#define trace_entry_size(size, var) \ + do { \ + IF_SIZE(size, var, struct ftrace_entry, TRACE_FN); \ + IF_SIZE(size, var, struct stack_entry, TRACE_STACK); \ + IF_SIZE(size, var, struct userstack_entry, \ + TRACE_USER_STACK); \ + IF_SIZE(size, var, struct print_entry, TRACE_PRINT); \ + IF_SIZE(size, var, struct bprint_entry, TRACE_BPRINT); \ + IF_SIZE(size, var, struct bputs_entry, TRACE_BPUTS); \ + IF_SIZE(size, var, struct trace_branch, TRACE_BRANCH); \ + IF_SIZE(size, var, struct ftrace_graph_ent_entry, \ + TRACE_GRAPH_ENT); \ + IF_SIZE(size, var, struct ftrace_graph_ret_entry, \ + TRACE_GRAPH_RET); \ + } while (0) + +/* * An option specific to a tracer. This is a boolean value. * The bit is the bit index that sets its value on the * flags value in struct tracer_flags.
Currently ring buffer is the only one output of Function traces, this patch added trace_export concept which would process the traces and export traces to a registered destination which can be ring buffer or some other storage, in this way if we want Function traces to be sent to other destination rather than ring buffer only, we just need to register a new trace_export and implement its own .commit() callback or just use 'trace_generic_commit()' which this patch also added and hooks up its own .write() functio for writing traces to the storage. Currently, only Function trace (TRACE_FN) is supported. Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org> --- include/linux/trace.h | 33 ++++++++++++++ kernel/trace/trace.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++- kernel/trace/trace.h | 31 +++++++++++++ 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 include/linux/trace.h