[Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization

[Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization

[Lluís Vilanova]

Removes the need for 'trace_events_dstate_init' and does a little cleanup in how
state values are modified (to avoid implicit conversions from bool).

Changes in v2
=============

* Fix late-init state value [Daniel P. Berrange].

Changes in v3
=============

* Avoid implicit conversions from bool to integers (second patch) [Daniel
  P. Berrange].

Changes in v4
=============

* Correctly implement idempotent state changes.
* Clarify when state changes are idempotent [Daniel P. Berrange].

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---

Lluís Vilanova (2):
      trace: Remove 'trace_events_dstate_init'
      trace: Avoid implicit bool->integer conversions


 stubs/trace-control.c  |   22 ++++++++++++++++++++--
 trace/control-target.c |   34 ++++++++++++++++++++++++++++++++--
 trace/control.c        |   23 ++++++++++-------------
 trace/control.h        |    3 +++
 trace/event-internal.h |    1 +
 5 files changed, 66 insertions(+), 17 deletions(-)


To: qemu-devel@nongnu.org
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: 'Daniel P. Berrange' <berrange@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH v4 1/2] 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 */

[Qemu-devel] [PATCH v4 2/2] trace: Avoid implicit bool->integer conversions

[Lluís Vilanova]

An explicit if/else is clearer than arithmetic assuming #true is 1,
while the compiler should be able to generate just as optimal code.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 stubs/trace-control.c  |   17 +++++++++++++++--
 trace/control-target.c |   31 ++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/stubs/trace-control.c b/stubs/trace-control.c
index 3740c38..2dfcd9f 100644
--- a/stubs/trace-control.c
+++ b/stubs/trace-control.c
@@ -19,10 +19,23 @@ void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
 void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
 {
     TraceEventID id;
+    bool state_pre;
     assert(trace_event_get_state_static(ev));
     id = trace_event_get_id(ev);
-    trace_events_enabled_count += state - trace_events_dstate[id];
-    trace_events_dstate[id] = state;
+    /*
+     * We ignore the "vcpu" property here, since there's no target code. Then
+     * dstate can only be 1 or 0.
+     */
+    state_pre = trace_events_dstate[id];
+    if (state_pre != state) {
+        if (state) {
+            trace_events_enabled_count++;
+            trace_events_dstate[id] = 1;
+        } else {
+            trace_events_enabled_count--;
+            trace_events_dstate[id] = 0;
+        }
+    }
 }
 
 void trace_event_set_vcpu_state_dynamic(CPUState *vcpu,
diff --git a/trace/control-target.c b/trace/control-target.c
index 4ee3733..72081e2 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -16,10 +16,22 @@
 void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
 {
     TraceEventID id = trace_event_get_id(ev);
+    bool state_pre;
     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;
+    /*
+     * We ignore the "vcpu" property here, since no vCPUs have been created
+     * yet. Then dstate can only be 1 or 0.
+     */
+    state_pre = trace_events_dstate[id];
+    if (state_pre != state) {
+        if (state) {
+            trace_events_enabled_count++;
+            trace_events_dstate[id] = 1;
+        } else {
+            trace_events_enabled_count--;
+            trace_events_dstate[id] = 0;
+        }
+    }
 }
 
 void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
@@ -31,9 +43,18 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
             trace_event_set_vcpu_state_dynamic(vcpu, ev, state);
         }
     } else {
+        /* Without the "vcpu" property, dstate can only be 1 or 0 */
         TraceEventID id = trace_event_get_id(ev);
-        trace_events_enabled_count += state - trace_events_dstate[id];
-        trace_events_dstate[id] = state;
+        bool state_pre = trace_events_dstate[id];
+        if (state_pre != state) {
+            if (state) {
+                trace_events_enabled_count++;
+                trace_events_dstate[id] = 1;
+            } else {
+                trace_events_enabled_count--;
+                trace_events_dstate[id] = 0;
+            }
+        }
     }
 }
 

