Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] tracing/osnoise: sampling state is never cleared when no-workload tracer stops --
commit f0d962e5159ec760bce6edd22ecbeb40abce0874 Author: Crystal Wood <[email protected]> tracing/osnoise: Per-cpu mutexes and fd detachment This commit cleans up synchronization issues by introducing per-cpu mutexes for guarding kthread changes and fd open/close/revoke operations. It also replaces a SIGKILL hack for userspace timerlat threads with a mutex-protected detachment mechanism. > diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c > --- a/kernel/trace/trace_osnoise.c > +++ b/kernel/trace/trace_osnoise.c [ ... ] > @@ -1976,32 +2008,23 @@ static int timerlat_main(void *data) > */ > static void stop_kthread(unsigned int cpu) > { > - struct task_struct *kthread; > + struct osnoise_variables *osn_var = cpu_osn_var(cpu); > > - kthread = xchg_relaxed(&cpu_osn_var(cpu)->kthread, NULL); > - if (kthread) { > - if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask) && > - !WARN_ON(!test_bit(OSN_WORKLOAD, &osnoise_options))) { > - kthread_stop(kthread); > - } else if (!WARN_ON(test_bit(OSN_WORKLOAD, &osnoise_options))) { > - /* > - * This is a user thread waiting on the timerlat_fd. We > need > - * to close all users, and the best way to guarantee > this is > - * by killing the thread. NOTE: this is a purpose > specific file. > - */ > - kill_pid(kthread->thread_pid, SIGKILL, 1); > - } > - put_task_struct(kthread); > - } else { > - /* if no workload, just return */ > - if (!test_bit(OSN_WORKLOAD, &osnoise_options)) { > - /* > - * This is set in the osnoise tracer case. > - */ > - cpu_osn_var(cpu)->sampling = false; > - barrier(); > - } > + mutex_lock(&osn_var->lock); > + > + if (osn_var->fdd) { > + WARN_ON(test_bit(OSN_WORKLOAD, &osnoise_options)); > + WARN_ON(osn_var->kthread); > + osn_var->fdd->detach(osn_var->fdd); > + } else if (osn_var->kthread) { > + WARN_ON(!cpumask_test_and_clear_cpu(cpu, &kthread_cpumask)); > + WARN_ON(!test_bit(OSN_WORKLOAD, &osnoise_options)); > + kthread_stop(osn_var->kthread); > + put_task_struct(osn_var->kthread); > + osn_var->kthread = NULL; > } > + > + mutex_unlock(&osn_var->lock); > } [Severity: Medium] Does this removal leave the sampling state permanently enabled when the tracer is stopped in the no-workload configuration? When the osnoise tracer starts without a workload (!OSN_WORKLOAD), start_kthread() sets osn_var->sampling = true but exits without spawning a kthread. Because both osn_var->kthread and osn_var->fdd remain NULL in this mode, this new logic in stop_kthread() appears to do nothing. The previous code had an else block that explicitly reset the sampling flag: } else { /* if no workload, just return */ if (!test_bit(OSN_WORKLOAD, &osnoise_options)) { /* * This is set in the osnoise tracer case. */ cpu_osn_var(cpu)->sampling = false; barrier(); } } Without this fallback, the sampling flag remains permanently true. This could cause tracepoint callbacks (like NMI, IRQ, and softirq) to continue incurring execution overhead and recording events into the ring buffer even after the tracer is stopped, eventually triggering warnings in osn_var_reset() upon subsequent starts. Could we restore the cleanup of the sampling flag for the no-workload case? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
