Re: [Qemu-devel] [PATCH 2/2] trace: check backend dstate in trace_event_get_state()

[Lluís Vilanova]

Daniel P Berrange writes:

> On Fri, Jul 28, 2017 at 10:20:53AM +0100, Stefan Hajnoczi wrote:
>> Code that checks dstate is unaware of SystemTap and LTTng UST dstate, so
>> the following trace event will not fire when solely enabled by SystemTap
>> or LTTng UST:
>> 
>> if (trace_event_get_state(TRACE_MY_EVENT)) {
>> str = g_strdup_printf("Expensive string to generate ...",
>> ...);
>> trace_my_event(str);
>> g_free(str);
>> }
>> 
>> Most users of trace_event_get_state() want to know both QEMU and backend
>> dstate for the event.  Update the macro to include backend dstate.
>> 
>> Introduce trace_event_get_state_qemu() for those callers who really only
>> want QEMU dstate.  This includes the trace backends (like 'log') which
>> should only trigger when event are enabled via QEMU.

> I'm not convinced this is quite right as is.

> The QEMU state for an event is set via cli flags to -trace and that
> is intended for use with with things like simpletrace or log which
> have no other way to filter which events are turned on at runtime.
> For these calling trace_event_get_state_dynamic_by_id() is right.

> For DTrace / LTT-NG, and event is active if the kernel has turned
> on the dynamic probe location.  Any command line args like -trace
> should not influence this at all, so we should *not* involve QEMU
> state at all - only the backend state. So for these we should only
> be calling the backend specific test and ignoring
> trace_event_get_state_dynamic_by_id() entirely. Your patch though
> makes it consider both.

I think this is because Stefan's proposal is to have code written into QEMU's
code that needs to know if *any+ backend has that event enabled, in order to
know if the expensive argument must be generated. Remember, that's regular QEMU
code, not auto-generated.

I would also try to minimise changes and name this new functionality as
trace_event_get_state_all, _any, _backends or something on those
lines. Otherwise we'll break the consistency of the API with other functions
(e.g., trace_event_get_vcpu_state only checks QEMU's state, and will always do
so because external backends do not support that feature).


Thanks,
  Lluis



>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>> trace/control.h                     | 20 ++++++++++++++++++--
>> scripts/tracetool/backend/ftrace.py |  2 +-
>> scripts/tracetool/backend/log.py    |  3 ++-
>> scripts/tracetool/backend/simple.py |  2 +-
>> scripts/tracetool/backend/syslog.py |  3 ++-
>> 5 files changed, 24 insertions(+), 6 deletions(-)
>> 
>> diff --git a/trace/control.h b/trace/control.h
>> index b931824d60..b996c34c08 100644
>> --- a/trace/control.h
>> +++ b/trace/control.h
>> @@ -93,16 +93,32 @@ static bool trace_event_is_vcpu(TraceEvent *ev);
>> static const char * trace_event_get_name(TraceEvent *ev);
>> 
>> /**
>> + * trace_event_get_state_qemu:
>> + * @id: Event identifier name.
>> + *
>> + * Get the tracing state of an event, both static and the QEMU dynamic 
>> state.
>> + * Note that some backends maintain their own dynamic state, use
>> + * trace_event_get_state() instead if you wish to include it.
>> + *
>> + * If the event has the disabled property, the check will have no 
>> performance
>> + * impact.
>> + */
>> +#define trace_event_get_state_qemu(id)                       \
>> +    ((id ##_ENABLED) && trace_event_get_state_dynamic_by_id(id))
>> +
>> +/**
>> * trace_event_get_state:
>> * @id: Event identifier name.
>> *
>> - * Get the tracing state of an event (both static and dynamic).
>> + * Get the tracing state of an event (both static and dynamic).  Both QEMU 
>> and
>> + * backend-specific dynamic state are included.
>> *
>> * If the event has the disabled property, the check will have no performance
>> * impact.
>> */
>> #define trace_event_get_state(id)                       \
>> -    ((id ##_ENABLED) && trace_event_get_state_dynamic_by_id(id))
>> +    ((id ##_ENABLED) && (trace_event_get_state_dynamic_by_id(id) || \
>> +                         id ##_BACKEND_DSTATE()))
>> 
>> /**
>> * trace_event_get_vcpu_state:
>> diff --git a/scripts/tracetool/backend/ftrace.py 
>> b/scripts/tracetool/backend/ftrace.py
>> index dd0eda4441..fba637b376 100644
>> --- a/scripts/tracetool/backend/ftrace.py
>> +++ b/scripts/tracetool/backend/ftrace.py
>> @@ -33,7 +33,7 @@ def generate_h(event, group):
>> '        char ftrace_buf[MAX_TRACE_STRLEN];',
>> '        int unused __attribute__ ((unused));',
>> '        int trlen;',
>> -        '        if (trace_event_get_state(%(event_id)s)) {',
>> +        '        if (trace_event_get_state_qemu(%(event_id)s)) {',
>> '            trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
>> '                             "%(name)s " %(fmt)s "\\n" %(argnames)s);',
>> '            trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
>> diff --git a/scripts/tracetool/backend/log.py 
>> b/scripts/tracetool/backend/log.py
>> index 54f0a69886..4562b9d12d 100644
>> --- a/scripts/tracetool/backend/log.py
>> +++ b/scripts/tracetool/backend/log.py
>> @@ -33,7 +33,8 @@ def generate_h(event, group):
>> # already checked on the generic format code
>> cond = "true"
>> else:
>> -        cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
>> +        cond = "trace_event_get_state_qemu(%s)" % \
>> +               ("TRACE_" + event.name.upper())
>> 
>> out('    if (%(cond)s) {',
>> '        struct timeval _now;',
>> diff --git a/scripts/tracetool/backend/simple.py 
>> b/scripts/tracetool/backend/simple.py
>> index f983670ee1..a39bbdc5c6 100644
>> --- a/scripts/tracetool/backend/simple.py
>> +++ b/scripts/tracetool/backend/simple.py
>> @@ -73,7 +73,7 @@ def generate_c(event, group):
>> # already checked on the generic format code
>> cond = "true"
>> else:
>> -        cond = "trace_event_get_state(%s)" % event_id
>> +        cond = "trace_event_get_state_qemu(%s)" % event_id
>> 
>> out('',
>> '    if (!%(cond)s) {',
>> diff --git a/scripts/tracetool/backend/syslog.py 
>> b/scripts/tracetool/backend/syslog.py
>> index 1ce627f0fc..3ee07bf7fd 100644
>> --- a/scripts/tracetool/backend/syslog.py
>> +++ b/scripts/tracetool/backend/syslog.py
>> @@ -33,7 +33,8 @@ def generate_h(event, group):
>> # already checked on the generic format code
>> cond = "true"
>> else:
>> -        cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
>> +        cond = "trace_event_get_state_qemu(%s)" % \
>> +               ("TRACE_" + event.name.upper())
>> 
>> out('    if (%(cond)s) {',
>> '        syslog(LOG_INFO, "%(name)s " %(fmt)s %(argnames)s);',
>> -- 
>> 2.13.3
>> 
>> 

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