Re: [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On Fri, Aug 12, 2016 at 05:33:35PM +0200, Lluís Vilanova wrote:
> Removes the need for 'trace_events_dstate_init' and does a little cleanup in how
> state values are modified (to avoid implicit conversions from bool).
> 
> Changes in v2
> =============
> 
> * Fix late-init state value [Daniel P. Berrange].
> 
> Changes in v3
> =============
> 
> * Avoid implicit conversions from bool to integers (second patch) [Daniel
>   P. Berrange].
> 
> Changes in v4
> =============
> 
> * Correctly implement idempotent state changes.
> * Clarify when state changes are idempotent [Daniel P. Berrange].
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> 
> Lluís Vilanova (2):
>       trace: Remove 'trace_events_dstate_init'
>       trace: Avoid implicit bool->integer conversions
> 
> 
>  stubs/trace-control.c  |   22 ++++++++++++++++++++--
>  trace/control-target.c |   34 ++++++++++++++++++++++++++++++++--
>  trace/control.c        |   23 ++++++++++-------------
>  trace/control.h        |    3 +++
>  trace/event-internal.h |    1 +
>  5 files changed, 66 insertions(+), 17 deletions(-)
> 
> 
> To: qemu-devel@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: 'Daniel P. Berrange' <berrange@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>

I will wait for Daniel Berrange to review before merging since he had
comments on previous revisions.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 1/2] trace: Remove 'trace_events_dstate_init'

[Daniel P. Berrange]

On Fri, Aug 12, 2016 at 05:33:41PM +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>

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


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 v4 2/2] trace: Avoid implicit bool->integer conversions

[Daniel P. Berrange]

On Fri, Aug 12, 2016 at 05:33:46PM +0200, Lluís Vilanova wrote:
> An explicit if/else is clearer than arithmetic assuming #true is 1,
> while the compiler should be able to generate just as optimal code.
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  stubs/trace-control.c  |   17 +++++++++++++++--
>  trace/control-target.c |   31 ++++++++++++++++++++++++++-----
>  2 files changed, 41 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

