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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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).

Paolo


>> Signed-off-by: Tanish Desai <tanishdesai37@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
>>