[Qemu-devel] [PATCH v5 06/20] trace: remove global 'uint16 dstate[]' array

["Daniel P. Berrange" <berrange@redhat.com>]

Instead of having a global dstate array, declare a single
'uint16 TRACE_${EVENT_NAME}_DSTATE' variable for each
trace event. Record a pointer to this variable in the
TraceEvent struct too.

By turning trace_event_get_state_dynamic_by_id into a
macro, this still hits the fast path, and cache affinity
is ensured by declaring all the uint16 vars adjacent to
each other.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 scripts/tracetool/__init__.py        |  3 ++-
 scripts/tracetool/format/events_c.py |  9 +++++++--
 scripts/tracetool/format/events_h.py |  3 +++
 stubs/trace-control.c                |  9 ++++-----
 trace/control-internal.h             | 14 ++++----------
 trace/control-target.c               | 20 ++++++++------------
 trace/control.c                      | 11 ++---------
 trace/event-internal.h               |  7 +++++++
 8 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index be24039..a11f36b 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -265,11 +265,12 @@ class Event(object):
 
     QEMU_TRACE               = "trace_%(name)s"
     QEMU_TRACE_TCG           = QEMU_TRACE + "_tcg"
+    QEMU_DSTATE              = "_TRACE_%(NAME)s_DSTATE"
 
     def api(self, fmt=None):
         if fmt is None:
             fmt = Event.QEMU_TRACE
-        return fmt % {"name": self.name}
+        return fmt % {"name": self.name, "NAME": self.name.upper()}
 
     def transform(self, *trans):
         """Return a new Event with transformed Arguments."""
diff --git a/scripts/tracetool/format/events_c.py b/scripts/tracetool/format/events_c.py
index 4012063..ef873fa 100644
--- a/scripts/tracetool/format/events_c.py
+++ b/scripts/tracetool/format/events_c.py
@@ -25,6 +25,9 @@ def generate(events, backend):
         '#include "trace/control.h"',
         '')
 
+    for e in events:
+        out('uint16_t %s;' % e.api(e.QEMU_DSTATE))
+
     out('TraceEvent trace_events[TRACE_EVENT_COUNT] = {')
 
     for e in events:
@@ -34,11 +37,13 @@ def generate(events, backend):
             vcpu_id = "TRACE_VCPU_EVENT_COUNT"
         out('    { .id = %(id)s, .vcpu_id = %(vcpu_id)s,'
             ' .name = \"%(name)s\",'
-            ' .sstate = %(sstate)s },',
+            ' .sstate = %(sstate)s,',
+            ' .dstate = &%(dstate)s, }, ',
             id = "TRACE_" + e.name.upper(),
             vcpu_id = vcpu_id,
             name = e.name,
-            sstate = "TRACE_%s_ENABLED" % e.name.upper())
+            sstate = "TRACE_%s_ENABLED" % e.name.upper(),
+            dstate = e.api(e.QEMU_DSTATE))
 
     out('};',
         '')
diff --git a/scripts/tracetool/format/events_h.py b/scripts/tracetool/format/events_h.py
index a9da60b..03417de 100644
--- a/scripts/tracetool/format/events_h.py
+++ b/scripts/tracetool/format/events_h.py
@@ -32,6 +32,9 @@ def generate(events, backend):
     out('    TRACE_EVENT_COUNT',
         '} TraceEventID;')
 
+    for e in events:
+        out('extern uint16_t %s;' % e.api(e.QEMU_DSTATE))
+
     # per-vCPU event identifiers
     out('typedef enum {')
 
diff --git a/stubs/trace-control.c b/stubs/trace-control.c
index 2dfcd9f..dd68f25 100644
--- a/stubs/trace-control.c
+++ b/stubs/trace-control.c
@@ -18,22 +18,21 @@ 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);
+
     /*
      * 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];
+    state_pre = *(ev->dstate);
     if (state_pre != state) {
         if (state) {
             trace_events_enabled_count++;
-            trace_events_dstate[id] = 1;
+            *(ev->dstate) = 1;
         } else {
             trace_events_enabled_count--;
-            trace_events_dstate[id] = 0;
+            *(ev->dstate) = 0;
         }
     }
 }
diff --git a/trace/control-internal.h b/trace/control-internal.h
index 7f31e39..a6d8d2e 100644
--- a/trace/control-internal.h
+++ b/trace/control-internal.h
@@ -16,7 +16,6 @@
 
 
 extern TraceEvent trace_events[];
-extern uint16_t trace_events_dstate[];
 extern int trace_events_enabled_count;
 
 
@@ -54,18 +53,13 @@ static inline bool trace_event_get_state_static(TraceEvent *ev)
     return ev->sstate;
 }
 
-static inline bool trace_event_get_state_dynamic_by_id(TraceEventID id)
-{
-    /* it's on fast path, avoid consistency checks (asserts) */
-    return unlikely(trace_events_enabled_count) && trace_events_dstate[id];
-}
+/* it's on fast path, avoid consistency checks (asserts) */
+#define trace_event_get_state_dynamic_by_id(id) \
+    (unlikely(trace_events_enabled_count) && _ ## id ## _DSTATE)
 
 static inline bool trace_event_get_state_dynamic(TraceEvent *ev)
 {
-    TraceEventID id;
-    assert(trace_event_get_state_static(ev));
-    id = trace_event_get_id(ev);
-    return trace_event_get_state_dynamic_by_id(id);
+    return unlikely(trace_events_enabled_count) && *ev->dstate;
 }
 
 static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState *vcpu,
diff --git a/trace/control-target.c b/trace/control-target.c
index 72081e2..c69dda9 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -15,21 +15,20 @@
 
 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));
     /*
      * 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];
+    state_pre = *ev->dstate;
     if (state_pre != state) {
         if (state) {
             trace_events_enabled_count++;
-            trace_events_dstate[id] = 1;
+            *ev->dstate = 1;
         } else {
             trace_events_enabled_count--;
-            trace_events_dstate[id] = 0;
+            *ev->dstate = 0;
         }
     }
 }
@@ -44,15 +43,14 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
         }
     } else {
         /* Without the "vcpu" property, dstate can only be 1 or 0 */
