Messages in this thread | | | Date | Wed, 5 Jan 2022 19:00:04 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH] trace/osnoise: fix event unhooking |
| |
On Mon, 3 Jan 2022 10:06:50 +0100 Daniel Bristot de Oliveira <bristot@kernel.org> 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 <nikita.yushchenko@virtuozzo.com> > > --- > > 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
| |