Re: [PATCH 2/3] trace/ftrace: seperate cold paths of tracing functions

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

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 <pbonzini@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 :|