Re: [PATCH] tracing: perf: Fix stale head for perf syscall tracing

[Steven Rostedt]

From Sashiko: https://sashiko.dev/#/patchset/20260724184250.357e91f1%40gandalf.local.home

> --- Patch [1]: [PATCH] tracing: perf: Fix stale head for perf syscall tracing ---
> commit c99950733eb63d5661f5e4d25f82dff506833b49
> Author: Steven Rostedt <rostedt@xxxxxxxxxxx>
> 
> tracing: perf: Fix stale head for perf syscall tracing
> 
> This commit delays capturing the head of the per CPU perf events list until
> after the faulting code completes. This prevents the head pointer from pointing
> to the wrong CPU event if the syscall data read enables preemption and migrates.
> 
> Link: https://sashiko.dev/#/patchset/20260717173252.3431565-1-usama.arif%40linux.dev
> 
> > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> > index e98ee7e1e66f4..e541e964d1930 100644
> > --- a/kernel/trace/trace_syscalls.c
> > +++ b/kernel/trace/trace_syscalls.c
> > @@ -1440,19 +1440,20 @@ static void perf_syscall_enter(void *ignore, struct pt_regs *regs, long id)
> 
> [ ... ]
> 
> >  	guard(preempt_notrace)();
> >  
> > -	head = this_cpu_ptr(sys_data->enter_event->perf_events);
> > -	if (hlist_empty(head))
> > -		return;
> > -
> >  	/* Check if this syscall event faults in user space memory */
> >  	mayfault = sys_data->user_mask != 0;
> >  
> >  	if (mayfault) {
> > +		/* Enables preemption and may migrate */
> >  		if (syscall_get_data(sys_data, args, &user_ptr,
> >  				     &size, user_sizes, &uargs, buf_size) < 0)
> >  			return;
> >  	}
> >  
> > +	head = this_cpu_ptr(sys_data->enter_event->perf_events);
> > +	if (hlist_empty(head))
> > +		return;
> 
> Will moving this early check below syscall_get_data() cause a severe
> performance regression for untraced CPUs?

I was thinking this might be the case. I originally thought it wouldn't be
too much of an issue, but I can see that the reading of user space when you
don't need to would be a regression in performance.

I'll change the above to be:

	guard(preempt_notrace)();

	head = this_cpu_ptr(sys_data->enter_event->perf_events);
	if (hlist_empty(head))
		return;

	/* Check if this syscall event faults in user space memory */
	mayfault = sys_data->user_mask != 0;

	if (mayfault) {
		if (syscall_get_data(sys_data, args, &user_ptr,
				     &size, user_sizes, &uargs, buf_size) < 0)
			return;

		/* The above may have caused a migration */
		head = this_cpu_ptr(sys_data->enter_event->perf_events);
		if (hlist_empty(head))
			return;
	}

-- Steve



> 
> Because the enabled_perf_enter_syscalls bitmap is global, enabling a perf
> syscall trace on one CPU causes perf_syscall_enter() to be invoked on all
> CPUs executing that syscall.
> 
> Previously, the early hlist_empty() check allowed CPUs not actively tracing
> the event to return immediately.
> 
> By moving this check until after syscall_get_data(), doesn't this force all
> untraced CPUs to unconditionally execute expensive user-space memory copies
> and preemption toggling before discarding the data?