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 <berra...@redhat.com> Disregarding comment below: Reviewed-by: Lluís Vilanova <vilan...@ac.upc.edu> > --- > 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