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.
Signed-off-by: Stefan Hajnoczi <stefanha@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
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.
> Signed-off-by: Stefan Hajnoczi <stefanha@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 :|
On Fri, Jul 28, 2017 at 12:05:44PM +0100, Daniel P. Berrange wrote:
> 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 you're referring to this:
#define trace_event_get_state(id) \
((id ##_ENABLED) && (trace_event_get_state_dynamic_by_id(id) || \
id ##_BACKEND_DSTATE()))
We could change it to:
#define trace_event_get_state(id) \
((id ##_ENABLED) && id ##_BACKEND_DSTATE())
and modify the log, syslog, ftrace, etc backends to emit
trace_event_get_state_dynamic_by_id(id) as their backend dstate.
However, in the multi-backend case with ./configure
--enable-trace-backends=log,syslog this expands to:
((id ##_ENABLED) && (trace_event_get_state_dynamic_by_id(id) || \
trace_event_get_state_dynamic_by_id(id) || \
false))
I couldn't bring myself to have multiple calls to
trace_event_get_state_dynamic_by_id() :).
What do you think?
On Fri, Jul 28, 2017 at 04:56:49PM +0100, Stefan Hajnoczi wrote:
> On Fri, Jul 28, 2017 at 12:05:44PM +0100, Daniel P. Berrange wrote:
> > 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 you're referring to this:
>
> #define trace_event_get_state(id) \
> ((id ##_ENABLED) && (trace_event_get_state_dynamic_by_id(id) || \
> id ##_BACKEND_DSTATE()))
>
> We could change it to:
>
> #define trace_event_get_state(id) \
> ((id ##_ENABLED) && id ##_BACKEND_DSTATE())
>
> and modify the log, syslog, ftrace, etc backends to emit
> trace_event_get_state_dynamic_by_id(id) as their backend dstate.
Yeah that's exactly what I imagined.
>
> However, in the multi-backend case with ./configure
> --enable-trace-backends=log,syslog this expands to:
>
> ((id ##_ENABLED) && (trace_event_get_state_dynamic_by_id(id) || \
> trace_event_get_state_dynamic_by_id(id) || \
> false))
>
> I couldn't bring myself to have multiple calls to
> trace_event_get_state_dynamic_by_id() :).
I don't think that's worth worrying about.
trace_event_get_state_dynamic_by_id() is optimized to a simple inline
condition check. IOW, if you have 3 backends enable concurrently which
each use trace_event_get_state_dynamic_by_id(), this is no worse than
if you have 3 other backends enabled concurrently that each perform
some backend specific check.
Since trace_event_get_state_dynamic_by_id() is inline check, the
compiler might even optimize away the redundant check of the same
variable, or the processor might have optimized it too.
Enabling many backends at once is not particularly common anyway,
but in the absolute worst case, you could make tracetool intelligent
enough to notice the duplicate calls to trace_event_get_state_dynamic_by_id
and merge them into one in the generated code.
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 :|
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 <stefanha@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 :|
© 2016 - 2026 Red Hat, Inc.