* [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and latency tracers
@ 2013-03-15 5:06 Steven Rostedt
2013-03-15 5:06 ` [PATCH 1/3 v2] tracing: Protect tracer flags with trace_types_lock Steven Rostedt
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Steven Rostedt @ 2013-03-15 5:06 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker
[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]
Ingo,
Here's take two of the last patch series I sent. The only difference
with this series is that I encapsulate the max_tr reference with
the #ifdef CONFIG_TRACER_MAX_TRACE. Other than that, it's the same.
Here's the recap:
The first patch fixes a long standing bug where the changing of
the trace options has absolutely no protection against multiple
changers. Luckily, this is something that requires human (root)
action, and is usually performed by a single individual. But as
scripts are starting to become more active, this bug is bound to
be triggered.
The second patch is a bug where the setting of the overwrite flag
can cause the max and normal buffers to get out of sync. This
is more predomitinant with the 3.9 snapshot feature, but still
has an affect on the latency tracers. Which bring us to the third
patch.
The third patch I found out that disabling overwrite to the buffers
screws with the latency tracers. If the buffer is in produce consumer
mode, the irqsoff tracer and friends wont show any output at all.
This was a bit frustrating to figure out.
Hopefully this time it's good.
Thanks,
-- Steve
Please pull the latest tip/perf/urgent-2 tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/perf/urgent-2
Head SHA1: 613f04a0f51e6e68ac6fe571ab79da3c0a5eb4da
Steven Rostedt (Red Hat) (3):
tracing: Protect tracer flags with trace_types_lock
tracing: Keep overwrite in sync between regular and snapshot buffers
tracing: Prevent buffer overwrite disabled for latency tracers
----
kernel/trace/trace.c | 56 +++++++++++++++++++++++++++++--------
kernel/trace/trace.h | 6 ++++
kernel/trace/trace_irqsoff.c | 19 +++++++++----
kernel/trace/trace_sched_wakeup.c | 18 ++++++++----
4 files changed, 78 insertions(+), 21 deletions(-)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/3 v2] tracing: Protect tracer flags with trace_types_lock 2013-03-15 5:06 [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and latency tracers Steven Rostedt @ 2013-03-15 5:06 ` Steven Rostedt 2013-03-15 5:06 ` [PATCH 2/3 v2] tracing: Keep overwrite in sync between regular and snapshot buffers Steven Rostedt ` (2 subsequent siblings) 3 siblings, 0 replies; 5+ messages in thread From: Steven Rostedt @ 2013-03-15 5:06 UTC (permalink / raw) To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, stable [-- Attachment #1: Type: text/plain, Size: 1682 bytes --] From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org> Seems that the tracer flags have never been protected from synchronous writes. Luckily, admins don't usually modify the tracing flags via two different tasks. But if scripts were to be used to modify them, then they could get corrupted. Move the trace_types_lock that protects against tracers changing to also protect the flags being set. Cc: stable@vger.kernel.org Signed-off-by: Steven Rostedt <rostedt@goodmis.org> --- kernel/trace/trace.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 53df283..00daf5f 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -2916,6 +2916,8 @@ static int trace_set_options(char *option) cmp += 2; } + mutex_lock(&trace_types_lock); + for (i = 0; trace_options[i]; i++) { if (strcmp(cmp, trace_options[i]) == 0) { set_tracer_flags(1 << i, !neg); @@ -2924,11 +2926,10 @@ static int trace_set_options(char *option) } /* If no option could be set, test the specific tracer options */ - if (!trace_options[i]) { - mutex_lock(&trace_types_lock); + if (!trace_options[i]) ret = set_tracer_option(current_trace, cmp, neg); - mutex_unlock(&trace_types_lock); - } + + mutex_unlock(&trace_types_lock); return ret; } @@ -4781,7 +4782,10 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt, if (val != 0 && val != 1) return -EINVAL; + + mutex_lock(&trace_types_lock); set_tracer_flags(1 << index, val); + mutex_unlock(&trace_types_lock); *ppos += cnt; -- 1.7.10.4 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3 v2] tracing: Keep overwrite in sync between regular and snapshot buffers 2013-03-15 5:06 [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and latency tracers Steven Rostedt 2013-03-15 5:06 ` [PATCH 1/3 v2] tracing: Protect tracer flags with trace_types_lock Steven Rostedt @ 2013-03-15 5:06 ` Steven Rostedt 2013-03-15 5:06 ` [PATCH 3/3 v2] tracing: Prevent buffer overwrite disabled for latency tracers Steven Rostedt 2013-03-18 8:49 ` [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and " Ingo Molnar 3 siblings, 0 replies; 5+ messages in thread From: Steven Rostedt @ 2013-03-15 5:06 UTC (permalink / raw) To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, stable [-- Attachment #1: Type: text/plain, Size: 1238 bytes --] From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org> Changing the overwrite mode for the ring buffer via the trace option only sets the normal buffer. But the snapshot buffer could swap with it, and then the snapshot would be in non overwrite mode and the normal buffer would be in overwrite mode, even though the option flag states otherwise. Keep the two buffers overwrite modes in sync. Cc: stable@vger.kernel.org Signed-off-by: Steven Rostedt <rostedt@goodmis.org> --- kernel/trace/trace.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 00daf5f..02debab 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -2895,8 +2895,12 @@ static void set_tracer_flags(unsigned int mask, int enabled) if (mask == TRACE_ITER_RECORD_CMD) trace_event_enable_cmd_record(enabled); - if (mask == TRACE_ITER_OVERWRITE) + if (mask == TRACE_ITER_OVERWRITE) { ring_buffer_change_overwrite(global_trace.buffer, enabled); +#ifdef CONFIG_TRACER_MAX_TRACE + ring_buffer_change_overwrite(max_tr.buffer, enabled); +#endif + } if (mask == TRACE_ITER_PRINTK) trace_printk_start_stop_comm(enabled); -- 1.7.10.4 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3 v2] tracing: Prevent buffer overwrite disabled for latency tracers 2013-03-15 5:06 [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and latency tracers Steven Rostedt 2013-03-15 5:06 ` [PATCH 1/3 v2] tracing: Protect tracer flags with trace_types_lock Steven Rostedt 2013-03-15 5:06 ` [PATCH 2/3 v2] tracing: Keep overwrite in sync between regular and snapshot buffers Steven Rostedt @ 2013-03-15 5:06 ` Steven Rostedt 2013-03-18 8:49 ` [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and " Ingo Molnar 3 siblings, 0 replies; 5+ messages in thread From: Steven Rostedt @ 2013-03-15 5:06 UTC (permalink / raw) To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, stable [-- Attachment #1: Type: text/plain, Size: 9231 bytes --] From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org> The latency tracers require the buffers to be in overwrite mode, otherwise they get screwed up. Force the buffers to stay in overwrite mode when latency tracers are enabled. Added a flag_changed() method to the tracer structure to allow the tracers to see what flags are being changed, and also be able to prevent the change from happing. Cc: stable@vger.kernel.org Signed-off-by: Steven Rostedt <rostedt@goodmis.org> --- kernel/trace/trace.c | 38 +++++++++++++++++++++++++++++++------ kernel/trace/trace.h | 6 ++++++ kernel/trace/trace_irqsoff.c | 19 ++++++++++++++----- kernel/trace/trace_sched_wakeup.c | 18 +++++++++++++----- 4 files changed, 65 insertions(+), 16 deletions(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 02debab..4f1dade 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -2881,11 +2881,25 @@ static int set_tracer_option(struct tracer *trace, char *cmp, int neg) return -EINVAL; } -static void set_tracer_flags(unsigned int mask, int enabled) +/* Some tracers require overwrite to stay enabled */ +int trace_keep_overwrite(struct tracer *tracer, u32 mask, int set) +{ + if (tracer->enabled && (mask & TRACE_ITER_OVERWRITE) && !set) + return -1; + + return 0; +} + +int set_tracer_flag(unsigned int mask, int enabled) { /* do nothing if flag is already set */ if (!!(trace_flags & mask) == !!enabled) - return; + return 0; + + /* Give the tracer a chance to approve the change */ + if (current_trace->flag_changed) + if (current_trace->flag_changed(current_trace, mask, !!enabled)) + return -EINVAL; if (enabled) trace_flags |= mask; @@ -2904,13 +2918,15 @@ static void set_tracer_flags(unsigned int mask, int enabled) if (mask == TRACE_ITER_PRINTK) trace_printk_start_stop_comm(enabled); + + return 0; } static int trace_set_options(char *option) { char *cmp; int neg = 0; - int ret = 0; + int ret = -ENODEV; int i; cmp = strstrip(option); @@ -2924,7 +2940,7 @@ static int trace_set_options(char *option) for (i = 0; trace_options[i]; i++) { if (strcmp(cmp, trace_options[i]) == 0) { - set_tracer_flags(1 << i, !neg); + ret = set_tracer_flag(1 << i, !neg); break; } } @@ -2943,6 +2959,7 @@ tracing_trace_options_write(struct file *filp, const char __user *ubuf, size_t cnt, loff_t *ppos) { char buf[64]; + int ret; if (cnt >= sizeof(buf)) return -EINVAL; @@ -2952,7 +2969,9 @@ tracing_trace_options_write(struct file *filp, const char __user *ubuf, buf[cnt] = 0; - trace_set_options(buf); + ret = trace_set_options(buf); + if (ret < 0) + return ret; *ppos += cnt; @@ -3256,6 +3275,9 @@ static int tracing_set_tracer(const char *buf) goto out; trace_branch_disable(); + + current_trace->enabled = false; + if (current_trace->reset) current_trace->reset(tr); @@ -3300,6 +3322,7 @@ static int tracing_set_tracer(const char *buf) } current_trace = t; + current_trace->enabled = true; trace_branch_enable(tr); out: mutex_unlock(&trace_types_lock); @@ -4788,9 +4811,12 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt, return -EINVAL; mutex_lock(&trace_types_lock); - set_tracer_flags(1 << index, val); + ret = set_tracer_flag(1 << index, val); mutex_unlock(&trace_types_lock); + if (ret < 0) + return ret; + *ppos += cnt; return cnt; diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index 57d7e53..2081971 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -283,11 +283,15 @@ struct tracer { enum print_line_t (*print_line)(struct trace_iterator *iter); /* If you handled the flag setting, return 0 */ int (*set_flag)(u32 old_flags, u32 bit, int set); + /* Return 0 if OK with change, else return non-zero */ + int (*flag_changed)(struct tracer *tracer, + u32 mask, int set); struct tracer *next; struct tracer_flags *flags; bool print_max; bool use_max_tr; bool allocated_snapshot; + bool enabled; }; @@ -943,6 +947,8 @@ extern const char *__stop___trace_bprintk_fmt[]; void trace_printk_init_buffers(void); void trace_printk_start_comm(void); +int trace_keep_overwrite(struct tracer *tracer, u32 mask, int set); +int set_tracer_flag(unsigned int mask, int enabled); #undef FTRACE_ENTRY #define FTRACE_ENTRY(call, struct_name, id, tstruct, print, filter) \ diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c index 713a2ca..443b25b 100644 --- a/kernel/trace/trace_irqsoff.c +++ b/kernel/trace/trace_irqsoff.c @@ -32,7 +32,7 @@ enum { static int trace_type __read_mostly; -static int save_lat_flag; +static int save_flags; static void stop_irqsoff_tracer(struct trace_array *tr, int graph); static int start_irqsoff_tracer(struct trace_array *tr, int graph); @@ -558,8 +558,11 @@ static void stop_irqsoff_tracer(struct trace_array *tr, int graph) static void __irqsoff_tracer_init(struct trace_array *tr) { - save_lat_flag = trace_flags & TRACE_ITER_LATENCY_FMT; - trace_flags |= TRACE_ITER_LATENCY_FMT; + save_flags = trace_flags; + + /* non overwrite screws up the latency tracers */ + set_tracer_flag(TRACE_ITER_OVERWRITE, 1); + set_tracer_flag(TRACE_ITER_LATENCY_FMT, 1); tracing_max_latency = 0; irqsoff_trace = tr; @@ -573,10 +576,13 @@ static void __irqsoff_tracer_init(struct trace_array *tr) static void irqsoff_tracer_reset(struct trace_array *tr) { + int lat_flag = save_flags & TRACE_ITER_LATENCY_FMT; + int overwrite_flag = save_flags & TRACE_ITER_OVERWRITE; + stop_irqsoff_tracer(tr, is_graph()); - if (!save_lat_flag) - trace_flags &= ~TRACE_ITER_LATENCY_FMT; + set_tracer_flag(TRACE_ITER_LATENCY_FMT, lat_flag); + set_tracer_flag(TRACE_ITER_OVERWRITE, overwrite_flag); } static void irqsoff_tracer_start(struct trace_array *tr) @@ -609,6 +615,7 @@ static struct tracer irqsoff_tracer __read_mostly = .print_line = irqsoff_print_line, .flags = &tracer_flags, .set_flag = irqsoff_set_flag, + .flag_changed = trace_keep_overwrite, #ifdef CONFIG_FTRACE_SELFTEST .selftest = trace_selftest_startup_irqsoff, #endif @@ -642,6 +649,7 @@ static struct tracer preemptoff_tracer __read_mostly = .print_line = irqsoff_print_line, .flags = &tracer_flags, .set_flag = irqsoff_set_flag, + .flag_changed = trace_keep_overwrite, #ifdef CONFIG_FTRACE_SELFTEST .selftest = trace_selftest_startup_preemptoff, #endif @@ -677,6 +685,7 @@ static struct tracer preemptirqsoff_tracer __read_mostly = .print_line = irqsoff_print_line, .flags = &tracer_flags, .set_flag = irqsoff_set_flag, + .flag_changed = trace_keep_overwrite, #ifdef CONFIG_FTRACE_SELFTEST .selftest = trace_selftest_startup_preemptirqsoff, #endif diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c index 75aa97f..fde652c 100644 --- a/kernel/trace/trace_sched_wakeup.c +++ b/kernel/trace/trace_sched_wakeup.c @@ -36,7 +36,7 @@ static void __wakeup_reset(struct trace_array *tr); static int wakeup_graph_entry(struct ftrace_graph_ent *trace); static void wakeup_graph_return(struct ftrace_graph_ret *trace); -static int save_lat_flag; +static int save_flags; #define TRACE_DISPLAY_GRAPH 1 @@ -540,8 +540,11 @@ static void stop_wakeup_tracer(struct trace_array *tr) static int __wakeup_tracer_init(struct trace_array *tr) { - save_lat_flag = trace_flags & TRACE_ITER_LATENCY_FMT; - trace_flags |= TRACE_ITER_LATENCY_FMT; + save_flags = trace_flags; + + /* non overwrite screws up the latency tracers */ + set_tracer_flag(TRACE_ITER_OVERWRITE, 1); + set_tracer_flag(TRACE_ITER_LATENCY_FMT, 1); tracing_max_latency = 0; wakeup_trace = tr; @@ -563,12 +566,15 @@ static int wakeup_rt_tracer_init(struct trace_array *tr) static void wakeup_tracer_reset(struct trace_array *tr) { + int lat_flag = save_flags & TRACE_ITER_LATENCY_FMT; + int overwrite_flag = save_flags & TRACE_ITER_OVERWRITE; + stop_wakeup_tracer(tr); /* make sure we put back any tasks we are tracing */ wakeup_reset(tr); - if (!save_lat_flag) - trace_flags &= ~TRACE_ITER_LATENCY_FMT; + set_tracer_flag(TRACE_ITER_LATENCY_FMT, lat_flag); + set_tracer_flag(TRACE_ITER_OVERWRITE, overwrite_flag); } static void wakeup_tracer_start(struct trace_array *tr) @@ -594,6 +600,7 @@ static struct tracer wakeup_tracer __read_mostly = .print_line = wakeup_print_line, .flags = &tracer_flags, .set_flag = wakeup_set_flag, + .flag_changed = trace_keep_overwrite, #ifdef CONFIG_FTRACE_SELFTEST .selftest = trace_selftest_startup_wakeup, #endif @@ -615,6 +622,7 @@ static struct tracer wakeup_rt_tracer __read_mostly = .print_line = wakeup_print_line, .flags = &tracer_flags, .set_flag = wakeup_set_flag, + .flag_changed = trace_keep_overwrite, #ifdef CONFIG_FTRACE_SELFTEST .selftest = trace_selftest_startup_wakeup, #endif -- 1.7.10.4 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and latency tracers 2013-03-15 5:06 [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and latency tracers Steven Rostedt ` (2 preceding siblings ...) 2013-03-15 5:06 ` [PATCH 3/3 v2] tracing: Prevent buffer overwrite disabled for latency tracers Steven Rostedt @ 2013-03-18 8:49 ` Ingo Molnar 3 siblings, 0 replies; 5+ messages in thread From: Ingo Molnar @ 2013-03-18 8:49 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel, Andrew Morton, Frederic Weisbecker * Steven Rostedt <rostedt@goodmis.org> wrote: > > Ingo, > > Here's take two of the last patch series I sent. The only difference > with this series is that I encapsulate the max_tr reference with > the #ifdef CONFIG_TRACER_MAX_TRACE. Other than that, it's the same. > > Here's the recap: > > The first patch fixes a long standing bug where the changing of > the trace options has absolutely no protection against multiple > changers. Luckily, this is something that requires human (root) > action, and is usually performed by a single individual. But as > scripts are starting to become more active, this bug is bound to > be triggered. > > The second patch is a bug where the setting of the overwrite flag > can cause the max and normal buffers to get out of sync. This > is more predomitinant with the 3.9 snapshot feature, but still > has an affect on the latency tracers. Which bring us to the third > patch. > > The third patch I found out that disabling overwrite to the buffers > screws with the latency tracers. If the buffer is in produce consumer > mode, the irqsoff tracer and friends wont show any output at all. > This was a bit frustrating to figure out. > > Hopefully this time it's good. > > Thanks, > > -- Steve > > Please pull the latest tip/perf/urgent-2 tree, which can be found at: > > git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git > tip/perf/urgent-2 > > Head SHA1: 613f04a0f51e6e68ac6fe571ab79da3c0a5eb4da > > > Steven Rostedt (Red Hat) (3): > tracing: Protect tracer flags with trace_types_lock > tracing: Keep overwrite in sync between regular and snapshot buffers > tracing: Prevent buffer overwrite disabled for latency tracers > > ---- > kernel/trace/trace.c | 56 +++++++++++++++++++++++++++++-------- > kernel/trace/trace.h | 6 ++++ > kernel/trace/trace_irqsoff.c | 19 +++++++++---- > kernel/trace/trace_sched_wakeup.c | 18 ++++++++---- > 4 files changed, 78 insertions(+), 21 deletions(-) Pulled, thanks Steve! Ingo ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-03-18 8:49 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-15 5:06 [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and latency tracers Steven Rostedt 2013-03-15 5:06 ` [PATCH 1/3 v2] tracing: Protect tracer flags with trace_types_lock Steven Rostedt 2013-03-15 5:06 ` [PATCH 2/3 v2] tracing: Keep overwrite in sync between regular and snapshot buffers Steven Rostedt 2013-03-15 5:06 ` [PATCH 3/3 v2] tracing: Prevent buffer overwrite disabled for latency tracers Steven Rostedt 2013-03-18 8:49 ` [PATCH 0/3 v2] [GIT PULL][3.9] tracing: Fixes with flags and " Ingo Molnar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox