| Message ID | 1474296549-29171-5-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/19/2016 09:48 AM, Daniel P. Berrange wrote: > Instead of having a global dstate array, declare a single > 'uint16 TRACE_${EVENT_NAME}_DSTATE' variable for each > trace event. Record a pointer to this variable in the > TraceEvent struct too. > > By turning trace_event_get_state_dynamic_by_id into a > macro, this still hits the fast path, and cache affinity > is ensured by declaring all the uint16 vars adjacent to > each other. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > +++ b/scripts/tracetool/__init__.py > @@ -265,11 +265,12 @@ class Event(object): > > QEMU_TRACE = "trace_%(name)s" > QEMU_TRACE_TCG = QEMU_TRACE + "_tcg" > + QEMU_DSTATE = "___TRACE_%(NAME)s_DSTATE" Inconsistent indentation. > +++ b/trace/control.c > @@ -28,12 +28,6 @@ > #include "monitor/monitor.h" > > int trace_events_enabled_count; > -/* > - * Interpretation depends on wether the event has the 'vcpu' property: Nice - we're nuking a typo while at it... > +++ b/trace/event-internal.h > @@ -19,6 +19,11 @@ > * @vcpu_id: Unique per-vCPU event identifier. > * @name: Event name. > * @sstate: Static tracing state. > + * @dstate: Dynamic tracing state > + * > + * Interpretation of @dstate depends on wether the event has the 'vcpu' property: ...oh, we just moved it. s/wether/whether/ while touching this.
Daniel P Berrange writes: > Instead of having a global dstate array, declare a single > 'uint16 TRACE_${EVENT_NAME}_DSTATE' variable for each > trace event. Record a pointer to this variable in the > TraceEvent struct too. > By turning trace_event_get_state_dynamic_by_id into a > macro, this still hits the fast path, and cache affinity > is ensured by declaring all the uint16 vars adjacent to > each other. > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Besides Eric's comments: Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu> > --- > scripts/tracetool/__init__.py | 3 ++- > scripts/tracetool/format/events_c.py | 9 +++++++-- > scripts/tracetool/format/events_h.py | 3 +++ > stubs/trace-control.c | 9 ++++----- > trace/control-internal.h | 14 ++++---------- > trace/control-target.c | 20 ++++++++------------ > trace/control.c | 11 ++--------- > trace/event-internal.h | 6 ++++++ > 8 files changed, 36 insertions(+), 39 deletions(-) > diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py > index be24039..5191df9 100644 > --- a/scripts/tracetool/__init__.py > +++ b/scripts/tracetool/__init__.py > @@ -265,11 +265,12 @@ class Event(object): > QEMU_TRACE = "trace_%(name)s" > QEMU_TRACE_TCG = QEMU_TRACE + "_tcg" > + QEMU_DSTATE = "___TRACE_%(NAME)s_DSTATE" > def api(self, fmt=None): > if fmt is None: > fmt = Event.QEMU_TRACE > - return fmt % {"name": self.name} > + return fmt % {"name": self.name, "NAME": self.name.upper()} > def transform(self, *trans): > """Return a new Event with transformed Arguments.""" > diff --git a/scripts/tracetool/format/events_c.py b/scripts/tracetool/format/events_c.py > index 4012063..ef873fa 100644 > --- a/scripts/tracetool/format/events_c.py > +++ b/scripts/tracetool/format/events_c.py > @@ -25,6 +25,9 @@ def generate(events, backend): > '#include "trace/control.h"', > '') > + for e in events: > + out('uint16_t %s;' % e.api(e.QEMU_DSTATE)) > + > out('TraceEvent trace_events[TRACE_EVENT_COUNT] = {') > for e in events: > @@ -34,11 +37,13 @@ def generate(events, backend): > vcpu_id = "TRACE_VCPU_EVENT_COUNT" > out(' { .id = %(id)s, .vcpu_id = %(vcpu_id)s,' > ' .name = \"%(name)s\",' > - ' .sstate = %(sstate)s },', > + ' .sstate = %(sstate)s,', > + ' .dstate = &%(dstate)s, }, ', > id = "TRACE_" + e.name.upper(), > vcpu_id = vcpu_id, > name = e.name, > - sstate = "TRACE_%s_ENABLED" % e.name.upper()) > + sstate = "TRACE_%s_ENABLED" % e.name.upper(), > + dstate = e.api(e.QEMU_DSTATE)) > out('};', > '') > diff --git a/scripts/tracetool/format/events_h.py b/scripts/tracetool/format/events_h.py > index a9da60b..03417de 100644 > --- a/scripts/tracetool/format/events_h.py > +++ b/scripts/tracetool/format/events_h.py > @@ -32,6 +32,9 @@ def generate(events, backend): > 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 {') > diff --git a/stubs/trace-control.c b/stubs/trace-control.c > index 2dfcd9f..dd68f25 100644 > --- a/stubs/trace-control.c > +++ b/stubs/trace-control.c > @@ -18,22 +18,21 @@ void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state) > void trace_event_set_state_dynamic(TraceEvent *ev, bool state) > { > - TraceEventID id; > bool state_pre; > assert(trace_event_get_state_static(ev)); > - id = trace_event_get_id(ev); > + > /* > * We ignore the "vcpu" property here, since there's no target code. Then > * dstate can only be 1 or 0. > */ > - state_pre = trace_events_dstate[id]; > + state_pre = *(ev->dstate); > if (state_pre != state) { > if (state) { > trace_events_enabled_count++; > - trace_events_dstate[id] = 1; > + *(ev->dstate) = 1; > } else { > trace_events_enabled_count--; > - trace_events_dstate[id] = 0; > + *(ev->dstate) = 0; > } > } > } > diff --git a/trace/control-internal.h b/trace/control-internal.h > index 7f31e39..828c1fc 100644 > --- a/trace/control-internal.h > +++ b/trace/control-internal.h > @@ -16,7 +16,6 @@ > extern TraceEvent trace_events[]; > -extern uint16_t trace_events_dstate[]; > extern int trace_events_enabled_count; > @@ -54,18 +53,13 @@ static inline bool trace_event_get_state_static(TraceEvent *ev) > return ev->sstate; > } > -static inline bool trace_event_get_state_dynamic_by_id(TraceEventID id) > -{ > - /* it's on fast path, avoid consistency checks (asserts) */ > - return unlikely(trace_events_enabled_count) && trace_events_dstate[id]; > -} > +/* it's on fast path, avoid consistency checks (asserts) */ > +#define trace_event_get_state_dynamic_by_id(id) \ > + (unlikely(trace_events_enabled_count) && ___ ## id ## _DSTATE) > static inline bool trace_event_get_state_dynamic(TraceEvent *ev) > { > - TraceEventID id; > - assert(trace_event_get_state_static(ev)); > - id = trace_event_get_id(ev); > - return trace_event_get_state_dynamic_by_id(id); > + return unlikely(trace_events_enabled_count) && *ev->dstate; > } > static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState *vcpu, > diff --git a/trace/control-target.c b/trace/control-target.c > index 72081e2..c69dda9 100644 > --- a/trace/control-target.c > +++ b/trace/control-target.c > @@ -15,21 +15,20 @@ > void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state) > { > - TraceEventID id = trace_event_get_id(ev); > bool state_pre; > assert(trace_event_get_state_static(ev)); > /* > * We ignore the "vcpu" property here, since no vCPUs have been created > * yet. Then dstate can only be 1 or 0. > */ > - state_pre = trace_events_dstate[id]; > + state_pre = *ev->dstate; > if (state_pre != state) { > if (state) { > trace_events_enabled_count++; > - trace_events_dstate[id] = 1; > + *ev->dstate = 1; > } else { > trace_events_enabled_count--; > - trace_events_dstate[id] = 0; > + *ev->dstate = 0; > } > } > } > @@ -44,15 +43,14 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) > } > } else { > /* Without the "vcpu" property, dstate can only be 1 or 0 */ > - TraceEventID id = trace_event_get_id(ev); > - bool state_pre = trace_events_dstate[id]; > + bool state_pre = *ev->dstate; > if (state_pre != state) { > if (state) { > trace_events_enabled_count++; > - trace_events_dstate[id] = 1; > + *ev->dstate = 1; > } else { > trace_events_enabled_count--; > - trace_events_dstate[id] = 0; > + *ev->dstate = 0; > } > } > } > @@ -61,23 +59,21 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) > void trace_event_set_vcpu_state_dynamic(CPUState *vcpu, > TraceEvent *ev, bool state) > { > - TraceEventID id; > TraceEventVCPUID vcpu_id; > bool state_pre; > assert(trace_event_get_state_static(ev)); > assert(trace_event_is_vcpu(ev)); > - id = trace_event_get_id(ev); > vcpu_id = trace_event_get_vcpu_id(ev); > state_pre = test_bit(vcpu_id, vcpu->trace_dstate); > if (state_pre != state) { > if (state) { > trace_events_enabled_count++; > set_bit(vcpu_id, vcpu->trace_dstate); > - trace_events_dstate[id]++; > + (*ev->dstate)++; > } else { > trace_events_enabled_count--; > clear_bit(vcpu_id, vcpu->trace_dstate); > - trace_events_dstate[id]--; > + (*ev->dstate)--; > } > } > } > diff --git a/trace/control.c b/trace/control.c > index 303b3f3..c681fa0 100644 > --- a/trace/control.c > +++ b/trace/control.c > @@ -28,12 +28,6 @@ > #include "monitor/monitor.h" > int trace_events_enabled_count; > -/* > - * Interpretation depends on wether the event has the 'vcpu' property: > - * - false: Boolean value indicating whether the event is active. > - * - true : Integral counting the number of vCPUs that have this event enabled. > - */ > -uint16_t trace_events_dstate[TRACE_EVENT_COUNT]; > QemuOptsList qemu_trace_opts = { > .name = "trace", > @@ -294,11 +288,10 @@ void trace_init_vcpu_events(void) > if (trace_event_is_vcpu(ev) && > trace_event_get_state_static(ev) && > trace_event_get_state_dynamic(ev)) { > - TraceEventID id = trace_event_get_id(ev); > /* check preconditions */ > - assert(trace_events_dstate[id] == 1); > + assert(*ev->dstate == 1); > /* disable early-init state ... */ > - trace_events_dstate[id] = 0; > + *ev->dstate = 0; > trace_events_enabled_count--; > /* ... and properly re-enable */ > trace_event_set_state_dynamic(ev, true); > diff --git a/trace/event-internal.h b/trace/event-internal.h > index 074faf6..3b9ceb5 100644 > --- a/trace/event-internal.h > +++ b/trace/event-internal.h > @@ -19,6 +19,11 @@ > * @vcpu_id: Unique per-vCPU event identifier. > * @name: Event name. > * @sstate: Static tracing state. > + * @dstate: Dynamic tracing state > + * > + * Interpretation of @dstate depends on wether the event has the 'vcpu' property: > + * - false: Boolean value indicating whether the event is active. > + * - true : Integral counting the number of vCPUs that have this event enabled. > * > * Opaque generic description of a tracing event. > */ > @@ -27,6 +32,7 @@ typedef struct TraceEvent { > TraceEventVCPUID vcpu_id; > const char * name; > const bool sstate; > + uint16_t *dstate; > } TraceEvent; > void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state); > -- > 2.7.4
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py index be24039..5191df9 100644 --- a/scripts/tracetool/__init__.py +++ b/scripts/tracetool/__init__.py @@ -265,11 +265,12 @@ class Event(object): QEMU_TRACE = "trace_%(name)s" QEMU_TRACE_TCG = QEMU_TRACE + "_tcg" + QEMU_DSTATE = "___TRACE_%(NAME)s_DSTATE" def api(self, fmt=None): if fmt is None: fmt = Event.QEMU_TRACE - return fmt % {"name": self.name} + return fmt % {"name": self.name, "NAME": self.name.upper()} def transform(self, *trans): """Return a new Event with transformed Arguments.""" diff --git a/scripts/tracetool/format/events_c.py b/scripts/tracetool/format/events_c.py index 4012063..ef873fa 100644 --- a/scripts/tracetool/format/events_c.py +++ b/scripts/tracetool/format/events_c.py @@ -25,6 +25,9 @@ def generate(events, backend): '#include "trace/control.h"', '') + for e in events: + out('uint16_t %s;' % e.api(e.QEMU_DSTATE)) + out('TraceEvent trace_events[TRACE_EVENT_COUNT] = {') for e in events: @@ -34,11 +37,13 @@ def generate(events, backend): vcpu_id = "TRACE_VCPU_EVENT_COUNT" out(' { .id = %(id)s, .vcpu_id = %(vcpu_id)s,' ' .name = \"%(name)s\",' - ' .sstate = %(sstate)s },', + ' .sstate = %(sstate)s,', + ' .dstate = &%(dstate)s, }, ', id = "TRACE_" + e.name.upper(), vcpu_id = vcpu_id, name = e.name, - sstate = "TRACE_%s_ENABLED" % e.name.upper()) + sstate = "TRACE_%s_ENABLED" % e.name.upper(), + dstate = e.api(e.QEMU_DSTATE)) out('};', '') diff --git a/scripts/tracetool/format/events_h.py b/scripts/tracetool/format/events_h.py index a9da60b..03417de 100644 --- a/scripts/tracetool/format/events_h.py +++ b/scripts/tracetool/format/events_h.py @@ -32,6 +32,9 @@ def generate(events, backend): 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 {') diff --git a/stubs/trace-control.c b/stubs/trace-control.c index 2dfcd9f..dd68f25 100644 --- a/stubs/trace-control.c +++ b/stubs/trace-control.c @@ -18,22 +18,21 @@ void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state) void trace_event_set_state_dynamic(TraceEvent *ev, bool state) { - TraceEventID id; bool state_pre; assert(trace_event_get_state_static(ev)); - id = trace_event_get_id(ev); + /* * We ignore the "vcpu" property here, since there's no target code. Then * dstate can only be 1 or 0. */ - state_pre = trace_events_dstate[id]; + state_pre = *(ev->dstate); if (state_pre != state) { if (state) { trace_events_enabled_count++; - trace_events_dstate[id] = 1; + *(ev->dstate) = 1; } else { trace_events_enabled_count--; - trace_events_dstate[id] = 0; + *(ev->dstate) = 0; } } } diff --git a/trace/control-internal.h b/trace/control-internal.h index 7f31e39..828c1fc 100644 --- a/trace/control-internal.h +++ b/trace/control-internal.h @@ -16,7 +16,6 @@ extern TraceEvent trace_events[]; -extern uint16_t trace_events_dstate[]; extern int trace_events_enabled_count; @@ -54,18 +53,13 @@ static inline bool trace_event_get_state_static(TraceEvent *ev) return ev->sstate; } -static inline bool trace_event_get_state_dynamic_by_id(TraceEventID id) -{ - /* it's on fast path, avoid consistency checks (asserts) */ - return unlikely(trace_events_enabled_count) && trace_events_dstate[id]; -} +/* it's on fast path, avoid consistency checks (asserts) */ +#define trace_event_get_state_dynamic_by_id(id) \ + (unlikely(trace_events_enabled_count) && ___ ## id ## _DSTATE) static inline bool trace_event_get_state_dynamic(TraceEvent *ev) { - TraceEventID id; - assert(trace_event_get_state_static(ev)); - id = trace_event_get_id(ev); - return trace_event_get_state_dynamic_by_id(id); + return unlikely(trace_events_enabled_count) && *ev->dstate; } static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState *vcpu, diff --git a/trace/control-target.c b/trace/control-target.c index 72081e2..c69dda9 100644 --- a/trace/control-target.c +++ b/trace/control-target.c @@ -15,21 +15,20 @@ void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state) { - TraceEventID id = trace_event_get_id(ev); bool state_pre; assert(trace_event_get_state_static(ev)); /* * We ignore the "vcpu" property here, since no vCPUs have been created * yet. Then dstate can only be 1 or 0. */ - state_pre = trace_events_dstate[id]; + state_pre = *ev->dstate; if (state_pre != state) { if (state) { trace_events_enabled_count++; - trace_events_dstate[id] = 1; + *ev->dstate = 1; } else { trace_events_enabled_count--; - trace_events_dstate[id] = 0; + *ev->dstate = 0; } } } @@ -44,15 +43,14 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) } } else { /* Without the "vcpu" property, dstate can only be 1 or 0 */ - TraceEventID id = trace_event_get_id(ev); - bool state_pre = trace_events_dstate[id]; + bool state_pre = *ev->dstate; if (state_pre != state) { if (state) { trace_events_enabled_count++; - trace_events_dstate[id] = 1; + *ev->dstate = 1; } else { trace_events_enabled_count--; - trace_events_dstate[id] = 0; + *ev->dstate = 0; } } } @@ -61,23 +59,21 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) void trace_event_set_vcpu_state_dynamic(CPUState *vcpu, TraceEvent *ev, bool state) { - TraceEventID id; TraceEventVCPUID vcpu_id; bool state_pre; assert(trace_event_get_state_static(ev)); assert(trace_event_is_vcpu(ev)); - id = trace_event_get_id(ev); vcpu_id = trace_event_get_vcpu_id(ev); state_pre = test_bit(vcpu_id, vcpu->trace_dstate); if (state_pre != state) { if (state) { trace_events_enabled_count++; set_bit(vcpu_id, vcpu->trace_dstate); - trace_events_dstate[id]++; + (*ev->dstate)++; } else { trace_events_enabled_count--; clear_bit(vcpu_id, vcpu->trace_dstate); - trace_events_dstate[id]--; + (*ev->dstate)--; } } } diff --git a/trace/control.c b/trace/control.c index 303b3f3..c681fa0 100644 --- a/trace/control.c +++ b/trace/control.c @@ -28,12 +28,6 @@ #include "monitor/monitor.h" int trace_events_enabled_count; -/* - * Interpretation depends on wether the event has the 'vcpu' property: - * - false: Boolean value indicating whether the event is active. - * - true : Integral counting the number of vCPUs that have this event enabled. - */ -uint16_t trace_events_dstate[TRACE_EVENT_COUNT]; QemuOptsList qemu_trace_opts = { .name = "trace", @@ -294,11 +288,10 @@ void trace_init_vcpu_events(void) if (trace_event_is_vcpu(ev) && trace_event_get_state_static(ev) && trace_event_get_state_dynamic(ev)) { - TraceEventID id = trace_event_get_id(ev); /* check preconditions */ - assert(trace_events_dstate[id] == 1); + assert(*ev->dstate == 1); /* disable early-init state ... */ - trace_events_dstate[id] = 0; + *ev->dstate = 0; trace_events_enabled_count--; /* ... and properly re-enable */ trace_event_set_state_dynamic(ev, true); diff --git a/trace/event-internal.h b/trace/event-internal.h index 074faf6..3b9ceb5 100644 --- a/trace/event-internal.h +++ b/trace/event-internal.h @@ -19,6 +19,11 @@ * @vcpu_id: Unique per-vCPU event identifier. * @name: Event name. * @sstate: Static tracing state. + * @dstate: Dynamic tracing state + * + * Interpretation of @dstate depends on wether the event has the 'vcpu' property: + * - false: Boolean value indicating whether the event is active. + * - true : Integral counting the number of vCPUs that have this event enabled. * * Opaque generic description of a tracing event. */ @@ -27,6 +32,7 @@ typedef struct TraceEvent { TraceEventVCPUID vcpu_id; const char * name; const bool sstate; + uint16_t *dstate; } TraceEvent; void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state);
Instead of having a global dstate array, declare a single 'uint16 TRACE_${EVENT_NAME}_DSTATE' variable for each trace event. Record a pointer to this variable in the TraceEvent struct too. By turning trace_event_get_state_dynamic_by_id into a macro, this still hits the fast path, and cache affinity is ensured by declaring all the uint16 vars adjacent to each other. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- scripts/tracetool/__init__.py | 3 ++- scripts/tracetool/format/events_c.py | 9 +++++++-- scripts/tracetool/format/events_h.py | 3 +++ stubs/trace-control.c | 9 ++++----- trace/control-internal.h | 14 ++++---------- trace/control-target.c | 20 ++++++++------------ trace/control.c | 11 ++--------- trace/event-internal.h | 6 ++++++ 8 files changed, 36 insertions(+), 39 deletions(-)