Re: [PATCH v2 2/2] tracing/snapshot: Use work_on_cpu to avoid -EBUSY during snapshot swap

[Steven Rostedt <rostedt@goodmis.org>]

Sashiko brings up a good point. This will break the recording of what
task is being interrupted.

  https://sashiko.dev/#/patchset/20260803005640.2445666-2-wutengda%40huaweicloud.com


> --- Patch [2]: [PATCH v2 2/2] tracing/snapshot: Use work_on_cpu to avoid -EBUSY during snapshot swap ---
> commit b61ca8359fdea427c80a410f6d4052c68af5eae3
> Author: Tengda Wu <wutengda@huaweicloud.com>
> 
> tracing/snapshot: Use work_on_cpu to avoid -EBUSY during snapshot swap
> 
> This commit replaces smp_call_function_single() with work_on_cpu() in
> the snapshot write path to avoid -EBUSY errors when a target CPU is
> actively writing to the ring buffer.
> 
> > diff --git a/kernel/trace/trace_snapshot.c b/kernel/trace/trace_snapshot.c
> > --- a/kernel/trace/trace_snapshot.c
> > +++ b/kernel/trace/trace_snapshot.c
> 
> [ ... ]
> 
> > -static void tracing_swap_cpu_buffer(void *tr)
> > +static long tracing_swap_cpu_buffer(void *tr)
> >  {
> > +	local_irq_disable();
> >  	update_max_tr_single((struct trace_array *)tr, current, smp_processor_id());
> 
> When calling update_max_tr_single() from a workqueue thread, does passing
> current result in the trace snapshot recording the kworker task instead of
> the interrupted task?
> 
> In the previous smp_call_function_single() implementation, current would
> represent the task executing on the target CPU, but now it will capture
> the kworker thread.

And thinking of one usecase, what if you had a high priroity task you
wanted to snashot? This wouldn't work to do that.

Now we can still keep the ipi and send data that holds the tr and
state. It waits for the ipi to finish anyway. Then it can check if the
swap happened or not, and could report a fail, or it could try again?

> 
> > +	local_irq_enable();
> > +	return 0;
> >  }
> 
> [ ... ]
> 
> > @@ -689,8 +692,7 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
> >  			update_max_tr(tr, current, smp_processor_id(), NULL);
> >  			local_irq_enable();
> >  		} else {
> > -			smp_call_function_single(iter->cpu_file, tracing_swap_cpu_buffer,
> > -						 (void *)tr, 1);
> > +			work_on_cpu(iter->cpu_file, tracing_swap_cpu_buffer, (void *)tr);
> 
> Can this result in swapping the wrong CPU's trace ring buffer if
> iter->cpu_file is an offline CPU, or if the CPU goes offline concurrently?
> 
> Because cpus_read_lock() is not held, if the target CPU is offline, the
> workqueue subsystem may execute the work on a fallback online CPU.
> 
> When tracing_swap_cpu_buffer() then calls smp_processor_id(), it will
> evaluate to the fallback CPU's ID rather than the intended target CPU.
> 
> This could potentially modify an unrelated online CPU's trace buffer while
> leaving the requested offline CPU's buffer unchanged.

This too looks to be a valid concern.

> 
> >  		}
> >  		tracing_disarm_snapshot(tr);
> >  		break;

-- Steve