Re: [RFC 4/5] xen/tracing: Add a function tracer

[Ryo Takakura <takakura@xxxxxxxxxxxxx>]

On Sat, 24 Oct 2026 16:24:17 +0200, Bernhard Kaindl wrote:
>> --- /dev/null
>> +++ b/xen/common/function_trace.c
>> @@ -0,0 +1,274 @@
>> +    if ( this_cpu(function_trace_recursing) )
>> +        return;
>> +
>> +    this_cpu(function_trace_recursing) = true;
>> +
>> +    buf = &this_cpu(function_trace_buffer);
>> +    rec = &buf->records[buf->head];
>> [...]
>> +    buf->head = (buf->head + 1) % FUNCTION_TRACE_BUFSIZE;
>> +    buf->written++;
>> +
>> +    this_cpu(function_trace_recursing) = false;
>
>What ensures that a callback which has already passed the dispatcher's
>tracing_on check has finished before another CPU reads or clears its
>buffer? The tracer registration does not appear to provide a stop hook:

There is currently no such synchronization in either hypfs_trace.c
or function_trace.c.

>> --- /dev/null
>> +++ b/xen/common/function_trace.c
>> @@ -0,0 +1,274 @@
>> +static struct tracer function_tracer = {
>> +    .name = "function",
>> +    .init = function_tracer_init,
>> +    .reset = function_tracer_reset,
>> +    .clear_trace = function_tracer_clear,
>> +    .get_trace_size = function_tracer_get_trace_size,
>> +    .print_trace = function_tracer_print_trace,
>> +};
>
>For reference, tracing_set_on() in xen/common/hypfs_trace.c, introduced
>by the preceding patch, disables recording with:
>
>>         if ( tracer->stop )
>>             rc = tracer->stop(&global_trace);
>>     }
>>
>>     if ( !rc )
>>         global_trace.tracing_on = on;
>
>Could an already admitted callback still hold a record slot when this
>clear resets its CPU's counters?

I see. Yes, the callback can still be running.

>> --- /dev/null
>> +++ b/xen/common/function_trace.c
>> @@ -0,0 +1,274 @@
>> +static int cf_check function_tracer_clear(struct trace_context *tr)
>> +{
>> +    struct function_trace_cpu_buffer *buf;
>> +    unsigned int cpu;
>> +
>> +    for_each_online_cpu ( cpu )
>> +    {
>> +        buf = &per_cpu(function_trace_buffer, cpu);
>> +        buf->head = 0;
>> +        buf->written = 0;
>> +    }
>
>If that callback then finishes writing its old slot and updates head
>and written, could the reset counters describe a different slot from
>the one just filled? Could the print path likewise observe a partially
>written record?

Yes, I think both can happen.

>The per-CPU recursion flag seems useful for nesting,
>but wouldn't it leave these remote accesses unsynchronized?

Yes, I added the flag only to prevent nesting.
It does not solve the issues you raised.

>Would a stop-and-drain protocol shared with callback removal let read,
>clear, and reset operate only once the buffers are stable?

Yes, I think extendeding the RCU read-side region which you
suggested in your feedback to the patch 3/5 can be used here as well.
I'll make sure callbacks are drained both when stopping tracing
and when removing the callback.

>There also seems to be a size-check consequence of a late writer:
>
>> --- /dev/null
>> +++ b/xen/common/function_trace.c
>> @@ -0,0 +1,274 @@
>> +    for_each_online_cpu ( cpu )
>> +    {
>> +        buf = &per_cpu(function_trace_buffer, cpu);
>> +        count = min(buf->written, (uint64_t)FUNCTION_TRACE_BUFSIZE);
>> +        size += count * FUNCTION_TRACE_RECORD_TEXT_SIZE;
>> +    }
>
>The existing hypfs_read() in xen/common/hypfs.c calculates the size and
>checks the guest-provided length before calling the reader:
>
>>     unsigned int size = entry->funcs->getsize(entry);
>> [...]
>>     ret = -ENOBUFS;
>>     if ( ulen < size + sizeof(e) )
>>         goto out;
>>
>>     guest_handle_add_offset(uaddr, sizeof(e));
>>
>>     ret = entry->funcs->read(entry, uaddr);
>
>The trace_read() callback in xen/common/hypfs_trace.c then samples the
>tracer size again, allocates that size, and later copies that amount:
>
>>     size = tracer->get_trace_size(&global_trace);
>> [...]
>>     buf = xzalloc_array(char, size);
>> [...]
>>     if ( copy_to_guest(uaddr, buf, size) )
>>         rc = -EFAULT;
>
>If a final callback increases written between these two size samples,
>while the ring is not yet full, could the second size exceed the length
>already checked for the request? Would an owned snapshot, or a shared
>stable-size protocol, keep the checked length and copy consistent?

Thank you for detailing the issue and the possible solution.
Yes, the second size could exceed the first size.
And to keep the size consistent, I'll make it to keep a snapshot
following your suggestion in the next version.

>Could the snapshot also use a stable CPU set? The sizing, clearing, and
>iterator construction each traverse for_each_online_cpu(), and the
>iterator retains buffer pointers while producing the output:
>
>xen/common/function_trace.c:
>> @@ -0,0 +1,274 @@
>> +    for_each_online_cpu ( cpu )
>> +    {
>> +        const struct function_trace_cpu_buffer *cbuf = 
>> &per_cpu(function_trace_buffer, cpu);
>> +        struct buffer_iterator *iter = &iters[nr_iters++];
>> +
>> +        iter->buffer = cbuf;
>> +        iter->count = min(cbuf->written, (uint64_t)FUNCTION_TRACE_BUFSIZE);
>> +        iter->start = cbuf->written >= FUNCTION_TRACE_BUFSIZE ? cbuf->head 
>> : 0;
>
>For reference, get_cpu_maps() in xen/common/cpu.c uses the CPU-hotplug
>lock rather than the HypFS lock:
>
>> bool get_cpu_maps(void)
>> {
>>     return read_trylock(&cpu_add_remove_lock);
>> }
>
>Would it make sense to pin the relevant CPU set while taking a snapshot,
>and use that same snapshot for sizing and rendering?

Yes, it makes sense.

>What should happen
>to records on a parked offline CPU? Could clearing only online buffers
>allow its old records to reappear when that CPU comes online again?

I see.
I'll make the buffers to be clear based on cpu_present_map.
I think this should prevent old records from reappearing
when a CPU comes online again.

>It looks like there is a design question of the stop-and-snapshot contract
>to drain callbacks safely in their permitted contexts and preserve
>one snapshot across HypFS's size check and copy. Locking each traversal
>separately would still leave the gap between the two size samples.

Understood. I'll make sure callbacks get drained, and keep a snapshot
that can be used for both sizing and copying.

>Thanks for checking these interactions. Is there an existing mechanism
>that already keeps the buffers and CPU set stable across these steps?

Thank you for your feedback. They are currently missing.
I'll make sure they get fixed in the next version.

Sincerely,
Ryo Takakura