-        TraceEventID id = trace_event_get_id(ev);
-        bool state_pre = trace_events_dstate[id];
+        bool state_pre = *ev->dstate;
         if (state_pre != state) {
             if (state) {
                 trace_events_enabled_count++;
-                trace_events_dstate[id] = 1;
+                *ev->dstate = 1;
             } else {
                 trace_events_enabled_count--;
-                trace_events_dstate[id] = 0;
+                *ev->dstate = 0;
             }
         }
     }
@@ -61,23 +59,21 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
 void trace_event_set_vcpu_state_dynamic(CPUState *vcpu,
                                         TraceEvent *ev, bool state)
 {
-    TraceEventID id;
     TraceEventVCPUID vcpu_id;
     bool state_pre;
     assert(trace_event_get_state_static(ev));
     assert(trace_event_is_vcpu(ev));
-    id = trace_event_get_id(ev);
     vcpu_id = trace_event_get_vcpu_id(ev);
     state_pre = test_bit(vcpu_id, vcpu->trace_dstate);
     if (state_pre != state) {
         if (state) {
             trace_events_enabled_count++;
             set_bit(vcpu_id, vcpu->trace_dstate);
-            trace_events_dstate[id]++;
+            (*ev->dstate)++;
         } else {
             trace_events_enabled_count--;
             clear_bit(vcpu_id, vcpu->trace_dstate);
-            trace_events_dstate[id]--;
+            (*ev->dstate)--;
         }
     }
 }
diff --git a/trace/control.c b/trace/control.c
index 250778a..a103560 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -28,12 +28,6 @@
 #include "monitor/monitor.h"
 
 int trace_events_enabled_count;
-/*
- * Interpretation depends on wether the event has the 'vcpu' property:
- * - false: Boolean value indicating whether the event is active.
- * - true : Integral counting the number of vCPUs that have this event enabled.
- */
-uint16_t trace_events_dstate[TRACE_EVENT_COUNT];
 
 QemuOptsList qemu_trace_opts = {
     .name = "trace",
@@ -285,11 +279,10 @@ void trace_init_vcpu_events(void)
         if (trace_event_is_vcpu(ev) &&
             trace_event_get_state_static(ev) &&
             trace_event_get_state_dynamic(ev)) {
-            TraceEventID id = trace_event_get_id(ev);
             /* check preconditions */
-            assert(trace_events_dstate[id] == 1);
+            assert(*ev->dstate == 1);
             /* disable early-init state ... */
-            trace_events_dstate[id] = 0;
+            *ev->dstate = 0;
             trace_events_enabled_count--;
             /* ... and properly re-enable */
             trace_event_set_state_dynamic(ev, true);
diff --git a/trace/event-internal.h b/trace/event-internal.h
index 074faf6..4a98d09 100644
--- a/trace/event-internal.h
+++ b/trace/event-internal.h
@@ -19,6 +19,12 @@
  * @vcpu_id: Unique per-vCPU event identifier.
  * @name: Event name.
  * @sstate: Static tracing state.
+ * @dstate: Dynamic tracing state
+ *
+ * Interpretation of @dstate depends on whether the event has the 'vcpu'
+ *  property:
+ * - false: Boolean value indicating whether the event is active.
+ * - true : Integral counting the number of vCPUs that have this event enabled.
  *
  * Opaque generic description of a tracing event.
  */
@@ -27,6 +33,7 @@ typedef struct TraceEvent {
     TraceEventVCPUID vcpu_id;
     const char * name;
     const bool sstate;
+    uint16_t *dstate;
 } TraceEvent;
 
 void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state);
-- 
2.7.4