[Qemu-devel] [PATCH v5 10/20] trace: remove the TraceEventID and TraceEventVCPUID enums

[Daniel P. Berrange]

The TraceEventID and TraceEventVCPUID enums constants are
no longer actually used for anything critical.

The TRACE_EVENT_COUNT limit is used to determine the size
of the TraceEvents array, and can be removed if we just
NULL terminate the array instead.

The TRACE_VCPU_EVENT_COUNT limit is used as a magic value
for marking non-vCPU events, and also for declaring the
size of the trace dstate mask in the CPUState struct.
The former usage can be replaced by a dedicated constant
TRACE_EVENT_VCPU_NONE, defined as (uint32_t)-1. For the
latter usage, we can simply define a constant for the
number of VCPUs, avoiding the need for the full enum.

The only other usages of the enum values can be replaced
by accesing the id/vcpu_id fields via the named TraceEvent
structs.

Reviewed-by: Lluís Vilanova <address@hidden>
Reviewed-by: Stefan Hajnoczi <address@hidden>
Signed-off-by: Daniel P. Berrange <address@hidden>
---
 scripts/tracetool/backend/simple.py  |  4 ++--
 scripts/tracetool/format/events_c.py | 16 +++++++++++-----
 scripts/tracetool/format/events_h.py | 19 ++-----------------
 scripts/tracetool/format/h.py        |  3 +--
 trace/control-internal.h             | 19 ++++++++++---------
 trace/control-target.c               |  2 +-
 trace/control.c                      |  2 +-
 trace/control.h                      | 32 +++++++++-----------------------
 trace/event-internal.h               |  6 ++++++
 trace/simple.c                       |  8 ++++----
 trace/simple.h                       |  2 +-
 11 files changed, 48 insertions(+), 65 deletions(-)

diff --git a/scripts/tracetool/backend/simple.py 
b/scripts/tracetool/backend/simple.py
index 1bccada..1114e35 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -80,11 +80,11 @@ def generate_c(event):
         '        return;',
         '    }',
         '',
-        '    if (trace_record_start(&rec, %(event_id)s, %(size_str)s)) {',
+        '    if (trace_record_start(&rec, %(event_obj)s.id, %(size_str)s)) {',
         '        return; /* Trace Buffer Full, Event Dropped ! */',
         '    }',
         cond=cond,
-        event_id=event_id,
+        event_obj=event.api(event.QEMU_EVENT),
         size_str=sizestr)
 
     if len(event.args) > 0:
diff --git a/scripts/tracetool/format/events_c.py 
b/scripts/tracetool/format/events_c.py
index 43c558c..aff7c9a 100644
--- a/scripts/tracetool/format/events_c.py
+++ b/scripts/tracetool/format/events_c.py
@@ -28,11 +28,16 @@ def generate(events, backend):
     for e in events:
         out('uint16_t %s;' % e.api(e.QEMU_DSTATE))
 
+    next_id = 0
+    next_vcpu_id = 0
     for e in events:
+        id = next_id
+        next_id += 1
         if "vcpu" in e.properties:
-            vcpu_id = "TRACE_VCPU_" + e.name.upper()
+            vcpu_id = next_vcpu_id
+            next_vcpu_id += 1
         else:
-            vcpu_id = "TRACE_VCPU_EVENT_COUNT"
+            vcpu_id = "TRACE_VCPU_EVENT_NONE"
         out('TraceEvent %(event)s = {',
             '    .id = %(id)s,',
             '    .vcpu_id = %(vcpu_id)s,',
@@ -41,16 +46,17 @@ def generate(events, backend):
             '    .dstate = &%(dstate)s ',
             '};',
             event = e.api(e.QEMU_EVENT),
-            id = "TRACE_" + e.name.upper(),
+            id = id,
             vcpu_id = vcpu_id,
             name = e.name,
             sstate = "TRACE_%s_ENABLED" % e.name.upper(),
             dstate = e.api(e.QEMU_DSTATE))
 
-    out('TraceEvent *trace_events[TRACE_EVENT_COUNT] = {')
+    out('TraceEvent *trace_events[] = {')
 
     for e in events:
         out('&%(event)s,', event = e.api(e.QEMU_EVENT))
 
