[PATCH v7 00/10] tracing/osnoise: Synchronization fixes

[PATCH v7 00/10] tracing/osnoise: Synchronization fixes

[Crystal Wood]

Various improvements to synchronization in osnoise/timerlat.

v7:
 - Fix temporary build break in patch 2, with a change in patch 1
 - Other Sashiko-driven updates to patches 5 and 6

v6:
 - Revived the "Always check OSN_WORKLOAD" and "Split out per-cpu
   variable resets" patches, with fixes, to address minor issues raised
   by Sashiko
 - Other minor Sashiko-requseted fixes

https://lore.kernel.org/all/20261028220120.4005187-1-crwood@redhat.com/

v5:
 - Removed "Split out per-cpu variable resets" due to Sashiko feedback
   about races with NMIs and other tracing hooks; the main motivation
   for that patch was problems with osn->sampling and tlat->thread,
   both of which are now explicitly cleared.

 - Fixed some patch shuffling bugs in fd detachment that were revealed
   by dropping the reset patch

 - Added "Start all eligible cpus from hotplug workfn"

 - Removed "Always check OSN_WORKLOAD" which is no longer needed after the
   hotplug workfn patch

https://lore.kernel.org/all/20261024015614.3095251-1-crwood@redhat.com/

v4: fixed a problem with the v3 fix spotted by Sashiko

https://lore.kernel.org/all/20261014220759.908839-1-crwood@redhat.com/

v3: make sure osn->sampling gets cleared on stop, even for non-workload
osnoise (reported by Sashiko)

https://lore.kernel.org/all/20261014022640.695786-1-crwood@redhat.com/

v2: addresses issues raised by Sashiko, as well as a few more problems
discovered in additional testing.

https://lore.kernel.org/all/20261010022338.2507589-1-crwood@redhat.com/

v1: https://lore.kernel.org/all/20260824211544.3984835-1-crwood@redhat.com/

Crystal Wood (10):
  tracing/osnoise: Variable cleanup
  tracing/osnoise: start_kthread: Always check OSN_WORKLOAD
  tracing/osnoise: Start all eligible cpus from hotplug workfn
  tracing/osnoise: Hold a ref on osn->kthread
  tracing/osnoise: Per-cpu mutexes and fd detachment
  tracing/osnoise: Reset cleanup
  tracing/osnoise: kthreads: Verify CPU after migrate_disable()
  tracing/osnoise: Take trace_types_lock in timerlat_fd_open
  tracing/osnoise: Print taint message for timerlat
  tracing/osnoise: Refuse timerlat fd open if tracer inactive

 Documentation/trace/timerlat-tracer.rst |   5 +-
 kernel/trace/trace_osnoise.c            | 497 ++++++++++++++----------
 2 files changed, 290 insertions(+), 212 deletions(-)

-- 
2.56.0

[PATCH v7 01/10] tracing/osnoise: Variable cleanup

[Crystal Wood]

Add cpu_osn_var/cpu_tmr_var() accessors and use them.

Timerlat threads aren't always kthreads, so rename the variable to just
"thread".

rel_period isn't used at all, so remove it.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v7: Add osn variable to start_kthread(), replacing individual calls to
    cpu_osn_var(), which previously happened later in the patch set.
    This avoids a temporary build break in the next patch.
---
 kernel/trace/trace_osnoise.c | 54 +++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 995da9e0b548..2f3cbe71d313 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -251,6 +251,11 @@ static inline struct osnoise_variables *this_cpu_osn_var(void)
 	return this_cpu_ptr(&per_cpu_osnoise_var);
 }
 
+static inline struct osnoise_variables *cpu_osn_var(int cpu)
+{
+	return per_cpu_ptr(&per_cpu_osnoise_var, cpu);
+}
+
 /*
  * Protect the interface.
  */
@@ -261,9 +266,8 @@ static struct mutex interface_lock;
  * Runtime information for the timer mode.
  */
 struct timerlat_variables {
-	struct task_struct	*kthread;
+	struct task_struct	*thread;
 	struct hrtimer		timer;
-	u64			rel_period;
 	u64			abs_period;
 	bool			tracing_thread;
 	u64			count;
@@ -285,6 +289,11 @@ static inline struct timerlat_variables *this_cpu_tmr_var(void)
 	return this_cpu_ptr(&per_cpu_timerlat_var);
 }
 
+static inline struct timerlat_variables *cpu_tmr_var(int cpu)
+{
+	return per_cpu_ptr(&per_cpu_timerlat_var, cpu);
+}
+
 /*
  * tlat_var_reset - Reset the values of the given timerlat_variables
  */
@@ -301,8 +310,8 @@ static inline void tlat_var_reset(void)
 	 * zeroing the structure is perfect.
 	 */
 	for_each_online_cpu(cpu) {
-		tlat_var = per_cpu_ptr(&per_cpu_timerlat_var, cpu);
-		if (tlat_var->kthread)
+		tlat_var = cpu_tmr_var(cpu);
+		if (tlat_var->thread)
 			hrtimer_cancel(&tlat_var->timer);
 		memset(tlat_var, 0, sizeof(*tlat_var));
 	}
@@ -331,7 +340,7 @@ static inline void osn_var_reset(void)
 	 * zeroing the structure is perfect.
 	 */
 	for_each_online_cpu(cpu) {
-		osn_var = per_cpu_ptr(&per_cpu_osnoise_var, cpu);
+		osn_var = cpu_osn_var(cpu);
 		memset(osn_var, 0, sizeof(*osn_var));
 	}
 }
@@ -1227,9 +1236,9 @@ static void trace_sched_migrate_callback(void *data, struct task_struct *p, int
 	struct osnoise_variables *osn_var;
 	long cpu = task_cpu(p);
 
-	osn_var = per_cpu_ptr(&per_cpu_osnoise_var, cpu);
+	osn_var = cpu_osn_var(cpu);
 	if (osn_var->pid == p->pid && dest_cpu != cpu) {
-		per_cpu_ptr(&per_cpu_timerlat_var, cpu)->uthread_migrate = 1;
+		cpu_tmr_var(cpu)->uthread_migrate = 1;
 		osnoise_taint("timerlat user-thread migrated\n");
 		osnoise_stop_exception("timerlat user-thread migrated", cpu);
 	}
@@ -1827,13 +1836,13 @@ static enum hrtimer_restart timerlat_irq(struct hrtimer *timer)
 			osnoise_stop_tracing();
 			notify_new_max_latency(diff);
 
-			wake_up_process(tlat->kthread);
+			wake_up_process(tlat->thread);
 
 			return HRTIMER_NORESTART;
 		}
 	}
 
-	wake_up_process(tlat->kthread);
+	wake_up_process(tlat->thread);
 
 	if (osnoise_data.print_stack)
 		timerlat_save_stack(0);
@@ -1926,7 +1935,7 @@ static int timerlat_main(void *data)
 	tlat->tracing_thread = false;
 
 	hrtimer_setup(&tlat->timer, timerlat_irq, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
-	tlat->kthread = current;
+	tlat->thread = current;
 	osn_var->pid = current->pid;
 	/*
 	 * Annotate the arrival time.
@@ -1981,7 +1990,7 @@ static void stop_kthread(unsigned int cpu)
 {
 	struct task_struct *kthread;
 
-	kthread = xchg_relaxed(&(per_cpu(per_cpu_osnoise_var, cpu).kthread), NULL);
+	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))) {
@@ -2001,7 +2010,7 @@ static void stop_kthread(unsigned int cpu)
 			/*
 			 * This is set in the osnoise tracer case.
 			 */
-			per_cpu(per_cpu_osnoise_var, cpu).sampling = false;
+			cpu_osn_var(cpu)->sampling = false;
 			barrier();
 		}
 	}
