Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] tracing/osnoise: unreset delta_start and arrival_time in osn_reset_cpu() - [Medium] tracing/osnoise: race on osn->pid in timerlat_fd_detach() vs migration check --
commit fffc251bf1aed17ce8e75b3b4c7d5990c0ecad11 Author: Crystal Wood <[email protected]> tracing/osnoise: Split out per-cpu variable resets This commit splits per-CPU variable resets out of osn_var_reset() and tlat_var_reset() into osn_reset_cpu() and tlat_reset_cpu(), resetting per-CPU state whenever stopping a kthread or detaching an fd. It also moves the interrupt and thread tracking structures out of the zeroed group in struct osnoise_variables. > diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c > index cd386c75926e4..1abe2c3507f76 100644 > --- a/kernel/trace/trace_osnoise.c > +++ b/kernel/trace/trace_osnoise.c > @@ -237,15 +237,15 @@ struct osnoise_variables { > struct mutex lock; /* covers kthread and fdd changes */ > struct task_struct *kthread; > struct fd_data *fdd; > + struct osn_nmi nmi; > + struct osn_irq irq; > + struct osn_softirq softirq; > + struct osn_thread thread; > + local_t int_counter; > > 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; > ); > }; [Severity: Medium] Can moving nmi, irq, softirq, and thread out of struct_group(zero, ...) without resetting their delta_start and arrival_time fields in osn_reset_cpu() leave stale non-zero timestamps across runs? In addition to count, struct osn_nmi, struct osn_irq, struct osn_softirq, and struct osn_thread contain arrival_time and delta_start. A non-zero delta_start is used to detect an active window in cond_move_irq_delta_start(), cond_move_softirq_delta_start(), cond_move_thread_delta_start(), and timerlat_irq(): if (!IS_ENABLED(CONFIG_PREEMPT_RT) && osn_var->softirq.delta_start) { copy_int_safe_time(osn_var, &osn_var->thread.delta_start, &osn_var->softirq.delta_start); copy_int_safe_time(osn_var, &osn_var->softirq.delta_start, &osn_var->irq.delta_start); } Normally, delta_start and arrival_time are cleared to zero in osnoise_trace_irq_exit(), trace_softirq_exit_callback(), and thread_exit(). However, all three exit callbacks return early when osn_var->sampling is false: if (!osn_var->sampling) return; If osn_var->sampling is cleared while a measurement window is open (for example, when timerlat_main() exits while sampling is still true and schedules in the next task via thread_entry() before stop_kthread() runs, when NO_OSNOISE_WORKLOAD or timerlat_fd stops while a task is running on the CPU, or when timerlat_tracer_stop() clears sampling from another CPU while a softirq or IRQ is in flight), the exit callback skips clearing delta_start and arrival_time. On a subsequent timerlat or NO_OSNOISE_WORKLOAD run, won't a stale non-zero osn_var->softirq.delta_start cause timerlat_irq() to misdetect softirq preemption and overwrite thread.delta_start on every period, or a stale osn_var->thread.delta_start corrupt the first thread_exit() duration? [ ... ] > @@ -2452,25 +2452,22 @@ static void timerlat_fd_detach(struct fd_data *fdd) > if (WARN_ON(fdd != osn->fdd)) > return; > > - WARN_ON(osn->kthread); > WARN_ON(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); > + > } [Severity: Medium] Does removing osn->pid = 0 from timerlat_fd_detach() open a race window with trace_sched_migrate_callback() before osn_reset_cpu() runs? In timerlat_fd_detach(), waking tlat->thread and unlocking tlat->detach_lock allows the user thread in timerlat_fd_read() to finish its read path, call migrate_enable(), and return to userspace (or get pushed off a dying CPU during hotplug via osnoise_cpu_die() -> stop_kthread() -> timerlat_fd_detach()) before the caller invokes osn_reset_cpu(cpu) to zero osn->pid. While osn->pid still holds the detaching thread's PID, trace_sched_migrate_callback() checks osn_var->pid locklessly: osn_var = cpu_osn_var(cpu); if (osn_var->pid == p->pid && dest_cpu != cpu) { cpu_tmr_var(cpu)->uthread_migrate = 1; osnoise_taint("timerlat user-thread migrated\n"); osnoise_stop_exception("timerlat user-thread migrated", cpu); } If the detached thread migrates in that window before osn_reset_cpu() clears osn->pid, won't trace_sched_migrate_callback() falsely taint and stop tracing? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
