* [PATCH] tracing/ftrace: add missing wake-up on some callsites @ 2009-02-22 21:56 Frederic Weisbecker 2009-02-22 22:16 ` Ingo Molnar 2009-02-23 15:37 ` Steven Rostedt 0 siblings, 2 replies; 14+ messages in thread From: Frederic Weisbecker @ 2009-02-22 21:56 UTC (permalink / raw) To: Ingo Molnar; +Cc: linux-kernel, Steven Rostedt, Arnaldo Carvalho de Melo Impact: fix unwaken pipe Now that we use a common wakeup infrastructure, we must append a wakeup on few callsites which lack it or tasks reading trace_pipe will not be awaken when events come on few tracers. Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> --- kernel/trace/trace.c | 2 ++ kernel/trace/trace_branch.c | 2 ++ kernel/trace/trace_hw_branches.c | 2 ++ 3 files changed, 6 insertions(+), 0 deletions(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index e1f3b99..7f450b6 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -3055,6 +3055,8 @@ int trace_vprintk(unsigned long ip, int depth, const char *fmt, va_list args) out: preempt_enable_notrace(); + trace_wake_up(); + return len; } EXPORT_SYMBOL_GPL(trace_vprintk); diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c index c2e68d4..8c8f8c0 100644 --- a/kernel/trace/trace_branch.c +++ b/kernel/trace/trace_branch.c @@ -78,6 +78,8 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect) out: atomic_dec(&tr->data[cpu]->disabled); local_irq_restore(flags); + + trace_wake_up(); } static inline diff --git a/kernel/trace/trace_hw_branches.c b/kernel/trace/trace_hw_branches.c index 3561aac..ddd87fd 100644 --- a/kernel/trace/trace_hw_branches.c +++ b/kernel/trace/trace_hw_branches.c @@ -202,6 +202,8 @@ void trace_hw_branch(u64 from, u64 to) out: atomic_dec(&tr->data[cpu]->disabled); local_irq_restore(irq1); + + trace_wake_up(); } static void trace_bts_at(const struct bts_trace *trace, void *at) -- 1.6.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-22 21:56 [PATCH] tracing/ftrace: add missing wake-up on some callsites Frederic Weisbecker @ 2009-02-22 22:16 ` Ingo Molnar 2009-02-23 15:37 ` Steven Rostedt 1 sibling, 0 replies; 14+ messages in thread From: Ingo Molnar @ 2009-02-22 22:16 UTC (permalink / raw) To: Frederic Weisbecker Cc: linux-kernel, Steven Rostedt, Arnaldo Carvalho de Melo * Frederic Weisbecker <fweisbec@gmail.com> wrote: > Impact: fix unwaken pipe > > Now that we use a common wakeup infrastructure, we must append a wakeup > on few callsites which lack it or tasks reading trace_pipe will not be > awaken when events come on few tracers. > > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > --- > kernel/trace/trace.c | 2 ++ > kernel/trace/trace_branch.c | 2 ++ > kernel/trace/trace_hw_branches.c | 2 ++ > 3 files changed, 6 insertions(+), 0 deletions(-) Applied to tip:tracing/ftrace, thanks Frederic! Ingo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-22 21:56 [PATCH] tracing/ftrace: add missing wake-up on some callsites Frederic Weisbecker 2009-02-22 22:16 ` Ingo Molnar @ 2009-02-23 15:37 ` Steven Rostedt 2009-02-23 15:42 ` Steven Rostedt 2009-02-23 16:22 ` Frederic Weisbecker 1 sibling, 2 replies; 14+ messages in thread From: Steven Rostedt @ 2009-02-23 15:37 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Ingo Molnar, LKML, Arnaldo Carvalho de Melo On Sun, 22 Feb 2009, Frederic Weisbecker wrote: > Impact: fix unwaken pipe > > Now that we use a common wakeup infrastructure, we must append a wakeup > on few callsites which lack it or tasks reading trace_pipe will not be > awaken when events come on few tracers. > > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > --- > kernel/trace/trace.c | 2 ++ > kernel/trace/trace_branch.c | 2 ++ > kernel/trace/trace_hw_branches.c | 2 ++ > 3 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index e1f3b99..7f450b6 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -3055,6 +3055,8 @@ int trace_vprintk(unsigned long ip, int depth, const char *fmt, va_list args) > out: > preempt_enable_notrace(); > > + trace_wake_up(); > + > return len; > } > EXPORT_SYMBOL_GPL(trace_vprintk); > diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c > index c2e68d4..8c8f8c0 100644 > --- a/kernel/trace/trace_branch.c > +++ b/kernel/trace/trace_branch.c > @@ -78,6 +78,8 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect) > out: > atomic_dec(&tr->data[cpu]->disabled); > local_irq_restore(flags); > + > + trace_wake_up(); > } > > static inline > diff --git a/kernel/trace/trace_hw_branches.c b/kernel/trace/trace_hw_branches.c > index 3561aac..ddd87fd 100644 > --- a/kernel/trace/trace_hw_branches.c > +++ b/kernel/trace/trace_hw_branches.c > @@ -202,6 +202,8 @@ void trace_hw_branch(u64 from, u64 to) > out: > atomic_dec(&tr->data[cpu]->disabled); > local_irq_restore(irq1); > + > + trace_wake_up(); > } > > static void trace_bts_at(const struct bts_trace *trace, void *at) Ah, we don't wake up purposely on these three places. ftrace_printk is meant to be called anywhere (including the scheduler). And the branch tracers are also allowed to be called anywhere (they usually are). Calling "wake_up" from any of these can easily cause a dead lock with the run queue lock, because all three can be called from with in the scheduler. Sorry, but I have to NACK this change. -- Steve ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 15:37 ` Steven Rostedt @ 2009-02-23 15:42 ` Steven Rostedt 2009-02-23 16:26 ` Frederic Weisbecker 2009-02-23 16:22 ` Frederic Weisbecker 1 sibling, 1 reply; 14+ messages in thread From: Steven Rostedt @ 2009-02-23 15:42 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Ingo Molnar, LKML, Arnaldo Carvalho de Melo On Mon, 23 Feb 2009, Steven Rostedt wrote: > > > On Sun, 22 Feb 2009, Frederic Weisbecker wrote: > > > Impact: fix unwaken pipe > > > > Now that we use a common wakeup infrastructure, we must append a wakeup > > on few callsites which lack it or tasks reading trace_pipe will not be > > awaken when events come on few tracers. > > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > > --- > > kernel/trace/trace.c | 2 ++ > > kernel/trace/trace_branch.c | 2 ++ > > kernel/trace/trace_hw_branches.c | 2 ++ > > 3 files changed, 6 insertions(+), 0 deletions(-) > > > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index e1f3b99..7f450b6 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -3055,6 +3055,8 @@ int trace_vprintk(unsigned long ip, int depth, const char *fmt, va_list args) > > out: > > preempt_enable_notrace(); > > > > + trace_wake_up(); > > + > > return len; > > } > > EXPORT_SYMBOL_GPL(trace_vprintk); > > diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c > > index c2e68d4..8c8f8c0 100644 > > --- a/kernel/trace/trace_branch.c > > +++ b/kernel/trace/trace_branch.c > > @@ -78,6 +78,8 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect) > > out: > > atomic_dec(&tr->data[cpu]->disabled); > > local_irq_restore(flags); > > + > > + trace_wake_up(); > > } > > > > static inline > > diff --git a/kernel/trace/trace_hw_branches.c b/kernel/trace/trace_hw_branches.c > > index 3561aac..ddd87fd 100644 > > --- a/kernel/trace/trace_hw_branches.c > > +++ b/kernel/trace/trace_hw_branches.c > > @@ -202,6 +202,8 @@ void trace_hw_branch(u64 from, u64 to) > > out: > > atomic_dec(&tr->data[cpu]->disabled); > > local_irq_restore(irq1); > > + > > + trace_wake_up(); > > } > > > > static void trace_bts_at(const struct bts_trace *trace, void *at) > > Ah, we don't wake up purposely on these three places. ftrace_printk is > meant to be called anywhere (including the scheduler). And the branch > tracers are also allowed to be called anywhere (they usually are). > > Calling "wake_up" from any of these can easily cause a dead lock with the > run queue lock, because all three can be called from with in the > scheduler. > > Sorry, but I have to NACK this change. Perhaps we could add these callsites back, but we would need to update trace_wake_up. Have trace_wake_up set a flag instead, and add a tracepoint around the scheduler (outside the grabbing of runqueue locks), that will have a callback to the tracing code. That call back can perform the wakeups. How does that sound? -- Steve ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 15:42 ` Steven Rostedt @ 2009-02-23 16:26 ` Frederic Weisbecker 2009-02-23 16:51 ` Steven Rostedt 0 siblings, 1 reply; 14+ messages in thread From: Frederic Weisbecker @ 2009-02-23 16:26 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, LKML, Arnaldo Carvalho de Melo On Mon, Feb 23, 2009 at 10:42:04AM -0500, Steven Rostedt wrote: > > On Mon, 23 Feb 2009, Steven Rostedt wrote: > > > > > > > On Sun, 22 Feb 2009, Frederic Weisbecker wrote: > > > > > Impact: fix unwaken pipe > > > > > > Now that we use a common wakeup infrastructure, we must append a wakeup > > > on few callsites which lack it or tasks reading trace_pipe will not be > > > awaken when events come on few tracers. > > > > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > > > --- > > > kernel/trace/trace.c | 2 ++ > > > kernel/trace/trace_branch.c | 2 ++ > > > kernel/trace/trace_hw_branches.c | 2 ++ > > > 3 files changed, 6 insertions(+), 0 deletions(-) > > > > > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > > index e1f3b99..7f450b6 100644 > > > --- a/kernel/trace/trace.c > > > +++ b/kernel/trace/trace.c > > > @@ -3055,6 +3055,8 @@ int trace_vprintk(unsigned long ip, int depth, const char *fmt, va_list args) > > > out: > > > preempt_enable_notrace(); > > > > > > + trace_wake_up(); > > > + > > > return len; > > > } > > > EXPORT_SYMBOL_GPL(trace_vprintk); > > > diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c > > > index c2e68d4..8c8f8c0 100644 > > > --- a/kernel/trace/trace_branch.c > > > +++ b/kernel/trace/trace_branch.c > > > @@ -78,6 +78,8 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect) > > > out: > > > atomic_dec(&tr->data[cpu]->disabled); > > > local_irq_restore(flags); > > > + > > > + trace_wake_up(); > > > } > > > > > > static inline > > > diff --git a/kernel/trace/trace_hw_branches.c b/kernel/trace/trace_hw_branches.c > > > index 3561aac..ddd87fd 100644 > > > --- a/kernel/trace/trace_hw_branches.c > > > +++ b/kernel/trace/trace_hw_branches.c > > > @@ -202,6 +202,8 @@ void trace_hw_branch(u64 from, u64 to) > > > out: > > > atomic_dec(&tr->data[cpu]->disabled); > > > local_irq_restore(irq1); > > > + > > > + trace_wake_up(); > > > } > > > > > > static void trace_bts_at(const struct bts_trace *trace, void *at) > > > > Ah, we don't wake up purposely on these three places. ftrace_printk is > > meant to be called anywhere (including the scheduler). And the branch > > tracers are also allowed to be called anywhere (they usually are). > > > > Calling "wake_up" from any of these can easily cause a dead lock with the > > run queue lock, because all three can be called from with in the > > scheduler. > > > > Sorry, but I have to NACK this change. > > > Perhaps we could add these callsites back, but we would need to update > trace_wake_up. > > Have trace_wake_up set a flag instead, and add a tracepoint around the > scheduler (outside the grabbing of runqueue locks), that will have a > callback to the tracing code. That call back can perform the wakeups. > > How does that sound? That sounds good but only for these particular tracers I guess. > -- Steve > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 16:26 ` Frederic Weisbecker @ 2009-02-23 16:51 ` Steven Rostedt 2009-02-23 17:05 ` Frederic Weisbecker 0 siblings, 1 reply; 14+ messages in thread From: Steven Rostedt @ 2009-02-23 16:51 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Ingo Molnar, LKML, Arnaldo Carvalho de Melo On Mon, 23 Feb 2009, Frederic Weisbecker wrote: > > > > Perhaps we could add these callsites back, but we would need to update > > trace_wake_up. > > > > Have trace_wake_up set a flag instead, and add a tracepoint around the > > scheduler (outside the grabbing of runqueue locks), that will have a > > callback to the tracing code. That call back can perform the wakeups. > > > > How does that sound? > > > That sounds good but only for these particular tracers I guess. OK, what about making a trace_delay_wake_up()? -- Steve ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 16:51 ` Steven Rostedt @ 2009-02-23 17:05 ` Frederic Weisbecker 2009-02-23 17:13 ` Steven Rostedt 0 siblings, 1 reply; 14+ messages in thread From: Frederic Weisbecker @ 2009-02-23 17:05 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, LKML, Arnaldo Carvalho de Melo On Mon, Feb 23, 2009 at 11:51:30AM -0500, Steven Rostedt wrote: > > On Mon, 23 Feb 2009, Frederic Weisbecker wrote: > > > > > > Perhaps we could add these callsites back, but we would need to update > > > trace_wake_up. > > > > > > Have trace_wake_up set a flag instead, and add a tracepoint around the > > > scheduler (outside the grabbing of runqueue locks), that will have a > > > callback to the tracing code. That call back can perform the wakeups. > > > > > > How does that sound? > > > > > > That sounds good but only for these particular tracers I guess. > > OK, what about making a trace_delay_wake_up()? Which would send a delayed work to wake up? > -- Steve > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 17:05 ` Frederic Weisbecker @ 2009-02-23 17:13 ` Steven Rostedt 2009-02-23 17:34 ` Frederic Weisbecker 2009-02-23 17:37 ` Ingo Molnar 0 siblings, 2 replies; 14+ messages in thread From: Steven Rostedt @ 2009-02-23 17:13 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Ingo Molnar, LKML, Arnaldo Carvalho de Melo On Mon, 23 Feb 2009, Frederic Weisbecker wrote: > On Mon, Feb 23, 2009 at 11:51:30AM -0500, Steven Rostedt wrote: > > > > On Mon, 23 Feb 2009, Frederic Weisbecker wrote: > > > > > > > > Perhaps we could add these callsites back, but we would need to update > > > > trace_wake_up. > > > > > > > > Have trace_wake_up set a flag instead, and add a tracepoint around the > > > > scheduler (outside the grabbing of runqueue locks), that will have a > > > > callback to the tracing code. That call back can perform the wakeups. > > > > > > > > How does that sound? > > > > > > > > > That sounds good but only for these particular tracers I guess. > > > > OK, what about making a trace_delay_wake_up()? > > > Which would send a delayed work to wake up? No, I was thinking that trace_delay_wake_up() would be called by these dangerous call sites. Then a per_cpu flag could be set. We could have a trace point in the scheduler code that is outside holding a runqueue lock, and this trace point would call a trace function that will clear the per cpu flag, and then call trace_wake_up(). -- Steve ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 17:13 ` Steven Rostedt @ 2009-02-23 17:34 ` Frederic Weisbecker 2009-02-23 17:37 ` Ingo Molnar 1 sibling, 0 replies; 14+ messages in thread From: Frederic Weisbecker @ 2009-02-23 17:34 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, LKML, Arnaldo Carvalho de Melo On Mon, Feb 23, 2009 at 12:13:36PM -0500, Steven Rostedt wrote: > > On Mon, 23 Feb 2009, Frederic Weisbecker wrote: > > > On Mon, Feb 23, 2009 at 11:51:30AM -0500, Steven Rostedt wrote: > > > > > > On Mon, 23 Feb 2009, Frederic Weisbecker wrote: > > > > > > > > > > Perhaps we could add these callsites back, but we would need to update > > > > > trace_wake_up. > > > > > > > > > > Have trace_wake_up set a flag instead, and add a tracepoint around the > > > > > scheduler (outside the grabbing of runqueue locks), that will have a > > > > > callback to the tracing code. That call back can perform the wakeups. > > > > > > > > > > How does that sound? > > > > > > > > > > > > That sounds good but only for these particular tracers I guess. > > > > > > OK, what about making a trace_delay_wake_up()? > > > > > > Which would send a delayed work to wake up? > > No, I was thinking that trace_delay_wake_up() would be called by these > dangerous call sites. Then a per_cpu flag could be set. We could have a > trace point in the scheduler code that is outside holding a runqueue lock, > and this trace point would call a trace function that will clear the per > cpu flag, and then call trace_wake_up(). > Oh yes, sounds nice! > -- Steve > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 17:13 ` Steven Rostedt 2009-02-23 17:34 ` Frederic Weisbecker @ 2009-02-23 17:37 ` Ingo Molnar 2009-02-23 18:03 ` Steven Rostedt 1 sibling, 1 reply; 14+ messages in thread From: Ingo Molnar @ 2009-02-23 17:37 UTC (permalink / raw) To: Steven Rostedt, Peter Zijlstra Cc: Frederic Weisbecker, LKML, Arnaldo Carvalho de Melo * Steven Rostedt <rostedt@goodmis.org> wrote: > > Which would send a delayed work to wake up? > > No, I was thinking that trace_delay_wake_up() would be called > by these dangerous call sites. Then a per_cpu flag could be > set. We could have a trace point in the scheduler code that is > outside holding a runqueue lock, and this trace point would > call a trace function that will clear the per cpu flag, and > then call trace_wake_up(). No, that's very roundabout and ugly. If we putting a tracepoint there we might as well put real scheduler code there that looks for such a flag. But i'm not convinced we need a flag ... Ingo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 17:37 ` Ingo Molnar @ 2009-02-23 18:03 ` Steven Rostedt 2009-02-23 18:09 ` Ingo Molnar 2009-02-27 17:04 ` Masami Hiramatsu 0 siblings, 2 replies; 14+ messages in thread From: Steven Rostedt @ 2009-02-23 18:03 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Frederic Weisbecker, LKML, Arnaldo Carvalho de Melo On Mon, 23 Feb 2009, Ingo Molnar wrote: > > * Steven Rostedt <rostedt@goodmis.org> wrote: > > > > Which would send a delayed work to wake up? > > > > No, I was thinking that trace_delay_wake_up() would be called > > by these dangerous call sites. Then a per_cpu flag could be > > set. We could have a trace point in the scheduler code that is > > outside holding a runqueue lock, and this trace point would > > call a trace function that will clear the per cpu flag, and > > then call trace_wake_up(). > > No, that's very roundabout and ugly. If we putting a tracepoint > there we might as well put real scheduler code there that looks > for such a flag. But i'm not convinced we need a flag ... Just a suggestion. I was trying to keep the tracer from being an overhead. But what else would you suggest? Just having the scheduler call trace_wakeup? -- Steve ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 18:03 ` Steven Rostedt @ 2009-02-23 18:09 ` Ingo Molnar 2009-02-27 17:04 ` Masami Hiramatsu 1 sibling, 0 replies; 14+ messages in thread From: Ingo Molnar @ 2009-02-23 18:09 UTC (permalink / raw) To: Steven Rostedt Cc: Peter Zijlstra, Frederic Weisbecker, LKML, Arnaldo Carvalho de Melo * Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 23 Feb 2009, Ingo Molnar wrote: > > > * Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > Which would send a delayed work to wake up? > > > > > > No, I was thinking that trace_delay_wake_up() would be > > > called by these dangerous call sites. Then a per_cpu flag > > > could be set. We could have a trace point in the scheduler > > > code that is outside holding a runqueue lock, and this > > > trace point would call a trace function that will clear > > > the per cpu flag, and then call trace_wake_up(). > > > > No, that's very roundabout and ugly. If we putting a > > tracepoint there we might as well put real scheduler code > > there that looks for such a flag. But i'm not convinced we > > need a flag ... > > Just a suggestion. I was trying to keep the tracer from being > an overhead. But what else would you suggest? Just having the > scheduler call trace_wakeup? i think we could use a TIF flag to trigger a wakeup at the return-to-userspace (or return-from-IRQ) stage or so? Ingo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 18:03 ` Steven Rostedt 2009-02-23 18:09 ` Ingo Molnar @ 2009-02-27 17:04 ` Masami Hiramatsu 1 sibling, 0 replies; 14+ messages in thread From: Masami Hiramatsu @ 2009-02-27 17:04 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, Peter Zijlstra, Frederic Weisbecker, LKML, Arnaldo Carvalho de Melo, systemtap-ml Steven Rostedt wrote: > On Mon, 23 Feb 2009, Ingo Molnar wrote: > >> * Steven Rostedt <rostedt@goodmis.org> wrote: >> >>>> Which would send a delayed work to wake up? >>> No, I was thinking that trace_delay_wake_up() would be called >>> by these dangerous call sites. Then a per_cpu flag could be >>> set. We could have a trace point in the scheduler code that is >>> outside holding a runqueue lock, and this trace point would >>> call a trace function that will clear the per cpu flag, and >>> then call trace_wake_up(). >> No, that's very roundabout and ugly. If we putting a tracepoint >> there we might as well put real scheduler code there that looks >> for such a flag. But i'm not convinced we need a flag ... > > Just a suggestion. I was trying to keep the tracer from being an overhead. > But what else would you suggest? Just having the scheduler call > trace_wakeup? Actually, Systemtap(LTTng too?) also has same problem. Currently, we're using a periodical timer to wake the reader process up. I assume if we can put a tracepoint at the beginning of schedule(), we can share it. Thank you, > > -- Steve > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: mhiramat@redhat.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] tracing/ftrace: add missing wake-up on some callsites 2009-02-23 15:37 ` Steven Rostedt 2009-02-23 15:42 ` Steven Rostedt @ 2009-02-23 16:22 ` Frederic Weisbecker 1 sibling, 0 replies; 14+ messages in thread From: Frederic Weisbecker @ 2009-02-23 16:22 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, LKML, Arnaldo Carvalho de Melo On Mon, Feb 23, 2009 at 10:37:52AM -0500, Steven Rostedt wrote: > > > On Sun, 22 Feb 2009, Frederic Weisbecker wrote: > > > Impact: fix unwaken pipe > > > > Now that we use a common wakeup infrastructure, we must append a wakeup > > on few callsites which lack it or tasks reading trace_pipe will not be > > awaken when events come on few tracers. > > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > > --- > > kernel/trace/trace.c | 2 ++ > > kernel/trace/trace_branch.c | 2 ++ > > kernel/trace/trace_hw_branches.c | 2 ++ > > 3 files changed, 6 insertions(+), 0 deletions(-) > > > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index e1f3b99..7f450b6 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -3055,6 +3055,8 @@ int trace_vprintk(unsigned long ip, int depth, const char *fmt, va_list args) > > out: > > preempt_enable_notrace(); > > > > + trace_wake_up(); > > + > > return len; > > } > > EXPORT_SYMBOL_GPL(trace_vprintk); > > diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c > > index c2e68d4..8c8f8c0 100644 > > --- a/kernel/trace/trace_branch.c > > +++ b/kernel/trace/trace_branch.c > > @@ -78,6 +78,8 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect) > > out: > > atomic_dec(&tr->data[cpu]->disabled); > > local_irq_restore(flags); > > + > > + trace_wake_up(); > > } > > > > static inline > > diff --git a/kernel/trace/trace_hw_branches.c b/kernel/trace/trace_hw_branches.c > > index 3561aac..ddd87fd 100644 > > --- a/kernel/trace/trace_hw_branches.c > > +++ b/kernel/trace/trace_hw_branches.c > > @@ -202,6 +202,8 @@ void trace_hw_branch(u64 from, u64 to) > > out: > > atomic_dec(&tr->data[cpu]->disabled); > > local_irq_restore(irq1); > > + > > + trace_wake_up(); > > } > > > > static void trace_bts_at(const struct bts_trace *trace, void *at) > > Ah, we don't wake up purposely on these three places. ftrace_printk is > meant to be called anywhere (including the scheduler). And the branch > tracers are also allowed to be called anywhere (they usually are). > > Calling "wake_up" from any of these can easily cause a dead lock with the > run queue lock, because all three can be called from with in the > scheduler. > > Sorry, but I have to NACK this change. And fortunately you NACK, I didn't realized how dangerous it can be. I will send a patch to make these branch tracers use the old polling wake up. But another solution should be found for ftrace_printk() since it doesn't necessarily rely on any tracer. > -- Steve ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-02-27 17:03 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-02-22 21:56 [PATCH] tracing/ftrace: add missing wake-up on some callsites Frederic Weisbecker 2009-02-22 22:16 ` Ingo Molnar 2009-02-23 15:37 ` Steven Rostedt 2009-02-23 15:42 ` Steven Rostedt 2009-02-23 16:26 ` Frederic Weisbecker 2009-02-23 16:51 ` Steven Rostedt 2009-02-23 17:05 ` Frederic Weisbecker 2009-02-23 17:13 ` Steven Rostedt 2009-02-23 17:34 ` Frederic Weisbecker 2009-02-23 17:37 ` Ingo Molnar 2009-02-23 18:03 ` Steven Rostedt 2009-02-23 18:09 ` Ingo Molnar 2009-02-27 17:04 ` Masami Hiramatsu 2009-02-23 16:22 ` Frederic Weisbecker
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.