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

[Daniel P. Berrange]

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