On Thu, Jun 05, 2025 at 08:49:36PM +0200, Paolo Bonzini wrote:
> On 6/5/25 20:37, Stefan Hajnoczi wrote:
> > On Thu, Jun 5, 2025 at 9:57 AM Paolo Bonzini <pbonz...@redhat.com> wrote:
> > > > It's easier to understand the code generator and the generated code when
> > > > each trace event is implemented as a single function in the header file.
> > > > Splitting the trace event up adds complexity. I don't think this is a
> > > > step in the right direction.
> > >
> > > I am not sure I agree on that; something like
> > >
> > > static inline void trace_smmu_config_cache_inv(uint32_t sid)
> > > {
> > > if (trace_event_get_state(TRACE_SMMU_CONFIG_CACHE_INV)) {
> > > _simple__trace_smmu_config_cache_inv(sid);
> > > _log__trace_smmu_config_cache_inv(sid);
> > > }
> > > QEMU_SMMU_CONFIG_CACHE_INV(sid);
> > > tracepoint(qemu, smmu_config_cache_inv(sid));
> > > }
> > >
> > > and one function per backend seems the most readable way to format the
> > > code in the headers. I understand that most of the time you'll have
> > > only one backend enabled, but still the above seems pretty good and
> > > clarifies the difference between efficient backends like dtrace and UST
> > > and the others.
> > >
> > > This series doesn't go all the way to something like the above, but it
> > > does go in that direction.
> >
> > It's nice to share a single trace_event_get_state() conditional
> > between all backends that use it. There is no need to move the
> > generated code from .h into a .c file to achieve this though.
>
> Ok, I see what you mean. Personally I like that the backend code is
> completely out of sight and you only have a single line of code per backend;
> but it's a matter of taste I guess.
>
> > In the absence of performance data this patch series seems like
> > premature optimization and code churn to me.
> >
> > > Now, in all honesty the main reason to do this was to allow reusing the
> > > C code generator when it's Rust code that is using tracepoints; but I do
> > > believe that these changes make sense on their own, and I didn't want to
> > > make these a blocker for Rust enablement as well (Tanish has already
> > > looked into generating Rust code for the simple backend, for example).
> >
> > How is this patch series related to Rust tracing? If generated code
> > needs to be restructured so Rust can call it, then that's a strong
> > justification.
> Well, moving code to the .c file would make it possible to call it in Rust
> without duplicating code generation for the various backends (other than the
> "if" and function calls, of course, but those are easy). However, this is
> only handy and not absolutely necessary for the Rust tracing project.
This might work for some trace backends, but certainly for dtrace/systemtap
I'd expect us to use a native rust impl to get the optimal low overhead.
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 :|
