Re: [Qemu-devel] [PATCH v3 08/18] trace: remove the TraceEventID and TraceEventVCPUID enums

[Lluís Vilanova]

Daniel P Berrange writes:

> 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_EVENT_VCPU_COUNT limit is used as a magic value

It's TRACE_VCPU_EVENT_COUNT.


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

> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  scripts/tracetool/backend/simple.py  |  2 +-
>  scripts/tracetool/format/events_c.py | 16 +++++++++++-----
>  scripts/tracetool/format/events_h.py | 18 +++---------------
>  scripts/tracetool/format/h.py        |  3 +--
>  trace/control-internal.h             | 16 ++++++++--------
>  trace/control-target.c               |  2 +-
>  trace/control.c                      |  2 +-
>  trace/control.h                      | 30 +++++++-----------------------
>  trace/event-internal.h               |  6 ++++++
>  trace/simple.c                       |  6 +++---
>  trace/simple.h                       |  2 +-
>  11 files changed, 43 insertions(+), 60 deletions(-)

> diff --git a/scripts/tracetool/backend/simple.py 
> b/scripts/tracetool/backend/simple.py
> index 1bccada..971bd97 100644
> --- a/scripts/tracetool/backend/simple.py
> +++ b/scripts/tracetool/backend/simple.py
> @@ -80,7 +80,7 @@ def generate_c(event):
>          '        return;',
>          '    }',
>          '',
> -        '    if (trace_record_start(&rec, %(event_id)s, %(size_str)s)) {',
> +        '    if (trace_record_start(&rec, %(event_id)s_EV.id, %(size_str)s)) 
> {',

You should use Event.api() here too.


>          '        return; /* Trace Buffer Full, Event Dropped ! */',
>          '    }',
>          cond=cond,
> diff --git a/scripts/tracetool/format/events_c.py 
> b/scripts/tracetool/format/events_c.py
> index 5f67a7b..07665df 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,17 +46,18 @@ def generate(events, backend):
>              '  .dstate = &%(dstate)s ',
>              '};',
>              event = "TRACE_" + e.name.upper() + "_EV",
> -            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 = "TRACE_" + e.name.upper() + "_EV")
 
> -    out('};',
> +    out('  NULL,',
> +        '};',
>          '')
> diff --git a/scripts/tracetool/format/events_h.py 
> b/scripts/tracetool/format/events_h.py
> index 1478e4c..35cbc91 100644
> --- a/scripts/tracetool/format/events_h.py
> +++ b/scripts/tracetool/format/events_h.py
> @@ -28,27 +28,15 @@ def generate(events, backend):
>      for e in events:
>          out('extern TraceEvent TRACE_%s_EV;' % e.name.upper())
 
> -    # 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 {')
> -
> +    numvcpu = 0
>      for e in events:
>          if "vcpu" in e.properties:
> -            out('    TRACE_VCPU_%s,' % e.name.upper())
> +            numvcpu += 1
 
> -    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 52b6b72..c5bb53d 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)

Return should be size_t here too if the iterator patch uses size_t, right?

Same applies to all replacements of TraceEventID and TraceEventVCPUID.


>  {
>      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)
> @@ -63,11 +63,11 @@ static inline bool 
> trace_event_get_state_dynamic(TraceEvent *ev)
>  }
 
>  static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState 
> *vcpu,
> -                                                                 
> TraceEventVCPUID id)
> +                                                                 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 +76,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 71ad6ba..60235c5 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..c7c0a88 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,16 @@ 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 ## _EV.vcpu_id))

You should directly use the vCPU id value to keep performance at the same
level.

For that, I'd keep the TraceEventVCPUID enum, or some define for them. Note that
changes would only affect files including the tracing headers for events with
the vcpu property, which will typically be limited to a single header (with the
"guest_" events for now).

You can pre-calculate the .id and .vcpu_id values when reading all events (e.g.,
calculated in tracetool._read_events() and stored in the Event class).


>  /**
>   * trace_event_get_state_static:
> diff --git a/trace/event-internal.h b/trace/event-internal.h
> index 36906e2..0276804 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..716b66d 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -18,7 +18,7 @@
>  #include "trace/simple.h"
 
>  /** Trace file header event ID */
> -#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with 
> TraceEventIDs */
> +#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with trace 
> event IDs */
 
>  /** 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