On Mon, 2026-08-24 at 21:30 +0000, [email protected] wrote:
> > @@ -2086,15 +2100,9 @@ static int start_per_cpu_kthreads(void)
> > */
> > cpumask_and(current_mask, cpu_online_mask, &osnoise_cpumask);
> >
> > - for_each_possible_cpu(cpu) {
> > - if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask)) {
> > - struct task_struct *kthread;
> > -
> > - kthread = xchg_relaxed(&(per_cpu(per_cpu_osnoise_var,
> > cpu).kthread), NULL);
> > - if (!WARN_ON(!kthread))
> > - kthread_stop(kthread);
> > - }
> > - }
> > + for_each_possible_cpu(cpu)
> > + if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask))
> > + stop_kthread(cpu);
>
> [Severity: Medium]
> Will this always trigger the WARN_ON() inside stop_kthread()?
>
> Here in start_per_cpu_kthreads(), the kthread_cpumask bit is cleared prior
> to calling stop_kthread(cpu).
>
> But in stop_kthread(cpu) introduced above, it expects the bit to still be
> set and asserts:
> WARN_ON(!cpumask_test_and_clear_cpu(cpu, &kthread_cpumask));
>
> Since the caller already cleared the bit, won't this result in a guaranteed
> warning stack trace whenever this recovery path is executed?
This should just be replaced by a warning, as we shouldn't be able to get
here with an active kthread. All callers of kthread_start() and
start_per_cpu_kthreads() are under trace_types_lock. All callers of the
latter stop all threads under the same lock.before restarting them --
except osnoise_workload_start(), which still holds the lock from when
the isntance was first initialized.
> > @@ -2528,6 +2577,13 @@ timerlat_fd_read(struct file *file, char __user
> > *ubuf, size_t count,
> >
> > osn_var = this_cpu_osn_var();
> >
> > + /* In normal usage, this should always be uncontended. */
> > + mutex_lock(&osn_var->lock);
> > + if (fdd != osn_var->fdd || current != fdd->thread) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > /*
> > * The timerlat in user-space runs in a different order:
> > * the read() starts from the execution of the previous occurrence,
> > @@ -2574,6 +2630,11 @@ timerlat_fd_read(struct file *file, char __user
> > *ubuf, size_t count,
> > /* wait for the next period */
> > wait_next_period(tlat);
> >
> > + if (fdd != osn_var->fdd) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
>
> [Severity: High]
> Is it safe to hold osn_var->lock across a sleep in TASK_INTERRUPTIBLE?
>
> The function acquires osn_var->lock and then calls wait_next_period(),
> which sets the task state to TASK_INTERRUPTIBLE and calls schedule().
>
> If a user task calling read() is suspended (e.g., via SIGSTOP), it could
> remain in TASK_STOPPED and sleep indefinitely while holding the mutex.
> This would permanently block any administrator actions like CPU hotplug
> or stopping the tracer, since stop_kthread() attempts to acquire this same
> lock.
That's not how signals work.
> [ ... ]
>
> > @@ -3157,10 +3203,13 @@ __init static int init_timerlat_tracer(void)
> >
> > __init static int init_osnoise_tracer(void)
> > {
> > - int ret;
> > + int ret, cpu;
> >
> > mutex_init(&interface_lock);
> >
> > + for_each_online_cpu(cpu)
> > + mutex_init(&cpu_osn_var(cpu)->lock);
>
> [Severity: High]
> Does this skip possible CPUs that are offline during initialization?
>
> If a CPU is offline at boot time, its mutex won't be initialized here.
> Later, if an administrator hotplugs this CPU, osnoise_cpu_init() will
> schedule a workqueue to invoke start_kthread() on the newly online CPU.
> start_kthread() will immediately attempt to acquire the uninitialized lock,
> leading to a system crash or lockdep splat.
>
> Should this use for_each_possible_cpu(cpu) instead to ensure all CPUs
> have their mutex properly initialized for hotplug?
Yes, will fix. We should probably do the same in osn_var_reset() and
tlat_var_reset().
-Crystal
