Re: [Qemu-devel] [PATCH v4 08/17] 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_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.

> Signed-off-by: Daniel P. Berrange <address@hidden>

Disregarding comment below:

Reviewed-by: Lluís Vilanova <address@hidden>

> ---
>  scripts/tracetool/backend/simple.py  |  4 ++--
>  scripts/tracetool/format/events_c.py | 16 +++++++++++-----
>  scripts/tracetool/format/events_h.py | 18 +++---------------
>  scripts/tracetool/format/h.py        |  3 +--
>  trace/control-internal.h             | 19 ++++++++++---------
>  trace/control-target.c               |  2 +-
>  trace/control.c                      |  2 +-
>  trace/control.h                      | 31 ++++++++-----------------------
>  trace/event-internal.h               |  6 ++++++
>  trace/simple.c                       |  8 ++++----
>  trace/simple.h                       |  2 +-
>  11 files changed, 48 insertions(+), 63 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 0dd4a33..325f4e0 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..5da1d4c 100644
> --- a/scripts/tracetool/format/events_h.py
> +++ b/scripts/tracetool/format/events_h.py
> @@ -29,27 +29,15 @@ 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 {')
> -
> +    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;')

Here's a more pythonic way to write it:

    numvcpu = len([e for e in events if "vcpu" in e.properties])


> +    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..8bfbdfb 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..21ce5f1 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,17 @@ 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


Thanks,
  Lluis