@@ -2030,12 +2039,13 @@ static void stop_per_cpu_kthreads(void)
  */
 static int start_kthread(unsigned int cpu)
 {
+	struct osnoise_variables *osn = cpu_osn_var(cpu);
 	struct task_struct *kthread;
 	void *main = osnoise_main;
 	char comm[24];
 
 	/* Do not start a new thread if it is already running */
-	if (per_cpu(per_cpu_osnoise_var, cpu).kthread)
+	if (osn->kthread)
 		return 0;
 
 	if (timerlat_enabled()) {
@@ -2044,7 +2054,7 @@ static int start_kthread(unsigned int cpu)
 	} else {
 		/* if no workload, just return */
 		if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
-			per_cpu(per_cpu_osnoise_var, cpu).sampling = true;
+			osn->sampling = true;
 			barrier();
 			return 0;
 		}
@@ -2058,7 +2068,7 @@ static int start_kthread(unsigned int cpu)
 		return -ENOMEM;
 	}
 
-	per_cpu(per_cpu_osnoise_var, cpu).kthread = kthread;
+	osn->kthread = kthread;
 	cpumask_set_cpu(cpu, &kthread_cpumask);
 
 	return 0;
@@ -2091,7 +2101,7 @@ static int start_per_cpu_kthreads(void)
 		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);
+			kthread = xchg_relaxed(&cpu_osn_var(cpu)->kthread, NULL);
 			if (!WARN_ON(!kthread))
 				kthread_stop(kthread);
 		}
@@ -2520,7 +2530,7 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 			return -EINVAL;
 		}
 	} else {
-		per_cpu_ptr(&per_cpu_timerlat_var, cpu)->uthread_migrate = 1;
+		cpu_tmr_var(cpu)->uthread_migrate = 1;
 		osnoise_taint("timerlat user thread migrate\n");
 		osnoise_stop_tracing();
 		migrate_enable();
@@ -2564,7 +2574,7 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 		}
 	} else {
 		tlat->tracing_thread = false;
-		tlat->kthread = current;
+		tlat->thread = current;
 
 		/* Annotate now to drift new period */
 		tlat->abs_period = hrtimer_cb_get_time(&tlat->timer);
@@ -2613,10 +2623,10 @@ static int timerlat_fd_release(struct inode *inode, struct file *file)
 	migrate_disable();
 	mutex_lock(&interface_lock);
 
-	osn_var = per_cpu_ptr(&per_cpu_osnoise_var, cpu);
-	tlat_var = per_cpu_ptr(&per_cpu_timerlat_var, cpu);
+	osn_var = cpu_osn_var(cpu);
+	tlat_var = cpu_tmr_var(cpu);
 
-	if (tlat_var->kthread)
+	if (tlat_var->thread)
 		hrtimer_cancel(&tlat_var->timer);
 	memset(tlat_var, 0, sizeof(*tlat_var));
 
@@ -3096,7 +3106,7 @@ static void timerlat_tracer_stop(struct trace_array *tr)
 	 */
 	if (!osnoise_has_registered_instances()) {
 		for_each_online_cpu(cpu)
-			per_cpu(per_cpu_osnoise_var, cpu).sampling = 0;
+			cpu_osn_var(cpu)->sampling = 0;
 	}
 
 	osnoise_workload_stop();
-- 
2.56.0

[PATCH v7 02/10] tracing/osnoise: start_kthread: Always check OSN_WORKLOAD

[Crystal Wood]

We can get here on timerlat with OSN_WORKLOAD cleared via
osnoise_hotplug_workfn().

While we're moving this code, remove the pointless compiler barrier
immediately before returning from a function.  Even if it's inlined,
it's not doing anything useful.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
 kernel/trace/trace_osnoise.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 2f3cbe71d313..b812a73fa5da 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -2048,16 +2048,17 @@ static int start_kthread(unsigned int cpu)
 	if (osn->kthread)
 		return 0;
 
+	/* Can still happen with timerlat via cpu hotplug */
+	if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
+		if (!timerlat_enabled())
+			osn->sampling = true;
+		return 0;
+	}
+
 	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)) {
-			osn->sampling = true;
-			barrier();
-			return 0;
-		}
 		snprintf(comm, 24, "osnoise/%d", cpu);
 	}
 
-- 
2.56.0

[PATCH v7 03/10] tracing/osnoise: Start all eligible cpus from hotplug workfn

[Crystal Wood]

Previously, a start could be lost if multiple cpus get onlined
before the workqueue has a chance to run.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v5: new patch

I have this in my notes as being a Sashiko comment, though I can't
find the source now.  I wasn't able to reproduce the race given how
slow and serialized cpu hotplug operations are, but it's possible.

On the other hand, user threads detach on cpu offline with no mechanism
for reattaching (other than userspace figuring it out and reopening the
fd), and you really probably shouldn't be offlining cpus that you're
measuring with tools like this, so you could make an argument for removing
the restart mechanism entirely... it'd certainly shrink the potential bug
surface.

This also eliminates the need for the "Always check OSN_WORKLOAD" patch.

v6:

Removed the OSN_WORKLOAD check from this function, and revived the
"Always check OSN_WORKLOAD", so that start_kthread() can set osn->sampling
in the NO_OSN_WORKLOAD osnoise case
---
 kernel/trace/trace_osnoise.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index b812a73fa5da..1ddeffbf437d 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -2125,7 +2125,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);
 
@@ -2135,13 +2135,10 @@ static void osnoise_hotplug_workfn(struct work_struct *dummy)
 	guard(cpus_read_lock)();
 	guard(mutex)(&interface_lock);
 
-	if (!cpu_online(cpu))
-		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);
+	}
 }
 
 static DECLARE_WORK(osnoise_hotplug_work, osnoise_hotplug_workfn);
@@ -2151,7 +2148,7 @@ static DECLARE_WORK(osnoise_hotplug_work, osnoise_hotplug_workfn);
  */
 static int osnoise_cpu_init(unsigned int cpu)
 {
-	schedule_work_on(cpu, &osnoise_hotplug_work);
+	schedule_work(&osnoise_hotplug_work);
 	return 0;
 }
 
-- 
2.56.0

[PATCH v7 04/10] tracing/osnoise: Hold a ref on osn->kthread

[Crystal Wood]

Deal with early kthread termination by holding a reference to the
task_struct and always running the cleanup code in stop_kthread(), rather
than with hacks like osnoise_migration_pending().

This avoids a potential deadlock when a later patch switches from xchg
to using a lock to protect osn->kthread updates, and also deals with
any other situations where a kthread might exit early.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v2: New patch.  I did this in response to Sashiko pointing out a deadlock
in v1 with the use of osn->lock in osnoise_migration_pending.  That might
no longer be an issue now that detach_lock is split out, but this is still
the cleaner and more robust approach.
---
 kernel/trace/trace_osnoise.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 1ddeffbf437d..7b343f47cf2f 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -1692,17 +1692,6 @@ static inline int osnoise_migration_pending(void)
 	 * thus: taint and leave to unblock the blocked thread.
 	 */
 	osnoise_taint("migration requested to osnoise threads, leaving.");
-
-	/*
-	 * Unset this thread from the threads managed by the interface.
-	 * The tracers are responsible for cleaning their env before
-	 * exiting.
-	 */
-	mutex_lock(&interface_lock);
-	this_cpu_osn_var()->kthread = NULL;
-	cpumask_clear_cpu(smp_processor_id(), &kthread_cpumask);
-	mutex_unlock(&interface_lock);
-
 	return 1;
 }
 
