On Thu, Jun 5, 2025 at 9:57 AM Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> On 6/3/25 00:24, Stefan Hajnoczi wrote:
> > On Sun, Jun 01, 2025 at 06:12:30PM +0000, Tanish Desai wrote:
> >> Moved rarely used (cold) code from the header file to the C file to avoid
> >> unnecessary inlining and reduce binary size.
> >
> > How much of a binary size reduction do you measure? Most trace events
> > are only called once, so the difference in code size is likely to be
> > small.
>
> Indeed I don't think there would be much reduction. I expect a bigger
> benefit in terms of improved register allocation when tracepoints are
> disabled, keeping unused tracepoint code out of the TLB, and the like.
>
> That is, the difference in code size would be in the functions that
> include tracepoints, not in QEMU overall. That's a bit difficult to
> measure because you would have to isolate tracepoint code in the
> "before" .o files, but we can try.
>
> >> This improves code organization
> >> and follows good practices for managing cold paths.
> >
> > 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.
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.
Stefan
>
> Paolo
>
>
> >> Signed-off-by: Tanish Desai <tanishdesa...@gmail.com>
> >> ---
> >> scripts/tracetool/backend/ftrace.py | 44 +++++++++++++++++++++--------
> >> 1 file changed, 32 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/scripts/tracetool/backend/ftrace.py
> >> b/scripts/tracetool/backend/ftrace.py
> >> index baed2ae61c..c9717d7b42 100644
> >> --- a/scripts/tracetool/backend/ftrace.py
> >> +++ b/scripts/tracetool/backend/ftrace.py
> >> @@ -23,6 +23,10 @@
> >> def generate_h_begin(events, group):
> >> out('#include "trace/ftrace.h"',
> >> '')
> >> + for event in events:
> >> + out('void _ftrace_%(api)s(%(args)s);',
> >> + api=event.api(),
> >> + args=event.args)
> >>
> >>
> >> def generate_h(event, group):
> >> @@ -30,26 +34,42 @@ def generate_h(event, group):
> >> if len(event.args) > 0:
> >> argnames = ", " + argnames
> >>
> >> - out(' {',
> >> + out(' if (trace_event_get_state(%(event_id)s)) {',
> >> + ' _ftrace_%(api)s(%(args)s);',
> >> + ' }',
> >> + name=event.name,
> >> + args=", ".join(event.args.names()),
> >> + event_id="TRACE_" + event.name.upper(),
> >> + event_lineno=event.lineno,
> >> + event_filename=os.path.relpath(event.filename),
> >> + fmt=event.fmt.rstrip("\n"),
> >> + argnames=argnames,
> >> + api=event.api()
> >> + )
> >> +
> >> +
> >> +def generate_c(event, group):
> >> + argnames = ", ".join(event.args.names())
> >> + if len(event.args) > 0:
> >> + argnames = ", " + argnames
> >> + out('void _ftrace_%(api)s(%(args)s){',
> >> ' char ftrace_buf[MAX_TRACE_STRLEN];',
> >> ' int unused __attribute__ ((unused));',
> >> ' int trlen;',
> >> - ' if (trace_event_get_state(%(event_id)s)) {',
> >> '#line %(event_lineno)d "%(event_filename)s"',
> >> - ' trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
> >> - ' "%(name)s " %(fmt)s "\\n"
> >> %(argnames)s);',
> >> + ' trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
> >> '#line %(out_next_lineno)d "%(out_filename)s"',
> >> - ' trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
> >> - ' unused = write(trace_marker_fd, ftrace_buf, trlen);',
> >> - ' }',
> >> - ' }',
> >> - name=event.name,
> >> - args=event.args,
> >> - event_id="TRACE_" + event.name.upper(),
> >> + ' "%(name)s " %(fmt)s "\\n" %(argnames)s);',
> >> + ' trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
> >> + ' unused = write(trace_marker_fd, ftrace_buf, trlen);',
> >> + '}',
> >> event_lineno=event.lineno,
> >> event_filename=os.path.relpath(event.filename),
> >> + name=event.name,
> >> fmt=event.fmt.rstrip("\n"),
> >> - argnames=argnames)
> >> + argnames=argnames,
> >> + api=event.api(),
> >> + args=event.args)
> >>
> >>
> >> def generate_h_backend_dstate(event, group):
> >> --
> >> 2.34.1
> >>
>
>
