Re: [PATCH 1/4] tracing/osnoise: Per-cpu mutex and fd detachment

On Mon, 2026-08-24 at 21:30 +0000, sashiko-bot@kernel.org wrote:

quoted

@@ -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.

quoted

@@ -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.

[ ... ]

quoted

@@ -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