@@ -2002,8 +1991,8 @@ static void stop_kthread(unsigned int cpu)
 			 * by killing the thread. NOTE: this is a purpose specific file.
 			 */
 			kill_pid(kthread->thread_pid, SIGKILL, 1);
-			put_task_struct(kthread);
 		}
+		put_task_struct(kthread);
 	} else {
 		/* if no workload, just return */
 		if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
@@ -2062,15 +2051,17 @@ static int start_kthread(unsigned int cpu)
 		snprintf(comm, 24, "osnoise/%d", cpu);
 	}
 
-	kthread = kthread_run_on_cpu(main, NULL, cpu, comm);
+	kthread = kthread_create_on_cpu(main, NULL, cpu, comm);
 
 	if (IS_ERR(kthread)) {
 		pr_err(BANNER "could not start sampling thread\n");
 		return -ENOMEM;
 	}
 
+	get_task_struct(kthread);
 	osn->kthread = kthread;
 	cpumask_set_cpu(cpu, &kthread_cpumask);
+	wake_up_process(kthread);
 
 	return 0;
 }
-- 
2.56.0

[PATCH v7 05/10] tracing/osnoise: Per-cpu mutexes and fd detachment

[Crystal Wood]

Clean up a variety of synchronization issues and related bandaids
by having a per-cpu mutex that guards changes to kthread, and
another than guards fd open/close/revoke.

Replace the SIGKILL hack for userspace timerlat threads (that doesn't
even work, because we don't wait for the process to actually die) with
a mutex-protected detachment mechanism.  The mutex should be uncontended
during normal timerlat_fd_read() usage.

This also eliminates the need for the "quick fix" of e6a53481da292
("tracing/timerlat: Only clear timer if a kthread exists").

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v2:
 - Use a separate lock for detachment, so that we can wake the thread that
   holds the detach lock, while holding osn->lock to make sure the thread
   is valid.
 - Return -EINVAL if detached while waiting, and -ERESTARTSYS on a signal
 - Init mutex on every possible cpu (as pointed out by Sashiko)
 - Update documentation for SIGKILL removal

v3:
 - Clear osn_var->sampling in stop_kthread(), as reporteed by Sashiko
   for the osnoise non-workload case

v5:
 - Fix some patch-shuffling bugs in timerlat_fd_detach() that were previously
   undone by the reset patch.

v6:
 - Move migration checks after the detach lock
 - Clear osn->sampling on signal exit.  It's not expected that the signal will
   be non-fatal given the limited use case, but this gives somewhat more reasonable
   behavior if it does get restarted.  Or do we want to fail more noisily
   in that case?
 - Remove detachment language from user doc that doesn't apply in all scenarios

v7:
 - Remove the uthread_migrate setting from timer_fd_read() which seems
   redundant with trace_sched_migrate_callback(), and complicates the
   locking.
 - Clear tlat->tracing_thread on abnormal timerlat_fd_read() exit
 - Use WARN_ON_ONCE consistently
---
 Documentation/trace/timerlat-tracer.rst |   5 +-
 kernel/trace/trace_osnoise.c            | 326 +++++++++++++++---------
 2 files changed, 203 insertions(+), 128 deletions(-)

diff --git a/Documentation/trace/timerlat-tracer.rst b/Documentation/trace/timerlat-tracer.rst
index 68d429d454a5..e23467376ff5 100644
--- a/Documentation/trace/timerlat-tracer.rst
+++ b/Documentation/trace/timerlat-tracer.rst
@@ -220,8 +220,9 @@ After reporting the total latency, timerlat will restart the cycle, arm
 a timer, and go to sleep for the following activation.
 
 If at any time one of the conditions is broken, e.g., the thread migrates
-while in user space, or the timerlat tracer is disabled, the SIG_KILL
-signal will be sent to the user-space thread.
+while in user space, or the timerlat tracer is disabled, subsequent usage
+will fail with EINVAL.  Note that older kernels may deliver a SIGKILL
+instead.
 
 Here is an basic example of user-space code for timerlat::
 
diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 7b343f47cf2f..e61f57230eef 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -223,19 +223,30 @@ struct osn_thread {
 	u64	delta_start;
 };
 
+struct fd_data {
+	struct task_struct *thread;
+	void (*detach)(struct fd_data *fdd);
+	int cpu;
+};
+
 /*
  * Runtime information: this structure saves the runtime information used by
  * one sampling thread.
  */
 struct osnoise_variables {
+	struct mutex		lock; /* covers kthread and fdd changes */
 	struct task_struct	*kthread;
-	bool			sampling;
-	pid_t			pid;
-	struct osn_nmi		nmi;
-	struct osn_irq		irq;
-	struct osn_softirq	softirq;
-	struct osn_thread	thread;
-	local_t			int_counter;
+	struct fd_data		*fdd;
+
+	struct_group(zero,
+		bool			sampling;
+		pid_t			pid;
+		struct osn_nmi		nmi;
+		struct osn_irq		irq;
+		struct osn_softirq	softirq;
+		struct osn_thread	thread;
+		local_t			int_counter;
+	);
 };
 
 /*
@@ -266,12 +277,25 @@ static struct mutex interface_lock;
  * Runtime information for the timer mode.
  */
 struct timerlat_variables {
-	struct task_struct	*thread;
-	struct hrtimer		timer;
-	u64			abs_period;
-	bool			tracing_thread;
-	u64			count;
-	bool			uthread_migrate;
+	/*
+	 * This is separate from osn->lock in order to allow waking
+	 * the waiting thread during forced detach, without having to
+	 * wait for the next period (and possibly longer, due to
+	 * priority-based lock stealing).
+	 *
+	 * Both locks must be held during the actual detachment.
+	 */
+	struct mutex		detach_lock;
+
+	struct_group(zero,
+		struct task_struct	*thread;
+		struct hrtimer		timer;
+		u64			abs_period;
+		bool			tracing_thread;
+		u64			count;
+		bool			uthread_migrate;
+		bool			detach;
+	);
 };
 
 static DEFINE_PER_CPU(struct timerlat_variables, per_cpu_timerlat_var);
@@ -302,26 +326,20 @@ static inline void tlat_var_reset(void)
 	struct timerlat_variables *tlat_var;
 	int cpu;
 
-	/* Synchronize with the timerlat interfaces */
-	mutex_lock(&interface_lock);
-
 	/*
 	 * So far, all the values are initialized as 0, so
 	 * zeroing the structure is perfect.
 	 */
 	for_each_online_cpu(cpu) {
 		tlat_var = cpu_tmr_var(cpu);
-		if (tlat_var->thread)
-			hrtimer_cancel(&tlat_var->timer);
-		memset(tlat_var, 0, sizeof(*tlat_var));
+		WARN_ON_ONCE(tlat_var->thread);
+		memset(&tlat_var->zero, 0, sizeof(tlat_var->zero));
 	}
 	/*
 	 * Reset also align_next, to be filled by a new offset by the first timerlat
 	 * thread that wakes up, if TIMERLAT_ALIGN is set.
 	 */
 	atomic64_set(&align_next, 0);
-
-	mutex_unlock(&interface_lock);
 }
 #else /* CONFIG_TIMERLAT_TRACER */
 #define tlat_var_reset()	do {} while (0)
@@ -341,7 +359,10 @@ static inline void osn_var_reset(void)
 	 */
 	for_each_online_cpu(cpu) {
 		osn_var = cpu_osn_var(cpu);
-		memset(osn_var, 0, sizeof(*osn_var));
+
+		WARN_ON_ONCE(osn_var->kthread);
+		WARN_ON_ONCE(osn_var->fdd);
+		memset(&osn_var->zero, 0, sizeof(osn_var->zero));
 	}
 }
 
