Re: [PATCH 08/16] tracetool: Add Rust format support

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Sep 24, 2025 at 07:49:49AM -0400, Stefan Hajnoczi wrote:
> On Wed, Sep 24, 2025 at 3:51 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Sep 24, 2025 at 09:13:15AM +0200, Paolo Bonzini wrote:
> > > On 9/23/25 21:23, Stefan Hajnoczi wrote:
> > > > > +    out('// SPDX-License-Identifier: GPL-2.0-or-later',
> > > > > +        '// This file is @generated by tracetool, do not edit.',
> > > > > +        '',
> > > > > +        '#[allow(unused_imports)]',
> > > > > +        'use std::ffi::c_char;',
> > > > > +        '#[allow(unused_imports)]',
> > > > > +        'use util::bindings;',
> > > > > +        '',
> > > > > +        '#[inline(always)]',
> > > > > +        'fn trace_event_get_state_dynamic_by_id(_id: u16) -> bool {',
> > > > > +        '    unsafe { (trace_events_enabled_count != 0) && (_id != 0) }',
> > > > > +        '}',
> > > >
> > > > This was translated to Rust from:
> > > >
> > > >    /* it's on fast path, avoid consistency checks (asserts) */
> > > >    #define trace_event_get_state_dynamic_by_id(id) \
> > > >        (unlikely(trace_events_enabled_count) && _ ## id ## _DSTATE)
> > > >
> > > > The _id != 0 expression is incorrect. The purpose was to check whether
> > > > the trace event is currently enabled (i.e. dynamically at runtime).
> > >
> > > The expression is correct, but the function and argument names are not. It
> > > should be
> > >
> > > fn trace_event_state_is_enabled(dstate: u16) -> bool {
> > >      unsafe { trace_events_enabled_count } != 0 && dstate != 0
> > > }
> > >
> > > > > +    # static state
> > > > > +    for e in events:
> > > > > +        if 'disable' in e.properties:
> > > > > +            enabled = "false"
> > > > > +        else:
> > > > > +            enabled = "true"
> > > >
> > > > What is the purpose of this loop? The variable enabled is unused so I
> > > > think it can be deleted.
> > >
> > > The Rust code generator is not emitting any code for disabled tracepoints.
> > > Unlike C, where the disabled tracepoints can produce e.g. -Wformat warnings,
> > > there's no real benefit here.
> > >
> > > In the RFC the "enabled" variable was used to produce a const for the static
> > > state; it had no user so I removed it, but I left behind this dead Python
> > > code.  Sorry about that!
> >
> > Is the concept of build time 'disabled' tracepoints actually useful to
> > still support ? AFAICT we use it in only places, which doesn't make it
> > sound too compelling:
> >
> > $ find -name trace-events | xargs grep '^disable'
> > ./target/hppa/trace-events:disable hppa_tlb_flush_ent(void *env, void *ent, uint64_t va_b, uint64_t va_e, uint64_t pa) "env=%p ent=%p va_b=0x%lx va_e=0x%lx pa=0x%lx"
> > ./target/hppa/trace-events:disable hppa_tlb_find_entry(void *env, void *ent, int valid, uint64_t va_b, uint64_t va_e, uint64_t pa) "env=%p ent=%p valid=%d va_b=0x%lx va_e=0x%lx pa=0x%lx"
> > ./target/hppa/trace-events:disable hppa_tlb_find_entry_not_found(void *env, uint64_t addr) "env=%p addr=%08lx"
> > ...snip...
> > ./target/hppa/trace-events:disable hppa_tlb_lpa_success(void *env, uint64_t addr, uint64_t phys) "env=%p addr=0x%lx phys=0x%lx"
> > ./target/hppa/trace-events:disable hppa_tlb_lpa_failed(void *env, uint64_t addr) "env=%p addr=0x%lx"
> > ./target/hppa/trace-events:disable hppa_tlb_probe(uint64_t addr, int level, int want) "addr=0x%lx level=%d want=%d"
> > ./hw/display/trace-events:disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
> 
> My recollection is that disabled events were supposed to eliminate the
> cost of trace events. Downstreams could disable trace events that they
> didn't need.

IMHO if that is a required use case, then the dtrace backend is the
answer.

The main premise of tracing is that you can rarely predict what info
will be needed ahead of time, so you need runtime controls. dtrace
gives that with near zero overhead, while QEMU's other backends have
that at the cost of a branch to check the enabled state.  Disabling
events at build time feels at odds with the intended use case of
tracing.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|