-    out('};',
+    out('  NULL,',
+        '};',
         '')
diff --git a/scripts/tracetool/format/events_h.py 
b/scripts/tracetool/format/events_h.py
index 80a66c5..ca6d730 100644
--- a/scripts/tracetool/format/events_h.py
+++ b/scripts/tracetool/format/events_h.py
@@ -29,27 +29,12 @@ def generate(events, backend):
         out('extern TraceEvent %(event)s;',
             event = e.api(e.QEMU_EVENT))
 
-    # event identifiers
-    out('typedef enum {')
-
-    for e in events:
-        out('    TRACE_%s,' % e.name.upper())
-
-    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 {')
-
-    for e in events:
-        if "vcpu" in e.properties:
-            out('    TRACE_VCPU_%s,' % e.name.upper())
+    numvcpu = len([e for e in events if "vcpu" in e.properties])
 
-    out('    TRACE_VCPU_EVENT_COUNT',
-        '} TraceEventVCPUID;')
+    out("#define TRACE_VCPU_EVENT_COUNT %d" % numvcpu)
 
     # static state
     for e in events:
diff --git a/scripts/tracetool/format/h.py b/scripts/tracetool/format/h.py
index 3763e9a..64a6680 100644
--- a/scripts/tracetool/format/h.py
+++ b/scripts/tracetool/format/h.py
@@ -32,8 +32,7 @@ def generate(events, backend):
         if "vcpu" in e.properties:
             trace_cpu = next(iter(e.args))[1]
             cond = "trace_event_get_vcpu_state(%(cpu)s,"\
-                   " TRACE_%(id)s,"\
-                   " TRACE_VCPU_%(id)s)"\
+                   " TRACE_%(id)s)"\
                    % dict(
                        cpu=trace_cpu,
                        id=e.name.upper())
diff --git a/trace/control-internal.h b/trace/control-internal.h
index 808f85d..9abbc96 100644
--- a/trace/control-internal.h
+++ b/trace/control-internal.h
@@ -25,20 +25,20 @@ static inline bool trace_event_is_pattern(const char *str)
     return strchr(str, '*') != NULL;
 }
 
-static inline TraceEventID trace_event_get_id(TraceEvent *ev)
+static inline uint32_t trace_event_get_id(TraceEvent *ev)
 {
     assert(ev != NULL);
     return ev->id;
 }
 
-static inline TraceEventVCPUID trace_event_get_vcpu_id(TraceEvent *ev)
+static inline uint32_t trace_event_get_vcpu_id(TraceEvent *ev)
 {
     return ev->vcpu_id;
 }
 
 static inline bool trace_event_is_vcpu(TraceEvent *ev)
 {
-    return ev->vcpu_id != TRACE_VCPU_EVENT_COUNT;
+    return ev->vcpu_id != TRACE_VCPU_EVENT_NONE;
 }
 
 static inline const char * trace_event_get_name(TraceEvent *ev)
@@ -62,12 +62,13 @@ static inline bool trace_event_get_state_dynamic(TraceEvent 
*ev)
     return unlikely(trace_events_enabled_count) && *ev->dstate;
 }
 
-static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState 
*vcpu,
-                                                                 
TraceEventVCPUID id)
+static inline bool
+trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState *vcpu,
+                                              uint32_t vcpu_id)
 {
     /* it's on fast path, avoid consistency checks (asserts) */
     if (unlikely(trace_events_enabled_count)) {
-        return test_bit(id, vcpu->trace_dstate);
+        return test_bit(vcpu_id, vcpu->trace_dstate);
     } else {
         return false;
     }
@@ -76,10 +77,10 @@ static inline bool 
trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState *vcpu,
 static inline bool trace_event_get_vcpu_state_dynamic(CPUState *vcpu,
                                                       TraceEvent *ev)
 {
-    TraceEventVCPUID id;
+    uint32_t vcpu_id;
     assert(trace_event_is_vcpu(ev));
-    id = trace_event_get_vcpu_id(ev);
-    return trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, id);
+    vcpu_id = trace_event_get_vcpu_id(ev);
+    return trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, vcpu_id);
 }
 
 #endif /* TRACE__CONTROL_INTERNAL_H */