@@ -1884,10 +1905,18 @@ static int wait_next_period(struct timerlat_variables *tlat)
 	}
 
 	set_current_state(TASK_INTERRUPTIBLE);
+	if (!tlat->detach && !signal_pending(current)) {
+		hrtimer_start(&tlat->timer, next_abs_period, HRTIMER_MODE_ABS_PINNED_HARD);
+		schedule();
+	}
+	__set_current_state(TASK_RUNNING);
 
-	hrtimer_start(&tlat->timer, next_abs_period, HRTIMER_MODE_ABS_PINNED_HARD);
-	schedule();
-	return 1;
+	if (tlat->detach || signal_pending(current)) {
+		hrtimer_cancel(&tlat->timer);
+		return -1;
+	}
+
+	return 0;
 }
 
 /*
@@ -1961,7 +1990,9 @@ static int timerlat_main(void *data)
 		wait_next_period(tlat);
 	}
 
+	WARN_ON_ONCE(hrtimer_active(&tlat->timer));
 	hrtimer_cancel(&tlat->timer);
+	tlat->thread = NULL;
 	migrate_enable();
 	return 0;
 }
@@ -1977,32 +2008,24 @@ 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_ONCE(test_bit(OSN_WORKLOAD, &osnoise_options));
+		WARN_ON_ONCE(osn_var->kthread);
+		osn_var->fdd->detach(osn_var->fdd);
+	} else if (osn_var->kthread) {
+		WARN_ON_ONCE(!cpumask_test_and_clear_cpu(cpu, &kthread_cpumask));
+		WARN_ON_ONCE(!test_bit(OSN_WORKLOAD, &osnoise_options));
+		kthread_stop(osn_var->kthread);
+		put_task_struct(osn_var->kthread);
+		osn_var->kthread = NULL;
 	}
+
+	osn_var->sampling = false;
+	mutex_unlock(&osn_var->lock);
 }
 
 /*
@@ -2032,16 +2055,20 @@ static int start_kthread(unsigned int cpu)
 	struct task_struct *kthread;
 	void *main = osnoise_main;
 	char comm[24];
+	int ret = 0;
+
+	lockdep_assert_held(&trace_types_lock);
+	mutex_lock(&osn->lock);
 
 	/* Do not start a new thread if it is already running */
 	if (osn->kthread)
-		return 0;
+		goto out;
 
 	/* Can still happen with timerlat via cpu hotplug */
 	if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
 		if (!timerlat_enabled())
 			osn->sampling = true;
