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
