Re: [PATCH v6 06/10] tracing/osnoise: Split out per-cpu variable resets

[sashiko-bot@kernel.org]

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 <crwood@redhat.com>

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/20261028220120.4005187-1-crwood@redhat.com?part=6