Re: [Qemu-devel] [PATCH] trace: Remove 'trace_events_dstate_init'

[Daniel P. Berrange]

On Wed, Aug 10, 2016 at 06:57:34PM +0200, Lluís Vilanova wrote:
> Removes the event state array used for early initialization. Since only
> events with the "vcpu" property need a late initialization fixup,
> threats their initialization specially.
> 
> Assumes that the user won't touch the state of "vcpu" events between
> early and late initialization (e.g., through QMP).
> 
> Signed-off-by: Lluís Vilanova <address@hidden>
> ---
>  stubs/trace-control.c  |    5 +++++
>  trace/control-target.c |    9 +++++++++
>  trace/control.c        |   21 ++++++++-------------
>  trace/control.h        |    3 +++
>  trace/event-internal.h |    1 +
>  5 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/stubs/trace-control.c b/stubs/trace-control.c
> index fe59836..3740c38 100644
> --- a/stubs/trace-control.c
> +++ b/stubs/trace-control.c
> @@ -11,6 +11,11 @@
>  #include "trace/control.h"
>  
>  
> +void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
> +{
> +    trace_event_set_state_dynamic(ev, state);
> +}
> +
>  void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
>  {
>      TraceEventID id;
> diff --git a/trace/control-target.c b/trace/control-target.c
> index 74c029a..4ee3733 100644
> --- a/trace/control-target.c
> +++ b/trace/control-target.c
> @@ -13,6 +13,15 @@
>  #include "translate-all.h"
>  
>  
> +void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
> +{
> +    TraceEventID id = trace_event_get_id(ev);
> +    assert(trace_event_get_state_static(ev));
> +    /* Ignore "vcpu" property, since no vCPUs have been created yet */
> +    trace_events_enabled_count += state - trace_events_dstate[id];
> +    trace_events_dstate[id] = state;
> +}
> +
>  void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
>  {
>      CPUState *vcpu;
> diff --git a/trace/control.c b/trace/control.c
> index d173c09..027adc7 100644
> --- a/trace/control.c
> +++ b/trace/control.c
> @@ -31,8 +31,6 @@ int trace_events_enabled_count;
>   * - true : Integral counting the number of vCPUs that have this event 
> enabled.
>   */
>  uint16_t trace_events_dstate[TRACE_EVENT_COUNT];
> -/* Marks events for late vCPU state init */
> -static bool trace_events_dstate_init[TRACE_EVENT_COUNT];
>  
>  QemuOptsList qemu_trace_opts = {
>      .name = "trace",
> @@ -142,10 +140,7 @@ static void do_trace_enable_events(const char *line_buf)
>          TraceEvent *ev = NULL;
>          while ((ev = trace_event_pattern(line_ptr, ev)) != NULL) {
>              if (trace_event_get_state_static(ev)) {
> -                /* start tracing */
> -                trace_event_set_state_dynamic(ev, enable);
> -                /* mark for late vCPU init */
> -                trace_events_dstate_init[ev->id] = true;
> +                trace_event_set_state_dynamic_init(ev, enable);
>              }
>          }
>      } else {
> @@ -157,10 +152,7 @@ static void do_trace_enable_events(const char *line_buf)
>              error_report("WARNING: trace event '%s' is not traceable",
>                           line_ptr);
>          } else {
> -            /* start tracing */
> -            trace_event_set_state_dynamic(ev, enable);
> -            /* mark for late vCPU init */
> -            trace_events_dstate_init[ev->id] = true;
> +            trace_event_set_state_dynamic_init(ev, enable);
>          }
>      }
>  }
> @@ -275,9 +267,12 @@ void trace_init_vcpu_events(void)
>  {
>      TraceEvent *ev = NULL;
>      while ((ev = trace_event_pattern("*", ev)) != NULL) {
> -        if (trace_event_is_vcpu(ev) &&
> -            trace_event_get_state_static(ev) &&
> -            trace_events_dstate_init[ev->id]) {
> +        if (trace_event_is_vcpu(ev) && trace_event_get_state_dynamic(ev)) {
> +            TraceEventID id = trace_event_get_id(ev);
> +            /* disable early-init state ... */
> +            trace_events_dstate[id] = false;

This variable is uint16_t, so assigning false to it is bogus.

> +            trace_events_enabled_count--;
> +            /* ... and properly re-enable */
>              trace_event_set_state_dynamic(ev, true);
>          }
>      }

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|