> 
> diff --git a/stubs/trace-control.c b/stubs/trace-control.c
> index 3740c38..2dfcd9f 100644
> --- a/stubs/trace-control.c
> +++ b/stubs/trace-control.c
> @@ -19,10 +19,23 @@ void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
>  void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
>  {
>      TraceEventID id;
> +    bool state_pre;
>      assert(trace_event_get_state_static(ev));
>      id = trace_event_get_id(ev);
> -    trace_events_enabled_count += state - trace_events_dstate[id];
> -    trace_events_dstate[id] = state;
> +    /*
> +     * We ignore the "vcpu" property here, since there's no target code. Then
> +     * dstate can only be 1 or 0.
> +     */
> +    state_pre = trace_events_dstate[id];
> +    if (state_pre != state) {
> +        if (state) {
> +            trace_events_enabled_count++;
> +            trace_events_dstate[id] = 1;
> +        } else {
> +            trace_events_enabled_count--;
> +            trace_events_dstate[id] = 0;
> +        }
> +    }
>  }
>  
>  void trace_event_set_vcpu_state_dynamic(CPUState *vcpu,
> diff --git a/trace/control-target.c b/trace/control-target.c
> index 4ee3733..72081e2 100644
> --- a/trace/control-target.c
> +++ b/trace/control-target.c
> @@ -16,10 +16,22 @@
>  void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
>  {
>      TraceEventID id = trace_event_get_id(ev);
> +    bool state_pre;
>      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;
> +    /*
> +     * We ignore the "vcpu" property here, since no vCPUs have been created
> +     * yet. Then dstate can only be 1 or 0.
> +     */
> +    state_pre = trace_events_dstate[id];
> +    if (state_pre != state) {
> +        if (state) {
> +            trace_events_enabled_count++;
> +            trace_events_dstate[id] = 1;
> +        } else {
> +            trace_events_enabled_count--;
> +            trace_events_dstate[id] = 0;
> +        }
> +    }
>  }
>  
>  void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
> @@ -31,9 +43,18 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
>              trace_event_set_vcpu_state_dynamic(vcpu, ev, state);
>          }
>      } else {
> +        /* Without the "vcpu" property, dstate can only be 1 or 0 */
>          TraceEventID id = trace_event_get_id(ev);
> -        trace_events_enabled_count += state - trace_events_dstate[id];
> -        trace_events_dstate[id] = state;
> +        bool state_pre = trace_events_dstate[id];
> +        if (state_pre != state) {
> +            if (state) {
> +                trace_events_enabled_count++;
> +                trace_events_dstate[id] = 1;
> +            } else {
> +                trace_events_enabled_count--;
> +                trace_events_dstate[id] = 0;
> +            }
> +        }
>      }
>  }
>  
> 

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 v4 0/2] trace: Simplify late initialization

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On Fri, Aug 12, 2016 at 05:33:35PM +0200, Lluís Vilanova wrote:
> Removes the need for 'trace_events_dstate_init' and does a little cleanup in how
> state values are modified (to avoid implicit conversions from bool).
> 
> Changes in v2
> =============
> 
> * Fix late-init state value [Daniel P. Berrange].
> 
> Changes in v3
> =============
> 
> * Avoid implicit conversions from bool to integers (second patch) [Daniel
>   P. Berrange].
> 
> Changes in v4
> =============
> 
> * Correctly implement idempotent state changes.
> * Clarify when state changes are idempotent [Daniel P. Berrange].
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> 
> Lluís Vilanova (2):
>       trace: Remove 'trace_events_dstate_init'
>       trace: Avoid implicit bool->integer conversions
> 
> 
>  stubs/trace-control.c  |   22 ++++++++++++++++++++--
>  trace/control-target.c |   34 ++++++++++++++++++++++++++++++++--
>  trace/control.c        |   23 ++++++++++-------------
>  trace/control.h        |    3 +++
>  trace/event-internal.h |    1 +
>  5 files changed, 66 insertions(+), 17 deletions(-)
> 
> 
> To: qemu-devel@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: 'Daniel P. Berrange' <berrange@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 

Thanks, applied to my tracing-next tree:
https://github.com/stefanha/qemu/commits/tracing-next

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On Fri, Aug 12, 2016 at 05:33:35PM +0200, Lluís Vilanova wrote:
> Removes the need for 'trace_events_dstate_init' and does a little cleanup in how
> state values are modified (to avoid implicit conversions from bool).
> 
> Changes in v2
> =============
> 
> * Fix late-init state value [Daniel P. Berrange].
> 
> Changes in v3
> =============
> 
> * Avoid implicit conversions from bool to integers (second patch) [Daniel
>   P. Berrange].
> 
> Changes in v4
> =============
> 
> * Correctly implement idempotent state changes.
> * Clarify when state changes are idempotent [Daniel P. Berrange].
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> 
> Lluís Vilanova (2):
>       trace: Remove 'trace_events_dstate_init'
>       trace: Avoid implicit bool->integer conversions
> 
> 
>  stubs/trace-control.c  |   22 ++++++++++++++++++++--
>  trace/control-target.c |   34 ++++++++++++++++++++++++++++++++--
>  trace/control.c        |   23 ++++++++++-------------
>  trace/control.h        |    3 +++
>  trace/event-internal.h |    1 +
>  5 files changed, 66 insertions(+), 17 deletions(-)
> 
> 
> To: qemu-devel@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: 'Daniel P. Berrange' <berrange@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>

The Travis CI system is reporting the following failure:

GTESTER check-qtest-aarch64
qemu-system-aarch64: ./trace/control-internal.h:77: trace_event_get_state_dynamic: Assertion `trace_event_get_state_static(ev)' failed.

https://travis-ci.org/stefanha/qemu/jobs/153235649

Please send a new revision that fixes the issue.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]