Re: [Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]' array
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
Besides Eric's comments:
Reviewed-by: Lluís Vilanova
> ---
> 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
Re: [Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]' array
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
> ---
> +++ 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.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]' array
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
---
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;
-asse
