On Mon, Sep 19, 2016 at 07:48:59PM +0200, Lluís Vilanova wrote: > 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 <[email protected]> > > --- > > 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?
No, see my response in the previous patch for why ID values are not the same as iterator event array indexes. > > @@ -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). No, this entire series is about explicitly *NOT* having to pre-calculate event ID values at time of generation, as that inherantly involves re-generating all code when ID values change in any single trace-events file and I want to avoid the assumptions that all vcpu events are in the same file that my earlier posting had. If the performance problem is genuinely a problem we can switch to having a 'int TRACE_EVENT_XXXXX_VCPU_ID' variable per event and then having 'TraceEvent' struct contain a 'int *vcpu_id', but I think we should only do that if there's a measurable performance problem demonstrated. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
