If start_per_cpu_kthreads() called from osnoise_workload_start() returns
error, event hooks are left in broken state: unhook_irq_events() called
but unhook_thread_events() and unhook_softirq_events() not called, and
trace_osnoise_callback_enabled flag not cleared.
On the next tracer enable, hooks get not installed due to
trace_osnoise_callback_enabled flag.
And on the further tracer disable an attempt to remove non-installed
hooks happened, hitting a WARN_ON_ONCE() in tracepoint_remove_func().
Fix the error path by adding the missing part of cleanup.
While at this, introduce osnoise_unhook_events() to avoid code
duplication between this error path and notmal tracer disable.
Fixes: bce29ac9ce0b ("trace: Add osnoise tracer")
Signed-off-by: Nikita Yushchenko <[email protected]>
---
kernel/trace/trace_osnoise.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 7520d43aed55..aa6f26612ccc 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -2123,6 +2123,13 @@ static int osnoise_hook_events(void)
return -EINVAL;
}
+static void osnoise_unhook_events(void)
+{
+ unhook_thread_events();
+ unhook_softirq_events();
+ unhook_irq_events();
+}
+
/*
* osnoise_workload_start - start the workload and hook to events
*/
@@ -2155,7 +2162,8 @@ static int osnoise_workload_start(void)
retval = start_per_cpu_kthreads();
if (retval) {
- unhook_irq_events();
+ trace_osnoise_callback_enabled = false;
+ osnoise_unhook_events();
return retval;
}
@@ -2186,9 +2194,7 @@ static void osnoise_workload_stop(void)
stop_per_cpu_kthreads();
- unhook_irq_events();
- unhook_softirq_events();
- unhook_thread_events();
+ osnoise_unhook_events();
}
static void osnoise_tracer_start(struct trace_array *tr)
--
2.30.2
Hi Nikita
On 12/28/21 15:07, Nikita Yushchenko wrote:
> If start_per_cpu_kthreads() called from osnoise_workload_start() returns
> error, event hooks are left in broken state: unhook_irq_events() called
> but unhook_thread_events() and unhook_softirq_events() not called, and
> trace_osnoise_callback_enabled flag not cleared.
>
> On the next tracer enable, hooks get not installed due to
> trace_osnoise_callback_enabled flag.
>
> And on the further tracer disable an attempt to remove non-installed
> hooks happened, hitting a WARN_ON_ONCE() in tracepoint_remove_func().
>
> Fix the error path by adding the missing part of cleanup.
Regarding the subject:
- use tracing/ as subsystem (yeah, I also made this mistake in the original
osnoise series).
- use a capital after the "tracing/osnoise:"
Using your subject as example, it should be:
tracing/osnoise: Fix event unhooking
Anyway, I suggest using something more precise, like:
tracing/osnoise: Properly unhook events if start_per_cpu_kthreads() fails
or something like that.
> While at this, introduce osnoise_unhook_events() to avoid code
> duplication between this error path and notmal tracer disable.
s/notmal/normal/
>
> Fixes: bce29ac9ce0b ("trace: Add osnoise tracer")
> Signed-off-by: Nikita Yushchenko <[email protected]>
> ---
> kernel/trace/trace_osnoise.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index 7520d43aed55..aa6f26612ccc 100644
> --- a/kernel/trace/trace_osnoise.c
> +++ b/kernel/trace/trace_osnoise.c
> @@ -2123,6 +2123,13 @@ static int osnoise_hook_events(void)
> return -EINVAL;
> }
>
> +static void osnoise_unhook_events(void)
> +{
> + unhook_thread_events();
> + unhook_softirq_events();
> + unhook_irq_events();
> +}
> +
> /*
> * osnoise_workload_start - start the workload and hook to events
> */
> @@ -2155,7 +2162,8 @@ static int osnoise_workload_start(void)
>
> retval = start_per_cpu_kthreads();
> if (retval) {
> - unhook_irq_events();
> + trace_osnoise_callback_enabled = false;
we need a barrier here, like:
/*
* Make sure that ftrace_nmi_enter/exit() see
* trace_osnoise_callback_enabled as false before continuing.
*/
barrier();
> + osnoise_unhook_events();
> return retval;
> }
>
Thanks!
-- Daniel
On Mon, 3 Jan 2022 10:06:50 +0100
Daniel Bristot de Oliveira <[email protected]> wrote:
> Hi Nikita
>
> On 12/28/21 15:07, Nikita Yushchenko wrote:
> > If start_per_cpu_kthreads() called from osnoise_workload_start() returns
> > error, event hooks are left in broken state: unhook_irq_events() called
> > but unhook_thread_events() and unhook_softirq_events() not called, and
> > trace_osnoise_callback_enabled flag not cleared.
> >
> > On the next tracer enable, hooks get not installed due to
> > trace_osnoise_callback_enabled flag.
> >
> > And on the further tracer disable an attempt to remove non-installed
> > hooks happened, hitting a WARN_ON_ONCE() in tracepoint_remove_func().
> >
> > Fix the error path by adding the missing part of cleanup.
>
> Regarding the subject:
>
> - use tracing/ as subsystem (yeah, I also made this mistake in the original
> osnoise series).
> - use a capital after the "tracing/osnoise:"
>
> Using your subject as example, it should be:
> tracing/osnoise: Fix event unhooking
Thanks for mentioning this, as was going to.
>
> Anyway, I suggest using something more precise, like:
>
> tracing/osnoise: Properly unhook events if start_per_cpu_kthreads() fails
>
> or something like that.
>
> > While at this, introduce osnoise_unhook_events() to avoid code
> > duplication between this error path and notmal tracer disable.
>
> s/notmal/normal/
>
> >
> > Fixes: bce29ac9ce0b ("trace: Add osnoise tracer")
> > Signed-off-by: Nikita Yushchenko <[email protected]>
> > ---
> > kernel/trace/trace_osnoise.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> > index 7520d43aed55..aa6f26612ccc 100644
> > --- a/kernel/trace/trace_osnoise.c
> > +++ b/kernel/trace/trace_osnoise.c
> > @@ -2123,6 +2123,13 @@ static int osnoise_hook_events(void)
> > return -EINVAL;
> > }
> >
> > +static void osnoise_unhook_events(void)
> > +{
> > + unhook_thread_events();
> > + unhook_softirq_events();
> > + unhook_irq_events();
> > +}
> > +
> > /*
> > * osnoise_workload_start - start the workload and hook to events
> > */
> > @@ -2155,7 +2162,8 @@ static int osnoise_workload_start(void)
> >
> > retval = start_per_cpu_kthreads();
> > if (retval) {
> > - unhook_irq_events();
> > + trace_osnoise_callback_enabled = false;
>
> we need a barrier here, like:
>
> /*
> * Make sure that ftrace_nmi_enter/exit() see
> * trace_osnoise_callback_enabled as false before continuing.
> */
> barrier();
Nikita, are you going to send a v2?
-- Steve