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

[Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>]

Hi Ryo,

Thanks for including a function tracer to try with the new interface.
I'd like to help check the buffer handling, with LLM assistance as I'm
still getting familiar with this area. I may have missed a guarantee
around stopping recording; could you help me check the following?

> --- /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:

> --- /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?

> --- /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? The per-CPU recursion flag seems useful for nesting,
but wouldn't it leave these remote accesses unsynchronized?

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

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?

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

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.

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

cu, Bernhard

-- 
Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>