[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]'
From: |
Lluís Vilanova |
Subject: |
Re: [Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]' array |
Date: |
Mon, 19 Sep 2016 19:05:27 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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 <address@hidden>
Besides Eric's comments:
Reviewed-by: Lluís Vilanova <address@hidden>
> ---
> 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
- Re: [Qemu-devel] [PATCH v3 02/18] trace: convert code to use event iterators, (continued)
- [Qemu-devel] [PATCH v3 01/18] trace: add trace event iterator APIs, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 03/18] trace: remove some now unused functions, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 05/18] trace: remove duplicate control.h includes in generated-tracers.h, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]' array, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 06/18] trace: break circular dependancy in event-internal.h, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 12/18] trace: dynamically allocate trace_dstate in CPUState, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 08/18] trace: remove the TraceEventID and TraceEventVCPUID enums, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 09/18] trace: emit name <-> ID mapping in simpletrace header, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 15/18] trace: rename _read_events to read_events, Daniel P. Berrange, 2016/09/19