Re: [PATCH v5 2/8] tracing/osnoise: Start all eligible cpus from hotplug workfn

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tracing/osnoise: osnoise_hotplug_workfn() skips sampling when !OSN_WORKLOAD
--

commit 4bedb2088cbb62deefeaad638901751b0c1e9f6d
Author: Crystal Wood <crwood@redhat.com>

tracing/osnoise: Start all eligible cpus from hotplug workfn

This commit updates osnoise_hotplug_workfn() to iterate over all online CPUs
in osnoise_cpumask and start their kthreads when scheduled from
osnoise_cpu_init(), avoiding lost starts if multiple CPUs come online before
the work item runs.

> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index aa764ae13047e..29fa4150b26c0 100644
> --- a/kernel/trace/trace_osnoise.c
> +++ b/kernel/trace/trace_osnoise.c
> @@ -2123,7 +2123,7 @@ static int start_per_cpu_kthreads(void)
>  #ifdef CONFIG_HOTPLUG_CPU
>  static void osnoise_hotplug_workfn(struct work_struct *dummy)
>  {
> -	unsigned int cpu = smp_processor_id();
> +	unsigned int cpu;
>  
>  	guard(mutex)(&trace_types_lock);
>  
> @@ -2133,13 +2133,13 @@ static void osnoise_hotplug_workfn(struct work_struct *dummy)
>  	guard(cpus_read_lock)();
>  	guard(mutex)(&interface_lock);
>  
> -	if (!cpu_online(cpu))
> +	if (!test_bit(OSN_WORKLOAD, &osnoise_options))
>  		return;
>  
> -	if (!cpumask_test_cpu(cpu, &osnoise_cpumask))
> -		return;
> -
> -	start_kthread(cpu);
> +	for_each_online_cpu(cpu) {
> +		if (cpumask_test_cpu(cpu, &osnoise_cpumask))
> +			start_kthread(cpu);
> +	}
>  }

[Severity: Medium]
Does returning early in osnoise_hotplug_workfn() whenever OSN_WORKLOAD is
clear leave per-CPU sampling disabled on newly onlined CPUs when running the
osnoise tracer with NO_OSNOISE_WORKLOAD?

When timerlat is disabled (i.e. the osnoise tracer is active) and
OSN_WORKLOAD is clear, start_kthread() is responsible for enabling sampling
on the CPU without spawning a kthread:

kernel/trace/trace_osnoise.c:start_kthread() {
	...
	if (timerlat_enabled()) {
		snprintf(comm, 24, "timerlat/%d", cpu);
		main = timerlat_main;
	} else {
		/* if no workload, just return */
		if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
			cpu_osn_var(cpu)->sampling = true;
			barrier();
			return 0;
		}
		snprintf(comm, 24, "osnoise/%d", cpu);
	}
	...
}

And when a CPU goes offline via osnoise_cpu_die() -> stop_kthread(),
sampling is cleared on that CPU:

kernel/trace/trace_osnoise.c:stop_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();
		}
	}
}

In start_per_cpu_kthreads(), the early return for !OSN_WORKLOAD is gated on
timerlat_enabled():

kernel/trace/trace_osnoise.c:start_per_cpu_kthreads() {
	...
	if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
		if (timerlat_enabled())
			return 0;
	}
	...
}

Should osnoise_hotplug_workfn() likewise check timerlat_enabled() before
returning early on !test_bit(OSN_WORKLOAD, &osnoise_options) (or let
start_kthread() handle the !OSN_WORKLOAD case) so that
cpu_osn_var(cpu)->sampling is set to true when a CPU in osnoise_cpumask comes
online?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024015614.3095251-2-crwood@redhat.com?part=2