Re: [PATCH v2 3/9] tracing/osnoise: Per-cpu mutexes and fd detachment

[sashiko-bot@kernel.org]

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 <crwood@redhat.com>

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/20261010022338.2507589-1-crwood@redhat.com?part=3