* [PATCH 0/3] tracing: fix macro expansion and refactor some of dynamic_array support @ 2014-03-01 5:32 Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 1/3] tracing: correctly expand len expressions from __dynamic_array macro Filipe Brandenburger ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Filipe Brandenburger @ 2014-03-01 5:32 UTC (permalink / raw) To: Steven Rostedt, Li Zefan Cc: Frederic Weisbecker, Ingo Molnar, linux-kernel, Filipe Brandenburger Hi Steven Rostedt, Li Zefan, This fixes an issue with macro expansion introduced in commit 7d536cb3f (tracing/events: record the size of dynamic arrays). I split it in 3 patches, the first fixes a bug, the second improves the code to evaluate the expression only once and the third refactors an u32 holding two pieces of data in lower/higher 16 bits into a struct to make the code cleaner. I split them this way since I expect the first two to be more straightforward while the third one might generate some discussion. I'd be happy to squash them into a single one if you'd prefer that. Cheers, Filipe Filipe Brandenburger (3): tracing: correctly expand len expressions from __dynamic_array macro tracing: evaluate len expression only once in __dynamic_array macro tracing: introduce a trace_data_offset struct to store array size include/linux/ftrace_event.h | 5 +++++ include/trace/ftrace.h | 26 ++++++++++++++------------ kernel/trace/trace_events_filter.c | 13 ++++++++++--- 3 files changed, 29 insertions(+), 15 deletions(-) -- 1.9.0.279.gdc9e3eb ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] tracing: correctly expand len expressions from __dynamic_array macro 2014-03-01 5:32 [PATCH 0/3] tracing: fix macro expansion and refactor some of dynamic_array support Filipe Brandenburger @ 2014-03-01 5:32 ` Filipe Brandenburger 2014-03-03 19:51 ` Steven Rostedt 2014-03-01 5:32 ` [PATCH 2/3] tracing: evaluate len expression only once in " Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 3/3] tracing: introduce a trace_data_offset struct to store array size Filipe Brandenburger 2 siblings, 1 reply; 8+ messages in thread From: Filipe Brandenburger @ 2014-03-01 5:32 UTC (permalink / raw) To: Steven Rostedt, Li Zefan Cc: Frederic Weisbecker, Ingo Molnar, linux-kernel, Filipe Brandenburger This fixes expansion of the len argument in __dynamic_array macros. The previous code from commit 7d536cb3f would not fully evaluate the expression before multiplying its result by the size of the type. This went unnoticed because the length stored in the high 16 bits of the offset (which is the one that was broken here) is only used by filter_pred_strloc which only acts on strings for which the size of the type is 1. Signed-off-by: Filipe Brandenburger <filbranden@google.com> --- include/trace/ftrace.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h index 1a8b28d..82e8d89 100644 --- a/include/trace/ftrace.h +++ b/include/trace/ftrace.h @@ -375,7 +375,7 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call) \ #define __dynamic_array(type, item, len) \ __data_offsets->item = __data_size + \ offsetof(typeof(*entry), __data); \ - __data_offsets->item |= (len * sizeof(type)) << 16; \ + __data_offsets->item |= ((len) * sizeof(type)) << 16; \ __data_size += (len) * sizeof(type); #undef __string -- 1.9.0.279.gdc9e3eb ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] tracing: correctly expand len expressions from __dynamic_array macro 2014-03-01 5:32 ` [PATCH 1/3] tracing: correctly expand len expressions from __dynamic_array macro Filipe Brandenburger @ 2014-03-03 19:51 ` Steven Rostedt 2014-03-03 19:57 ` Filipe Brandenburger 0 siblings, 1 reply; 8+ messages in thread From: Steven Rostedt @ 2014-03-03 19:51 UTC (permalink / raw) To: Filipe Brandenburger Cc: Li Zefan, Frederic Weisbecker, Ingo Molnar, linux-kernel On Fri, 28 Feb 2014 21:32:16 -0800 Filipe Brandenburger <filbranden@google.com> wrote: > This fixes expansion of the len argument in __dynamic_array macros. > The previous code from commit 7d536cb3f would not fully evaluate the > expression before multiplying its result by the size of the type. > > This went unnoticed because the length stored in the high 16 bits of the > offset (which is the one that was broken here) is only used by > filter_pred_strloc which only acts on strings for which the size of the > type is 1. > Was this visible in any of the tracepoints? Should this be marked for stable? It's been in the kernel for a long time (2009). Or is a new tracepoint showing a problem? -- Steve > Signed-off-by: Filipe Brandenburger <filbranden@google.com> > --- > include/trace/ftrace.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h > index 1a8b28d..82e8d89 100644 > --- a/include/trace/ftrace.h > +++ b/include/trace/ftrace.h > @@ -375,7 +375,7 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call) \ > #define __dynamic_array(type, item, len) \ > __data_offsets->item = __data_size + \ > offsetof(typeof(*entry), __data); \ > - __data_offsets->item |= (len * sizeof(type)) << 16; \ > + __data_offsets->item |= ((len) * sizeof(type)) << 16; \ > __data_size += (len) * sizeof(type); > > #undef __string ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] tracing: correctly expand len expressions from __dynamic_array macro 2014-03-03 19:51 ` Steven Rostedt @ 2014-03-03 19:57 ` Filipe Brandenburger 0 siblings, 0 replies; 8+ messages in thread From: Filipe Brandenburger @ 2014-03-03 19:57 UTC (permalink / raw) To: Steven Rostedt; +Cc: Li Zefan, Frederic Weisbecker, Ingo Molnar, linux-kernel Hi Steve, I don't think this needs to be backported to stable, since the only place that uses the high 16 bits of the offset field as the length is filter_pred_strloc which only works for strings and sizeof(char) == 1 so that essentially whenever the bug is triggered, the actual stored value is not used. I have a follow up patch to expose that field to TP_fast_assign (as something like __get_dynamic_array_length) in which case it's important that this is fixed, otherwise for __dynamic_array of something other than char we'll have a bug. Thanks, Filipe On Mon, Mar 3, 2014 at 11:51 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 28 Feb 2014 21:32:16 -0800 > Filipe Brandenburger <filbranden@google.com> wrote: > >> This fixes expansion of the len argument in __dynamic_array macros. >> The previous code from commit 7d536cb3f would not fully evaluate the >> expression before multiplying its result by the size of the type. >> >> This went unnoticed because the length stored in the high 16 bits of the >> offset (which is the one that was broken here) is only used by >> filter_pred_strloc which only acts on strings for which the size of the >> type is 1. >> > > Was this visible in any of the tracepoints? Should this be marked for > stable? It's been in the kernel for a long time (2009). Or is a new > tracepoint showing a problem? > > -- Steve > >> Signed-off-by: Filipe Brandenburger <filbranden@google.com> >> --- >> include/trace/ftrace.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h >> index 1a8b28d..82e8d89 100644 >> --- a/include/trace/ftrace.h >> +++ b/include/trace/ftrace.h >> @@ -375,7 +375,7 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call) \ >> #define __dynamic_array(type, item, len) \ >> __data_offsets->item = __data_size + \ >> offsetof(typeof(*entry), __data); \ >> - __data_offsets->item |= (len * sizeof(type)) << 16; \ >> + __data_offsets->item |= ((len) * sizeof(type)) << 16; \ >> __data_size += (len) * sizeof(type); >> >> #undef __string > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] tracing: evaluate len expression only once in __dynamic_array macro 2014-03-01 5:32 [PATCH 0/3] tracing: fix macro expansion and refactor some of dynamic_array support Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 1/3] tracing: correctly expand len expressions from __dynamic_array macro Filipe Brandenburger @ 2014-03-01 5:32 ` Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 3/3] tracing: introduce a trace_data_offset struct to store array size Filipe Brandenburger 2 siblings, 0 replies; 8+ messages in thread From: Filipe Brandenburger @ 2014-03-01 5:32 UTC (permalink / raw) To: Steven Rostedt, Li Zefan Cc: Frederic Weisbecker, Ingo Molnar, linux-kernel, Filipe Brandenburger Use a temporary variable to store the expansion of the len expression. If the evaluation is expensive, this commit will ensure it is evaluated only once inside ftrace_get_offsets_<call>. Signed-off-by: Filipe Brandenburger <filbranden@google.com> --- include/trace/ftrace.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h index 82e8d89..86a056a 100644 --- a/include/trace/ftrace.h +++ b/include/trace/ftrace.h @@ -373,10 +373,11 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call) \ #undef __dynamic_array #define __dynamic_array(type, item, len) \ + __item_length = (len) * sizeof(type); \ __data_offsets->item = __data_size + \ offsetof(typeof(*entry), __data); \ - __data_offsets->item |= ((len) * sizeof(type)) << 16; \ - __data_size += (len) * sizeof(type); + __data_offsets->item |= __item_length << 16; \ + __data_size += __item_length; #undef __string #define __string(item, src) __dynamic_array(char, item, \ @@ -388,6 +389,7 @@ static inline notrace int ftrace_get_offsets_##call( \ struct ftrace_data_offsets_##call *__data_offsets, proto) \ { \ int __data_size = 0; \ + int __maybe_unused __item_length; \ struct ftrace_raw_##call __maybe_unused *entry; \ \ tstruct; \ -- 1.9.0.279.gdc9e3eb ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] tracing: introduce a trace_data_offset struct to store array size 2014-03-01 5:32 [PATCH 0/3] tracing: fix macro expansion and refactor some of dynamic_array support Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 1/3] tracing: correctly expand len expressions from __dynamic_array macro Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 2/3] tracing: evaluate len expression only once in " Filipe Brandenburger @ 2014-03-01 5:32 ` Filipe Brandenburger 2014-03-03 20:48 ` Steven Rostedt 2 siblings, 1 reply; 8+ messages in thread From: Filipe Brandenburger @ 2014-03-01 5:32 UTC (permalink / raw) To: Steven Rostedt, Li Zefan Cc: Frederic Weisbecker, Ingo Molnar, linux-kernel, Filipe Brandenburger Commit 7d536cb3f stores the length of the array in the high 16 bits of the offset field. Using a struct with two separate 16 bit fields makes it cleaner. Tested: Boot kernel with this change, set a 'filename ~ "/usr/bin/pst*"' regex filter on events/sched/sched_process_exec/filter, enabled tracing, checked that calling pstree would log the trace event as expected. Signed-off-by: Filipe Brandenburger <filbranden@google.com> --- include/linux/ftrace_event.h | 5 +++++ include/trace/ftrace.h | 28 ++++++++++++++-------------- kernel/trace/trace_events_filter.c | 13 ++++++++++--- 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h index 4e4cc28..67e4122 100644 --- a/include/linux/ftrace_event.h +++ b/include/linux/ftrace_event.h @@ -123,6 +123,11 @@ struct trace_event { struct trace_event_functions *funcs; }; +struct trace_array_offset { + u16 offset; + u16 length; +}; + extern int register_ftrace_event(struct trace_event *event); extern int unregister_ftrace_event(struct trace_event *event); diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h index 86a056a..eac4d0a 100644 --- a/include/trace/ftrace.h +++ b/include/trace/ftrace.h @@ -48,7 +48,8 @@ #define __array(type, item, len) type item[len]; #undef __dynamic_array -#define __dynamic_array(type, item, len) u32 __data_loc_##item; +#define __dynamic_array(type, item, len) \ + struct trace_array_offset __data_loc_##item; #undef __string #define __string(item, src) __dynamic_array(char, item, -1) @@ -103,14 +104,14 @@ * Include the following: * * struct ftrace_data_offsets_<call> { - * u32 <item1>; - * u32 <item2>; + * struct trace_array_offset <item1>; + * struct trace_array_offset <item2>; * [...] * }; * - * The __dynamic_array() macro will create each u32 <item>, this is - * to keep the offset of each array from the beginning of the event. - * The size of an array is also encoded, in the higher 16 bits of <item>. + * The __dynamic_array() macro will create each trace_array_offset <item>, this + * is to keep the offset and length of each array from the beginning of the + * event. */ #undef __field @@ -123,7 +124,8 @@ #define __array(type, item, len) #undef __dynamic_array -#define __dynamic_array(type, item, len) u32 item; +#define __dynamic_array(type, item, len) \ + struct trace_array_offset item; #undef __string #define __string(item, src) __dynamic_array(char, item, -1) @@ -195,7 +197,7 @@ #undef __get_dynamic_array #define __get_dynamic_array(field) \ - ((void *)__entry + (__entry->__data_loc_##field & 0xffff)) + ((void *)__entry + __entry->__data_loc_##field.offset) #undef __get_str #define __get_str(field) (char *)__get_dynamic_array(field) @@ -373,11 +375,10 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call) \ #undef __dynamic_array #define __dynamic_array(type, item, len) \ - __item_length = (len) * sizeof(type); \ - __data_offsets->item = __data_size + \ + __data_offsets->item.offset = __data_size + \ offsetof(typeof(*entry), __data); \ - __data_offsets->item |= __item_length << 16; \ - __data_size += __item_length; + __data_offsets->item.length = (len) * sizeof(type); \ + __data_size += __data_offsets->item.length; #undef __string #define __string(item, src) __dynamic_array(char, item, \ @@ -389,7 +390,6 @@ static inline notrace int ftrace_get_offsets_##call( \ struct ftrace_data_offsets_##call *__data_offsets, proto) \ { \ int __data_size = 0; \ - int __maybe_unused __item_length; \ struct ftrace_raw_##call __maybe_unused *entry; \ \ tstruct; \ @@ -658,7 +658,7 @@ __attribute__((section("_ftrace_events"))) *__event_##call = &event_##call #undef __get_dynamic_array #define __get_dynamic_array(field) \ - ((void *)__entry + (__entry->__data_loc_##field & 0xffff)) + ((void *)__entry + __entry->__data_loc_##field.offset) #undef __get_str #define __get_str(field) (char *)__get_dynamic_array(field) diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c index 8a86319..805fc0d 100644 --- a/kernel/trace/trace_events_filter.c +++ b/kernel/trace/trace_events_filter.c @@ -234,12 +234,19 @@ static int filter_pred_pchar(struct filter_pred *pred, void *event) */ static int filter_pred_strloc(struct filter_pred *pred, void *event) { - u32 str_item = *(u32 *)(event + pred->offset); - int str_loc = str_item & 0xffff; - int str_len = str_item >> 16; + struct trace_array_offset *array_offset = + (struct trace_array_offset *)(event + pred->offset); + int str_loc = array_offset->offset; + int str_len = array_offset->length; char *addr = (char *)(event + str_loc); int cmp, match; + /* + * As struct trace_array_offset is replacing an u32, + * ensure they have the same size. + */ + BUILD_BUG_ON(sizeof(struct trace_array_offset) != sizeof(u32)); + cmp = pred->regex.match(addr, &pred->regex, str_len); match = cmp ^ pred->not; -- 1.9.0.279.gdc9e3eb ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] tracing: introduce a trace_data_offset struct to store array size 2014-03-01 5:32 ` [PATCH 3/3] tracing: introduce a trace_data_offset struct to store array size Filipe Brandenburger @ 2014-03-03 20:48 ` Steven Rostedt 2014-03-03 20:59 ` Steven Rostedt 0 siblings, 1 reply; 8+ messages in thread From: Steven Rostedt @ 2014-03-03 20:48 UTC (permalink / raw) To: Filipe Brandenburger Cc: Li Zefan, Frederic Weisbecker, Ingo Molnar, linux-kernel You're right, this one will bring up discussion. On Fri, 28 Feb 2014 21:32:18 -0800 Filipe Brandenburger <filbranden@google.com> wrote: > Commit 7d536cb3f stores the length of the array in the high 16 bits of > the offset field. Using a struct with two separate 16 bit fields makes > it cleaner. > > Tested: Boot kernel with this change, set a 'filename ~ "/usr/bin/pst*"' > regex filter on events/sched/sched_process_exec/filter, enabled tracing, > checked that calling pstree would log the trace event as expected. > I just applied this and tested it on my PPC64 box, and it gives me the following: without patch: pst2pdf-3506 [000] 120.700910: sched_process_exec: filename=/usr/bin/pst2pdf pid=3506 old_pid=3506 With patch: pstopnm-4432 [001] 1490.246765: sched_process_exec: filename= pid=4432 old_pid=4432 -- Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] tracing: introduce a trace_data_offset struct to store array size 2014-03-03 20:48 ` Steven Rostedt @ 2014-03-03 20:59 ` Steven Rostedt 0 siblings, 0 replies; 8+ messages in thread From: Steven Rostedt @ 2014-03-03 20:59 UTC (permalink / raw) To: Steven Rostedt Cc: Filipe Brandenburger, Li Zefan, Frederic Weisbecker, Ingo Molnar, linux-kernel On Mon, 3 Mar 2014 15:48:26 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > without patch: > > pst2pdf-3506 [000] 120.700910: sched_process_exec: filename=/usr/bin/pst2pdf pid=3506 old_pid=3506 > > > With patch: > > pstopnm-4432 [001] 1490.246765: sched_process_exec: filename= pid=4432 old_pid=4432 Now, after applying the following patch to your patch, I was able to get this again: pst2pdf-3512 [000] 99.693582: sched_process_exec: filename=/usr/bin/pst2pdf pid=3512 old_pid=3512 -- Steve diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h index 67e4122..2c6d1b0 100644 --- a/include/linux/ftrace_event.h +++ b/include/linux/ftrace_event.h @@ -124,8 +124,13 @@ struct trace_event { }; struct trace_array_offset { +#ifdef __BIG_ENDIAN + u16 length; + u16 offset; +#else u16 offset; u16 length; +#endif }; extern int register_ftrace_event(struct trace_event *event); ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-03-03 20:59 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-03-01 5:32 [PATCH 0/3] tracing: fix macro expansion and refactor some of dynamic_array support Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 1/3] tracing: correctly expand len expressions from __dynamic_array macro Filipe Brandenburger 2014-03-03 19:51 ` Steven Rostedt 2014-03-03 19:57 ` Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 2/3] tracing: evaluate len expression only once in " Filipe Brandenburger 2014-03-01 5:32 ` [PATCH 3/3] tracing: introduce a trace_data_offset struct to store array size Filipe Brandenburger 2014-03-03 20:48 ` Steven Rostedt 2014-03-03 20:59 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).