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 <stefa...@redhat.com>
>> ---
>> 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 :|
