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

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

[Lluís Vilanova]

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 <vilanova@ac.upc.edu>
---
 stubs/trace-control.c  |    5 +++++
 trace/control-target.c |    9 +++++++++
 trace/control.c        |   23 ++++++++++-------------
 trace/control.h        |    3 +++
 trace/event-internal.h |    1 +
 5 files changed, 28 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..f6e06cc 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,14 @@ 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);
+            /* check preconditions */
+            assert(trace_events_dstate[id] == 1);
+            /* disable early-init state ... */
+            trace_events_dstate[id] = 0;
+            trace_events_enabled_count--;
+            /* ... and properly re-enable */
             trace_event_set_state_dynamic(ev, true);
         }
     }
diff --git a/trace/control.h b/trace/control.h
index 0413b28..27a16fc 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -274,6 +274,9 @@ char *trace_opt_parse(const char *optarg);
  *
  * Re-synchronize initial event state with vCPUs (which can be created after
  * trace_init_events()).
+ *
+ * Precondition: event states won't be changed between trace_enable_events() and
+ * trace_init_vcpu_events() (e.g., through QMP).
  */
 void trace_init_vcpu_events(void);
 
diff --git a/trace/event-internal.h b/trace/event-internal.h
index 5d8fa97..074faf6 100644
--- a/trace/event-internal.h
+++ b/trace/event-internal.h
@@ -29,5 +29,6 @@ typedef struct TraceEvent {
     const bool sstate;
 } TraceEvent;
 
+void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state);
 
 #endif /* TRACE__EVENT_INTERNAL_H */

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

[Daniel P. Berrange]

On Thu, Aug 11, 2016 at 12:23:05PM +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 <vilanova@ac.upc.edu>
> ---
>  stubs/trace-control.c  |    5 +++++
>  trace/control-target.c |    9 +++++++++
>  trace/control.c        |   23 ++++++++++-------------
>  trace/control.h        |    3 +++
>  trace/event-internal.h |    1 +
>  5 files changed, 28 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];

This is doing arithmetic on a boolean value

> +    trace_events_dstate[id] = state;

This is assigning a boolean value to an int16_t

>  void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
>  {
>      CPUState *vcpu;

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

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

[Lluís Vilanova]

Daniel P Berrange writes:

> On Thu, Aug 11, 2016 at 12:23:05PM +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 <vilanova@ac.upc.edu>
>> ---
>> stubs/trace-control.c  |    5 +++++
>> trace/control-target.c |    9 +++++++++
>> trace/control.c        |   23 ++++++++++-------------
>> trace/control.h        |    3 +++
>> trace/event-internal.h |    1 +
>> 5 files changed, 28 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];

> This is doing arithmetic on a boolean value

>> +    trace_events_dstate[id] = state;

> This is assigning a boolean value to an int16_t

>> void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
>> {
>> CPUState *vcpu;


Mmmm, all these were c&p from existing code, just moved to a new function. I do
remember explicitly checking for the boolean values with if/else to do the
appropriate operations, but someone reviewing the code suggested using what
you're seeing to make it shorter and more readable.

Bottom line, the same reasons that led to this code style should hold here too.

PS: I don't know what the standard says about these implicit conversions.

Cheers,
  Lluis

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

[Daniel P. Berrange]

On Thu, Aug 11, 2016 at 01:03:10PM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
> 
> > On Thu, Aug 11, 2016 at 12:23:05PM +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 <vilanova@ac.upc.edu>
> >> ---
> >> stubs/trace-control.c  |    5 +++++
> >> trace/control-target.c |    9 +++++++++
> >> trace/control.c        |   23 ++++++++++-------------
> >> trace/control.h        |    3 +++
> >> trace/event-internal.h |    1 +
> >> 5 files changed, 28 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];
> 
> > This is doing arithmetic on a boolean value
> 
> >> +    trace_events_dstate[id] = state;
> 
> > This is assigning a boolean value to an int16_t
> 
> >> void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
> >> {
> >> CPUState *vcpu;
> 
> 
> Mmmm, all these were c&p from existing code, just moved to a new function. I do
> remember explicitly checking for the boolean values with if/else to do the
> appropriate operations, but someone reviewing the code suggested using what
> you're seeing to make it shorter and more readable.

I think this logic is considerably less understandable with this magic
trick, because it is not at all obvious that trace_events_dstate[id]
will always be 0 if state is 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 :|

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

[Lluís Vilanova]

Daniel P Berrange writes:

> On Thu, Aug 11, 2016 at 01:03:10PM +0200, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>> 
>> > On Thu, Aug 11, 2016 at 12:23:05PM +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 <vilanova@ac.upc.edu>
>> >> ---
>> >> stubs/trace-control.c  |    5 +++++
>> >> trace/control-target.c |    9 +++++++++
>> >> trace/control.c        |   23 ++++++++++-------------
>> >> trace/control.h        |    3 +++
>> >> trace/event-internal.h |    1 +
>> >> 5 files changed, 28 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];
>> 
>> > This is doing arithmetic on a boolean value
>> 
>> >> +    trace_events_dstate[id] = state;
>> 
>> > This is assigning a boolean value to an int16_t
>> 
>> >> void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
>> >> {
>> >> CPUState *vcpu;
>> 
>> 
>> Mmmm, all these were c&p from existing code, just moved to a new function. I do
>> remember explicitly checking for the boolean values with if/else to do the
>> appropriate operations, but someone reviewing the code suggested using what
>> you're seeing to make it shorter and more readable.

> I think this logic is considerably less understandable with this magic
> trick, because it is not at all obvious that trace_events_dstate[id]
> will always be 0 if state is True.

I agree. I can change it if noone else objects (Stefan?), but the rest of the
tracing code uses this shorter approach.

Cheers,
  Lluis