On Wed, Sep 24, 2025 at 3:51 AM Daniel P. Berrangé <berra...@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.
I'm not aware of any users besides the ones that your grep identified.
We could probably remove the disable keyword without any real impact
on users.
Stefan
