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

On 2026/8/8 22:58, Steven Rostedt wrote:
> 
> 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 <[email protected]>
>>
>> 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?
> 

Indeed, work_on_cpu() cannot handle these scenarios properly.

I just tried handling the return value and noticed that there is already
a log message inside update_max_tr_single() that reports the
ring_buffer_swap_cpu() error.
(I had previously overlooked this error handling and didn't notice it.)

So, it seems that no additional handling is necessary. I will keep the
current implementation unchanged.

Thank you for your accurate catch and thoughtful suggestions.

Best regards,
Tengda

>>
>>> +	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