diff --git a/trace/control-target.c b/trace/control-target.c
index c69dda9..ff1bf43 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -59,7 +59,7 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
 void trace_event_set_vcpu_state_dynamic(CPUState *vcpu,
                                         TraceEvent *ev, bool state)
 {
-    TraceEventVCPUID vcpu_id;
+    uint32_t vcpu_id;
     bool state_pre;
     assert(trace_event_get_state_static(ev));
     assert(trace_event_is_vcpu(ev));
diff --git a/trace/control.c b/trace/control.c
index 9107919..64aaede 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -105,7 +105,7 @@ void trace_event_iter_init(TraceEventIter *iter, const char 
*pattern)
 
 TraceEvent *trace_event_iter_next(TraceEventIter *iter)
 {
-    while (iter->event < TRACE_EVENT_COUNT) {
+    while (trace_events[iter->event] != NULL) {
         TraceEvent *ev = trace_events[iter->event];
         iter->event++;
         if (!iter->pattern ||
diff --git a/trace/control.h b/trace/control.h
index e80c220..b33e1a3 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -18,17 +18,6 @@ typedef struct TraceEventIter {
     const char *pattern;
 } TraceEventIter;
 
-/**
- * TraceEventID:
- *
- * Unique tracing event identifier.
- *
- * These are named as 'TRACE_${EVENT_NAME}'.
- *
- * See also: "trace/generated-events.h"
- */
-enum TraceEventID;
-
 
 /**
  * trace_event_iter_init:
@@ -76,17 +65,17 @@ static bool trace_event_is_pattern(const char *str);
  *
  * Get the identifier of an event.
  */
-static TraceEventID trace_event_get_id(TraceEvent *ev);
+static uint32_t trace_event_get_id(TraceEvent *ev);
 
 /**
  * trace_event_get_vcpu_id:
  *
  * Get the per-vCPU identifier of an event.
  *
- * Special value #TRACE_VCPU_EVENT_COUNT means the event is not vCPU-specific
+ * Special value #TRACE_VCPU_EVENT_NONE means the event is not vCPU-specific
  * (does not have the "vcpu" property).
  */
-static TraceEventVCPUID trace_event_get_vcpu_id(TraceEvent *ev);
+static uint32_t trace_event_get_vcpu_id(TraceEvent *ev);
 
 /**
  * trace_event_is_vcpu:
@@ -104,14 +93,12 @@ static const char * trace_event_get_name(TraceEvent *ev);
 
 /**
  * trace_event_get_state:
- * @id: Event identifier.
+ * @id: Event identifier name.
  *
  * Get the tracing state of an event (both static and dynamic).
  *
  * If the event has the disabled property, the check will have no performance
  * impact.
- *
- * As a down side, you must always use an immediate #TraceEventID value.
  */
 #define trace_event_get_state(id)                       \
     ((id ##_ENABLED) && trace_event_get_state_dynamic_by_id(id))
@@ -119,19 +106,18 @@ static const char * trace_event_get_name(TraceEvent *ev);
 /**
  * trace_event_get_vcpu_state:
  * @vcpu: Target vCPU.
- * @id: Event identifier (TraceEventID).
- * @vcpu_id: Per-vCPU event identifier (TraceEventVCPUID).
+ * @id: Event identifier name.
  *
  * Get the tracing state of an event (both static and dynamic) for the given
  * vCPU.
  *
  * If the event has the disabled property, the check will have no performance
  * impact.
- *
- * As a down side, you must always use an immediate #TraceEventID value.
  */
-#define trace_event_get_vcpu_state(vcpu, id, vcpu_id)                   \
-    ((id ##_ENABLED) && trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, 
vcpu_id))
+#define trace_event_get_vcpu_state(vcpu, id)                            \
+    ((id ##_ENABLED) &&                                                 \
+     trace_event_get_vcpu_state_dynamic_by_vcpu_id(                     \
+         vcpu, _ ## id ## _EVENT.vcpu_id))
 
 /**
  * trace_event_get_state_static:
diff --git a/trace/event-internal.h b/trace/event-internal.h
index 58f0551..f63500b 100644
--- a/trace/event-internal.h
+++ b/trace/event-internal.h
@@ -10,6 +10,12 @@
 #ifndef TRACE__EVENT_INTERNAL_H
 #define TRACE__EVENT_INTERNAL_H
 
+/*
+ * Special value for TraceEvent.vcpu_id field to indicate
+ * that the event is not VCPU specific
+ */
+#define TRACE_VCPU_EVENT_NONE ((uint32_t)-1)
+
 /**
  * TraceEvent:
  * @id: Unique event identifier.
diff --git a/trace/simple.c b/trace/simple.c
index 2f09daf..2ec32e1 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -17,8 +17,8 @@
 #include "trace/control.h"
 #include "trace/simple.h"
 
-/** Trace file header event ID */
-#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with 
TraceEventIDs */
+/** Trace file header event ID, picked to avoid conflict with real event IDs */
+#define HEADER_EVENT_ID (~(uint64_t)0)
 
 /** Trace file magic number */
 #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL
@@ -58,7 +58,7 @@ static char *trace_file_name;
 
 /* * Trace buffer entry */
 typedef struct {
-    uint64_t event; /*   TraceEventID */
+    uint64_t event; /* event ID value */
     uint64_t timestamp_ns;
     uint32_t length;   /*    in bytes */
     uint32_t pid;
@@ -202,7 +202,7 @@ void trace_record_write_str(TraceBufferRecord *rec, const 
char *s, uint32_t slen
     rec->rec_off = write_to_buffer(rec->rec_off, (void*)s, slen);
 }
 
-int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t 
datasize)
+int trace_record_start(TraceBufferRecord *rec, uint32_t event, size_t datasize)
 {
     unsigned int idx, rec_off, old_idx, new_idx;
     uint32_t rec_len = sizeof(TraceRecord) + datasize;
diff --git a/trace/simple.h b/trace/simple.h
index 1e7de45..17ce472 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -33,7 +33,7 @@ typedef struct {
  *
  * @arglen  number of bytes required for arguments
  */
-int trace_record_start(TraceBufferRecord *rec, TraceEventID id, size_t arglen);
+int trace_record_start(TraceBufferRecord *rec, uint32_t id, size_t arglen);
 
 /**
  * Append a 64-bit argument to a trace record
-- 
2.7.4