-		return 0;
+		goto out;
 	}
 
 	if (timerlat_enabled()) {
@@ -2055,7 +2082,8 @@ static int start_kthread(unsigned int cpu)
 
 	if (IS_ERR(kthread)) {
 		pr_err(BANNER "could not start sampling thread\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	get_task_struct(kthread);
@@ -2063,7 +2091,9 @@ static int start_kthread(unsigned int cpu)
 	cpumask_set_cpu(cpu, &kthread_cpumask);
 	wake_up_process(kthread);
 
-	return 0;
+out:
+	mutex_unlock(&osn->lock);
+	return ret;
 }
 
 /*
@@ -2089,15 +2119,8 @@ 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(&cpu_osn_var(cpu)->kthread, NULL);
-			if (!WARN_ON(!kthread))
-				kthread_stop(kthread);
-		}
-	}
+	for_each_possible_cpu(cpu)
+		WARN_ON_ONCE(cpumask_test_cpu(cpu, &kthread_cpumask));
 
 	for_each_cpu(cpu, current_mask) {
 		retval = start_kthread(cpu);
@@ -2417,11 +2440,46 @@ osnoise_cpus_write(struct file *filp, const char __user *ubuf, size_t count,
 }
 
 #ifdef CONFIG_TIMERLAT_TRACER
+static void timerlat_fd_detach(struct fd_data *fdd)
+{
+	struct osnoise_variables *osn;
+	struct timerlat_variables *tlat;
+
+	osn = cpu_osn_var(fdd->cpu);
+	tlat = cpu_tmr_var(fdd->cpu);
+
+	lockdep_assert_held(&osn->lock);
+	if (WARN_ON_ONCE(fdd != osn->fdd))
+		return;
+
+	WARN_ON_ONCE(osn->kthread);
+	WARN_ON_ONCE(fdd->thread != tlat->thread);
+	tlat->detach = true;
+	wake_up_process(tlat->thread);
+
+	mutex_lock(&tlat->detach_lock);
+
+	osn->sampling = 0;
+	osn->pid = 0;
+
+	WARN_ON_ONCE(hrtimer_active(&tlat->timer));
+	hrtimer_cancel(&tlat->timer);
+	put_task_struct(fdd->thread);
+
+	fdd->thread = NULL;
+	osn->fdd = NULL;
+	memset(&tlat->zero, 0, sizeof(tlat->zero));
+
+	mutex_unlock(&tlat->detach_lock);
+}
+
 static int timerlat_fd_open(struct inode *inode, struct file *file)
 {
+	struct fd_data *fdd;
 	struct osnoise_variables *osn_var;
 	struct timerlat_variables *tlat;
 	long cpu = (long) inode->i_cdev;
+	int ret = 0;
 
 	mutex_lock(&interface_lock);
 
@@ -2437,16 +2495,16 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	migrate_disable();
 
 	osn_var = this_cpu_osn_var();
+	tlat = this_cpu_tmr_var();
+	mutex_lock(&osn_var->lock);
 
-	/*
-	 * The osn_var->pid holds the single access to this file.
-	 */
-	if (osn_var->pid) {
-		mutex_unlock(&interface_lock);
-		migrate_enable();
-		return -EBUSY;
+	if (osn_var->fdd) {
+		ret = -EBUSY;
+		goto err;
 	}
 
+	WARN_ON_ONCE(osn_var->kthread);
+
 	/*
 	 * timerlat tracer is a per-cpu tracer. Check if the user-space too
 	 * is pinned to a single CPU. The tracer laters monitor if the task
@@ -2455,33 +2513,47 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	 * setup.
 	 */
 	if (current->nr_cpus_allowed > 1 ||  cpu != smp_processor_id()) {
-		mutex_unlock(&interface_lock);
-		migrate_enable();
-		return -EPERM;
+		ret = -EPERM;
+		goto err;
+	}
+
+	fdd = kmalloc(sizeof(*fdd), GFP_KERNEL);
+	if (!fdd) {
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	/*
 	 * From now on, it is good to go.
 	 */
-	file->private_data = inode->i_cdev;
+	fdd->thread = current;
+	fdd->detach = timerlat_fd_detach;
+	fdd->cpu = cpu;
+	file->private_data = fdd;
 
+	tlat->tracing_thread = false;
+	tlat->thread = current;
 	get_task_struct(current);
 
-	osn_var->kthread = current;
 	osn_var->pid = current->pid;
+	osn_var->fdd = fdd;
+
+	hrtimer_setup(&tlat->timer, timerlat_irq, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
 
 	/*
 	 * Setup is done.
 	 */
+	mutex_unlock(&osn_var->lock);
 	mutex_unlock(&interface_lock);
 
-	tlat = this_cpu_tmr_var();
-	tlat->count = 0;
-
-	hrtimer_setup(&tlat->timer, timerlat_irq, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
-
 	migrate_enable();
 	return 0;
+
+err:
+	mutex_unlock(&osn_var->lock);
+	mutex_unlock(&interface_lock);
+	migrate_enable();
+	return ret;
 };
 
 /*
@@ -2497,36 +2569,28 @@ static ssize_t
 timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 		  loff_t *ppos)
 {
-	long cpu = (long) file->private_data;
+	struct fd_data *fdd = file->private_data;
 	struct osnoise_variables *osn_var;
 	struct timerlat_variables *tlat;
 	struct timerlat_sample s;
 	s64 diff;
 	u64 now;
+	int ret = 0;
 
 	migrate_disable();
 
 	tlat = this_cpu_tmr_var();
+	osn_var = this_cpu_osn_var();
 
-	/*
-	 * While in user-space, the thread is migratable. There is nothing
-	 * we can do about it.
-	 * So, if the thread is running on another CPU, stop the machinery.
-	 */
-	if (cpu == smp_processor_id()) {
-		if (tlat->uthread_migrate) {
-			migrate_enable();
-			return -EINVAL;
-		}
-	} else {
-		cpu_tmr_var(cpu)->uthread_migrate = 1;
-		osnoise_taint("timerlat user thread migrate\n");
-		osnoise_stop_tracing();
-		migrate_enable();
-		return -EINVAL;
+	/* In normal usage, this should always be uncontended. */
+	mutex_lock(&tlat->detach_lock);
+	if (fdd != osn_var->fdd || current != fdd->thread ||
+	    tlat->uthread_migrate) {
+		ret = -EINVAL;
+		goto out;
 	}
 
-	osn_var = this_cpu_osn_var();
+	WARN_ON_ONCE(fdd->cpu != smp_processor_id());
 
 	/*
 	 * The timerlat in user-space runs in a different order:
@@ -2562,9 +2626,6 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 			}
 		}
 	} else {
-		tlat->tracing_thread = false;
-		tlat->thread = current;
-
 		/* Annotate now to drift new period */
 		tlat->abs_period = hrtimer_cb_get_time(&tlat->timer);
 
@@ -2572,7 +2633,21 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 	}
 
 	/* wait for the next period */
-	wait_next_period(tlat);
+	if (wait_next_period(tlat) < 0) {
+		if (signal_pending(current))
+			ret = -ERESTARTSYS;
+		else
+			ret = -EINVAL;
+
+		tlat->tracing_thread = false;
+		osn_var->sampling = false;
+		goto out;
+	}
+
+	if (WARN_ON_ONCE(fdd != osn_var->fdd)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/* This is the wakeup from this cycle */
 	now = ktime_to_ns(hrtimer_cb_get_time(&tlat->timer));
@@ -2599,39 +2674,24 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 	}
 
 out:
+	mutex_unlock(&tlat->detach_lock);
 	migrate_enable();
-	return 0;
+	return ret;
 }
 
 static int timerlat_fd_release(struct inode *inode, struct file *file)
 {
+	struct fd_data *fdd = file->private_data;
 	struct osnoise_variables *osn_var;
-	struct timerlat_variables *tlat_var;
-	long cpu = (long) file->private_data;
 
-	migrate_disable();
-	mutex_lock(&interface_lock);
-
-	osn_var = cpu_osn_var(cpu);
-	tlat_var = cpu_tmr_var(cpu);
+	osn_var = cpu_osn_var(fdd->cpu);
 
-	if (tlat_var->thread)
-		hrtimer_cancel(&tlat_var->timer);
-	memset(tlat_var, 0, sizeof(*tlat_var));
+	mutex_lock(&osn_var->lock);
+	if (fdd == osn_var->fdd)
+		timerlat_fd_detach(fdd);
+	mutex_unlock(&osn_var->lock);
 
-	osn_var->sampling = 0;
-	osn_var->pid = 0;
-
-	/*
-	 * We are leaving, not being stopped... see stop_kthread();
-	 */
-	if (osn_var->kthread) {
-		put_task_struct(osn_var->kthread);
-		osn_var->kthread = NULL;
-	}
-
-	mutex_unlock(&interface_lock);
-	migrate_enable();
+	kfree(fdd);
 	return 0;
 }
 #endif
@@ -3146,6 +3206,14 @@ static struct tracer timerlat_tracer __read_mostly = {
 
 __init static int init_timerlat_tracer(void)
 {
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		struct timerlat_variables *tlat = cpu_tmr_var(cpu);
+
+		mutex_init(&tlat->detach_lock);
+	}
+
 	return register_tracer(&timerlat_tracer);
 }
 #else /* CONFIG_TIMERLAT_TRACER */
@@ -3157,10 +3225,16 @@ __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_possible_cpu(cpu) {
+		struct osnoise_variables *osn = cpu_osn_var(cpu);
+
+		mutex_init(&osn->lock);
+	}
+
 	cpumask_copy(&osnoise_cpumask, cpu_all_mask);
 
 	ret = register_tracer(&osnoise_tracer);
-- 
2.56.0

[PATCH v7 06/10] tracing/osnoise: Reset cleanup

[Crystal Wood]

Reset per-cpu timerlat state every time we stop a kthread or detach an
fd, rather than assume everything gets cleaned up properly, and avoiding
the need to explicitly zero individual fields.

Also, stop initializing tlat->thread lazily, add some debug checks to
detect an unclean state, and clear osnoise_data.tainted in
osn_var_reset().

Combine the per-cpu tlat cleanup and clearing osn->sampling and osn->pid into
osn_reset_cpu(), which is called consistently regardless of the tracer mode.

Don't clear the rest of the osnoise per-cpu state except when the full
tracer has been reset, to avoid racing with interrupt trace hooks.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v2: New patch.  Without this, I was seeing sampling = 1 leaking from a
kthread context to an fd context, which confused the latter into
operating with tlat->thread set to the old kernel thread, causing the fd
thread to hang.

v3: Reset the per-cpu context unconditionally in stop_kthread(), so sampling
gets cleared as noted in patch 3/9

v4: Only reset the cpu data if the fdd is a match

v6: Revive patch, but remove the interrupt counters from the zeroed area

v7:
 - Go back to clearing interrupt counters on full tracer reset, explicily clearing
   osn->sampling and osn->pid only during cpu detachment.  The issue
   pointed out by Sashiko in v6, where we could have stale data after a
   detach, is still there but it is not a new issue and is out of scope
   for this patchset, and unlike v6 it doesn't persist across full tracer
   resets.
 - Use WARN_ON_ONCE consistently
---
 kernel/trace/trace_osnoise.c | 117 ++++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 56 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index e61f57230eef..8db0697b69e4 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -237,9 +237,10 @@ struct osnoise_variables {
 	struct mutex		lock; /* covers kthread and fdd changes */
 	struct task_struct	*kthread;
 	struct fd_data		*fdd;
+	bool			sampling;
 
+	/* reset only when the entire tracer is restarted */
 	struct_group(zero,
-		bool			sampling;
 		pid_t			pid;
 		struct osn_nmi		nmi;
 		struct osn_irq		irq;
@@ -287,6 +288,7 @@ struct timerlat_variables {
 	 */
 	struct mutex		detach_lock;
 
+	/* reset on per-cpu detachment */
 	struct_group(zero,
 		struct task_struct	*thread;
 		struct hrtimer		timer;
@@ -318,23 +320,19 @@ static inline struct timerlat_variables *cpu_tmr_var(int cpu)
 	return per_cpu_ptr(&per_cpu_timerlat_var, cpu);
 }
 
+static inline void tlat_reset_cpu(int cpu)
+{
+	struct timerlat_variables *tlat = cpu_tmr_var(cpu);
+
+	WARN_ON_ONCE(tlat->thread);
+	memset(&tlat->zero, 0, sizeof(tlat->zero));
+}
+
 /*
- * tlat_var_reset - Reset the values of the given timerlat_variables
+ * tlat_var_reset - Reset non-per-cpu state
  */
 static inline void tlat_var_reset(void)
 {
-	struct timerlat_variables *tlat_var;
-	int cpu;
-
-	/*
-	 * So far, all the values are initialized as 0, so
-	 * zeroing the structure is perfect.
-	 */
-	for_each_online_cpu(cpu) {
-		tlat_var = cpu_tmr_var(cpu);
-		WARN_ON_ONCE(tlat_var->thread);
-		memset(&tlat_var->zero, 0, sizeof(tlat_var->zero));
-	}
 	/*
 	 * Reset also align_next, to be filled by a new offset by the first timerlat
 	 * thread that wakes up, if TIMERLAT_ALIGN is set.
@@ -342,38 +340,12 @@ static inline void tlat_var_reset(void)
 	atomic64_set(&align_next, 0);
 }
 #else /* CONFIG_TIMERLAT_TRACER */
-#define tlat_var_reset()	do {} while (0)
-#endif /* CONFIG_TIMERLAT_TRACER */
-
-/*
- * osn_var_reset - Reset the values of the given osnoise_variables
- */
-static inline void osn_var_reset(void)
+static inline void tlat_reset_cpu(int cpu)
 {
-	struct osnoise_variables *osn_var;
-	int cpu;
-
-	/*
-	 * So far, all the values are initialized as 0, so
-	 * zeroing the structure is perfect.
-	 */
-	for_each_online_cpu(cpu) {
-		osn_var = cpu_osn_var(cpu);
-
-		WARN_ON_ONCE(osn_var->kthread);
-		WARN_ON_ONCE(osn_var->fdd);
-		memset(&osn_var->zero, 0, sizeof(osn_var->zero));
-	}
 }
 
-/*
- * osn_var_reset_all - Reset the value of all per-cpu osnoise_variables
- */
-static inline void osn_var_reset_all(void)
-{
-	osn_var_reset();
-	tlat_var_reset();
-}
+#define tlat_var_reset()	do {} while (0)
+#endif /* CONFIG_TIMERLAT_TRACER */
 
 /*
  * Tells NMIs to call back to the osnoise tracer to record timestamps.
@@ -408,6 +380,39 @@ static struct osnoise_data {
 #endif
 };
 
+static inline void osn_reset_cpu(int cpu)
+{
+	struct osnoise_variables *osn = cpu_osn_var(cpu);
+
+	WARN_ON_ONCE(osn->kthread);
+	WARN_ON_ONCE(osn->fdd);
+
+	osn->sampling = false;
+	osn->pid = 0;
+	tlat_reset_cpu(cpu);
+}
+
+/*
+ * osn_var_reset - Reset non-per-cpu state, and check for stale data
+ */
+static inline void osn_var_reset(void)
+{
+	struct osnoise_variables *osn;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		osn = cpu_osn_var(cpu);
+
+		WARN_ON_ONCE(osn->kthread);
+		WARN_ON_ONCE(osn->fdd);
+		WARN_ON_ONCE(osn->sampling);
+		memset(&osn->zero, 0, sizeof(osn->zero));
+	}
+
+	tlat_var_reset();
+	osnoise_data.tainted = false;
+}
+
 #ifdef CONFIG_TIMERLAT_TRACER
 static inline bool timerlat_enabled(void)
 {
@@ -2024,7 +2029,7 @@ static void stop_kthread(unsigned int cpu)
 		osn_var->kthread = NULL;
 	}
 
-	osn_var->sampling = false;
+	osn_reset_cpu(cpu);
 	mutex_unlock(&osn_var->lock);
 }
 
@@ -2452,23 +2457,19 @@ static void timerlat_fd_detach(struct fd_data *fdd)
 	if (WARN_ON_ONCE(fdd != osn->fdd))
 		return;
 
-	WARN_ON_ONCE(osn->kthread);
 	WARN_ON_ONCE(fdd->thread != tlat->thread);
 	tlat->detach = true;
 	wake_up_process(tlat->thread);
 
 	mutex_lock(&tlat->detach_lock);
 
-	osn->sampling = 0;
-	osn->pid = 0;
-
 	WARN_ON_ONCE(hrtimer_active(&tlat->timer));
 	hrtimer_cancel(&tlat->timer);
 	put_task_struct(fdd->thread);
 
 	fdd->thread = NULL;
+	tlat->thread = NULL;
 	osn->fdd = NULL;
-	memset(&tlat->zero, 0, sizeof(tlat->zero));
 
 	mutex_unlock(&tlat->detach_lock);
 }
@@ -2531,13 +2532,17 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	fdd->cpu = cpu;
 	file->private_data = fdd;
 
-	tlat->tracing_thread = false;
-	tlat->thread = current;
 	get_task_struct(current);
 
 	osn_var->pid = current->pid;
 	osn_var->fdd = fdd;
 
+	WARN_ON_ONCE(tlat->thread);
+	WARN_ON_ONCE(tlat->count != 0);
+	WARN_ON_ONCE(tlat->tracing_thread);
+	WARN_ON_ONCE(osn_var->sampling);
+	tlat->thread = current;
+
 	hrtimer_setup(&tlat->timer, timerlat_irq, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
 
 	/*
@@ -2682,13 +2687,13 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 static int timerlat_fd_release(struct inode *inode, struct file *file)
 {
 	struct fd_data *fdd = file->private_data;
-	struct osnoise_variables *osn_var;
-
-	osn_var = cpu_osn_var(fdd->cpu);
+	struct osnoise_variables *osn_var = cpu_osn_var(fdd->cpu);
 
 	mutex_lock(&osn_var->lock);
-	if (fdd == osn_var->fdd)
+	if (fdd == osn_var->fdd) {
 		timerlat_fd_detach(fdd);
+		osn_reset_cpu(fdd->cpu);
+	}
 	mutex_unlock(&osn_var->lock);
 
 	kfree(fdd);
@@ -3002,7 +3007,7 @@ static int osnoise_workload_start(void)
 	if (osnoise_has_registered_instances())
 		return 0;
 
-	osn_var_reset_all();
+	osn_var_reset();
 
 	retval = osnoise_hook_events();
 	if (retval)
-- 
2.56.0

[PATCH v7 07/10] tracing/osnoise: kthreads: Verify CPU after migrate_disable()

[Crystal Wood]

Pass osn vars as the kthread arg, and make sure it matches where ve're
running in case of (e.g.) hotplug issues.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v2:
 - Do it for osnoise too, not just timerlat
 - I haven't been able to reproduce this in a while, and I'm not seeing the
   hole in the hotplug sync that would allow it, but I also don't see what
   would have allowed it in v1 (maybe it was in some pre-v1 iteration? or
   maybe just a tiny race window).

   So we can maybe drop this patch, though it still feels better to explicitly
   splat if problems happen rather than rely on this_cpu_osn_var() blowing up
   due to the affinity being set to multiple cpus.
---
 kernel/trace/trace_osnoise.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 8db0697b69e4..a81992c937fb 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -238,6 +238,7 @@ struct osnoise_variables {
 	struct task_struct	*kthread;
 	struct fd_data		*fdd;
 	bool			sampling;
+	int			cpu;
 
 	/* reset only when the entire tracer is restarted */
 	struct_group(zero,
@@ -1729,6 +1730,7 @@ static inline int osnoise_migration_pending(void)
  */
 static int osnoise_main(void *data)
 {
+	struct osnoise_variables *osn = data;
 	unsigned long flags;
 
 	/*
@@ -1739,6 +1741,11 @@ static int osnoise_main(void *data)
 	 * flag.
 	 */
 	migrate_disable();
+	if (WARN_ON_ONCE(osn->cpu != smp_processor_id())) {
+		migrate_enable();
+		return 1;
+	}
+
 	raw_spin_lock_irqsave(&current->pi_lock, flags);
 	current->flags &= ~(PF_NO_SETAFFINITY);
 	raw_spin_unlock_irqrestore(&current->pi_lock, flags);
@@ -1929,8 +1936,8 @@ static int wait_next_period(struct timerlat_variables *tlat)
  */
 static int timerlat_main(void *data)
 {
-	struct osnoise_variables *osn_var = this_cpu_osn_var();
-	struct timerlat_variables *tlat = this_cpu_tmr_var();
+	struct osnoise_variables *osn_var = data;
+	struct timerlat_variables *tlat;
 	struct timerlat_sample s;
 	struct sched_param sp;
 	unsigned long flags;
@@ -1950,6 +1957,13 @@ static int timerlat_main(void *data)
 	 * flag.
 	 */
 	migrate_disable();
+	if (WARN_ON_ONCE(osn_var->cpu != smp_processor_id())) {
+		migrate_enable();
+		return 1;
+	}
+
+	tlat = this_cpu_tmr_var();
+
 	raw_spin_lock_irqsave(&current->pi_lock, flags);
 	current->flags &= ~(PF_NO_SETAFFINITY);
 	raw_spin_unlock_irqrestore(&current->pi_lock, flags);
@@ -2083,7 +2097,7 @@ static int start_kthread(unsigned int cpu)
 		snprintf(comm, 24, "osnoise/%d", cpu);
 	}
 
-	kthread = kthread_create_on_cpu(main, NULL, cpu, comm);
+	kthread = kthread_create_on_cpu(main, cpu_osn_var(cpu), cpu, comm);
 
 	if (IS_ERR(kthread)) {
 		pr_err(BANNER "could not start sampling thread\n");
@@ -3238,6 +3252,7 @@ __init static int init_osnoise_tracer(void)
 		struct osnoise_variables *osn = cpu_osn_var(cpu);
 
 		mutex_init(&osn->lock);
+		osn->cpu = cpu;
 	}
 
 	cpumask_copy(&osnoise_cpumask, cpu_all_mask);
-- 
2.56.0

[PATCH v7 08/10] tracing/osnoise: Take trace_types_lock in timerlat_fd_open

[Crystal Wood]

This prevents a user fd from being opened when osnoise_options_write()
has already stopped threads but not yet set OSN_WORKLOAD, and matches what
all callers of start_kthread() do.

The original intent was probably for interface_lock to cover things like
this, but it never covered the full race window, and currently can't
due to lock order issues stemming from using interface_lock in
osnoise_sleep().  There does not appear to be any remaining reason
to take interface_lock in timerlat_fd_open(), so remove it.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v2: convert rest of function to guards

Do we really need that interface_lock in osnoise_sleep()?  Technically
we should probably be grabbing it every time we access osnoise_data, but
we don't seem to do so anywhere else.  Perhaps this was due to concerns
about the atomicity of "period - runtime", but that's just synchronization
theater given that userspace has no way of writing to both atomically.

In any case, we shouldn't use the same lock for protecting osnoise_data
members that we use for major reconfiguration procedures.
---
 kernel/trace/trace_osnoise.c | 43 +++++++++---------------------------
 1 file changed, 10 insertions(+), 33 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index a81992c937fb..b2b9db69692d 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -2494,29 +2494,24 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	struct osnoise_variables *osn_var;
 	struct timerlat_variables *tlat;
 	long cpu = (long) inode->i_cdev;
-	int ret = 0;
 
-	mutex_lock(&interface_lock);
+	guard(mutex)(&trace_types_lock);
 
 	/*
 	 * This file is accessible only if timerlat is enabled, and
 	 * NO_OSNOISE_WORKLOAD is set.
 	 */
-	if (!timerlat_enabled() || test_bit(OSN_WORKLOAD, &osnoise_options)) {
-		mutex_unlock(&interface_lock);
+	if (!timerlat_enabled() || test_bit(OSN_WORKLOAD, &osnoise_options))
 		return -EINVAL;
-	}
 
-	migrate_disable();
+	guard(migrate)();
 
 	osn_var = this_cpu_osn_var();
 	tlat = this_cpu_tmr_var();
-	mutex_lock(&osn_var->lock);
+	guard(mutex)(&osn_var->lock);
 
-	if (osn_var->fdd) {
-		ret = -EBUSY;
-		goto err;
-	}
+	if (osn_var->fdd)
+		return -EBUSY;
 
 	WARN_ON_ONCE(osn_var->kthread);
 
@@ -2527,16 +2522,12 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	 * worth doing this basic acceptance test to avoid obviusly wrong
 	 * setup.
 	 */
-	if (current->nr_cpus_allowed > 1 ||  cpu != smp_processor_id()) {
-		ret = -EPERM;
-		goto err;
-	}
+	if (current->nr_cpus_allowed > 1 ||  cpu != smp_processor_id())
+		return -EPERM;
 
 	fdd = kmalloc(sizeof(*fdd), GFP_KERNEL);
-	if (!fdd) {
-		ret = -ENOMEM;
-		goto err;
-	}
+	if (!fdd)
+		return -ENOMEM;
 
 	/*
 	 * From now on, it is good to go.
@@ -2558,21 +2549,7 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	tlat->thread = current;
 
 	hrtimer_setup(&tlat->timer, timerlat_irq, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
-
-	/*
-	 * Setup is done.
-	 */
-	mutex_unlock(&osn_var->lock);
-	mutex_unlock(&interface_lock);
-
-	migrate_enable();
 	return 0;
-
-err:
-	mutex_unlock(&osn_var->lock);
-	mutex_unlock(&interface_lock);
-	migrate_enable();
-	return ret;
 };
 
 /*
-- 
2.56.0

[PATCH v7 09/10] tracing/osnoise: Print taint message for timerlat

[Crystal Wood]

Provide taint information the same as is done in print_osnoise_headers().

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v2: new patch
---
 kernel/trace/trace_osnoise.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index b2b9db69692d..4fbad6ab2adf 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -582,6 +582,9 @@ static void record_osnoise_sample(struct osnoise_sample *sample)
 #ifdef CONFIG_PREEMPT_RT
 static void print_timerlat_headers(struct seq_file *s)
 {
+	if (osnoise_data.tainted)
+		seq_puts(s, "# timerlat is tainted!\n");
+
 	seq_puts(s, "#                                _-------=> irqs-off\n");
 	seq_puts(s, "#                               / _------=> need-resched\n");
 	seq_puts(s, "#                              | / _-----=> need-resched-lazy\n");
@@ -599,6 +602,9 @@ static void print_timerlat_headers(struct seq_file *s)
 #else /* CONFIG_PREEMPT_RT */
 static void print_timerlat_headers(struct seq_file *s)
 {
+	if (osnoise_data.tainted)
+		seq_puts(s, "# timerlat is tainted!\n");
+
 	seq_puts(s, "#                                _-----=> irqs-off\n");
 	seq_puts(s, "#                               / _----=> need-resched\n");
 	seq_puts(s, "#                              | / _---=> hardirq/softirq\n");
-- 
2.56.0

[PATCH v7 10/10] tracing/osnoise: Refuse timerlat fd open if tracer inactive

[Crystal Wood]

Besides being generally bad, allowing an fd open when there are no active
instances means that osnoise_options_write() and osnoise_cpus_write() will
leave it active during reconfiguration, causing at least WARN splats.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v2: new patch
---
 kernel/trace/trace_osnoise.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 4fbad6ab2adf..3876e8cb51d8 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -2507,7 +2507,8 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	 * This file is accessible only if timerlat is enabled, and
 	 * NO_OSNOISE_WORKLOAD is set.
 	 */
-	if (!timerlat_enabled() || test_bit(OSN_WORKLOAD, &osnoise_options))
+	if (!osnoise_has_registered_instances() || !timerlat_enabled() ||
+	    test_bit(OSN_WORKLOAD, &osnoise_options))
 		return -EINVAL;
 
 	guard(migrate)();
-- 
2.56.0

Re: [PATCH v7 03/10] tracing/osnoise: Start all eligible cpus from hotplug workfn

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] tracing/osnoise: missing Fixes trailer for
  osnoise_hotplug_workfn() race fix
--

commit 20aa2424de0f956e6a71e5fe659aee66ed386374
Author: Crystal Wood <crwood@redhat.com>

> tracing/osnoise: Start all eligible cpus from hotplug workfn
>
> Previously, a start could be lost if multiple cpus get onlined
> before the workqueue has a chance to run.
>
> Signed-off-by: Crystal Wood <crwood@redhat.com>

[Severity: Low]
This fixes a race in the CPU hotplug workqueue handling where eligible CPUs
can be left without a running kthread if multiple CPUs come online before
osnoise_hotplug_work runs.  Should this include:

  Fixes: c8895e271f79 ("trace/osnoise: Support hotplug operations")

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

Re: [PATCH v7 10/10] tracing/osnoise: Refuse timerlat fd open if tracer inactive

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] tracing/osnoise: missing Fixes: trailer for timerlat_fd_open()
  fix
--

commit 2b08a7c2950ae2b8e79253b75767a234cab88c0c
Author: Crystal Wood <crwood@redhat.com>

> tracing/osnoise: Refuse timerlat fd open if tracer inactive
>
> Besides being generally bad, allowing an fd open when there are no active
> instances means that osnoise_options_write() and osnoise_cpus_write() will
> leave it active during reconfiguration, causing at least WARN splats.
>
> Signed-off-by: Crystal Wood <crwood@redhat.com>

[Severity: Low]
This fixes a bug in timerlat_fd_open() where a user fd can be opened while
no instances are registered.  Should this include:

  Fixes: e88ed227f639 ("tracing/timerlat: Add user-space interface")

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

Re: [PATCH v7 05/10] tracing/osnoise: Per-cpu mutexes and fd detachment

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] tracing/osnoise: missing migration detection in
  timerlat_fd_read()
--

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

tracing/osnoise: Per-cpu mutexes and fd detachment

This commit introduces per-cpu mutexes to protect kthread and timerlat file
descriptor state, replacing the SIGKILL mechanism for user-space timerlat
threads with a mutex-protected detachment mechanism.

[Severity: Medium]
In kernel/trace/trace_osnoise.c, timerlat_fd_read() removes the
cpu != smp_processor_id() migration handling (which set
cpu_tmr_var(cpu)->uthread_migrate = 1, tainted osnoise, and stopped
tracing) and instead locks this_cpu_tmr_var()->detach_lock before checking
fdd != osn_var->fdd or tlat->uthread_migrate.

Can this miss user-space thread migrations when NO_OSNOISE_WORKLOAD is
enabled after the timerlat tracer has already been started?

By default, OSN_WORKLOAD is set in osnoise_options. If the user enables
timerlat first and then writes NO_OSNOISE_WORKLOAD to osnoise/options (as
shown in Documentation/trace/timerlat-tracer.rst), osnoise_options_write()
does not call register_migration_monitor(), which only runs during initial
tracer startup in osnoise_workload_start() -> osnoise_hook_events() ->
hook_thread_events() -> register_migration_monitor() and skips registering
trace_sched_migrate_callback() when OSN_WORKLOAD is set.

Without trace_sched_migrate_callback() registered and with the migration
handling removed from timerlat_fd_read(), a thread that migrates to
another CPU and calls timerlat_fd_read() acquires the destination CPU's
tlat->detach_lock rather than fdd->cpu's lock, never sets
cpu_tmr_var(fdd->cpu)->uthread_migrate, and does not taint or stop
tracing; if the thread later migrates back to fdd->cpu, subsequent reads
continue succeeding instead of failing with -EINVAL.

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

Re: [PATCH v7 06/10] tracing/osnoise: Reset cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] tracing/osnoise: CPU hotplug race in timerlat_fd_open() leaves
  stale fdd
--

commit 25ee38a6983ff50cb9227e203eed4ba8758cefa4
Author: Crystal Wood <crwood@redhat.com>

tracing/osnoise: Reset cleanup

This commit reorganizes per-CPU and global state resets in the osnoise and
timerlat tracers when stopping kthreads, detaching file descriptors, or
restarting the tracer.

> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index e61f57230eefb..8db0697b69e4a 100644
> --- a/kernel/trace/trace_osnoise.c
> +++ b/kernel/trace/trace_osnoise.c
[ ... ]
> @@ -408,6 +380,39 @@ static struct osnoise_data {
>  #endif
>  };
>  
> +static inline void osn_reset_cpu(int cpu)
> +{
> +	struct osnoise_variables *osn = cpu_osn_var(cpu);
> +
> +	WARN_ON_ONCE(osn->kthread);
> +	WARN_ON_ONCE(osn->fdd);
> +
> +	osn->sampling = false;
> +	osn->pid = 0;
> +	tlat_reset_cpu(cpu);
> +}
> +
> +/*
> + * osn_var_reset - Reset non-per-cpu state, and check for stale data
> + */
> +static inline void osn_var_reset(void)
> +{
> +	struct osnoise_variables *osn;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		osn = cpu_osn_var(cpu);
> +
> +		WARN_ON_ONCE(osn->kthread);
> +		WARN_ON_ONCE(osn->fdd);
> +		WARN_ON_ONCE(osn->sampling);
> +		memset(&osn->zero, 0, sizeof(osn->zero));
> +	}
> +
> +	tlat_var_reset();
> +	osnoise_data.tainted = false;
> +}

[Severity: Low]
Can osn_var_reset() hit WARN_ON_ONCE(osn->fdd) if timerlat_fd_open() races
with CPU hotplug takedown?

timerlat_fd_open() disables migration without holding cpus_read_lock() or
checking whether the CPU is still active under osn_var->lock.  If a task
pinned to CPU0 enters timerlat_fd_open() just as CPU0 is being offlined,
balance_push() skips the task while migration is disabled, allowing
osnoise_cpu_die() to run before timerlat_fd_open() attaches osn_var->fdd:

CPU0 (user task)                  CPU0 (cpuhp/0)
timerlat_fd_open()
  migrate_disable()
                                  sched_cpu_deactivate()
                                  ...
                                  osnoise_cpu_die()
                                    stop_kthread(0)
                                  sched_cpu_wait_empty()
                                    balance_hotplug_wait()
  mutex_lock(&osn_var->lock)
  osn_var->fdd = fdd
  tlat->thread = current
  mutex_unlock(&osn_var->lock)
  migrate_enable()
                                  <takedown completes>

When the tracer is later stopped and restarted, stop_per_cpu_kthreads()
only iterates over online CPUs and misses offline CPU0, so osn_var_reset()
iterating over for_each_possible_cpu() will trigger WARN_ON_ONCE(osn->fdd)
with stale fdd and tlat->thread state left attached on the offline CPU.

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