* [Qemu-devel] [PATCH v4 1/2] trace: Remove 'trace_events_dstate_init'
2016-08-12 15:33 [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization Lluís Vilanova
@ 2016-08-12 15:33 ` Lluís Vilanova
2016-08-18 8:03 ` Daniel P. Berrange
2016-08-12 15:33 ` [Qemu-devel] [PATCH v4 2/2] trace: Avoid implicit bool->integer conversions Lluís Vilanova
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Lluís Vilanova @ 2016-08-12 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, 'Daniel P. Berrange', Stefan Hajnoczi
Removes the event state array used for early initialization. Since only
events with the "vcpu" property need a late initialization fixup,
threats their initialization specially.
Assumes that the user won't touch the state of "vcpu" events between
early and late initialization (e.g., through QMP).
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
stubs/trace-control.c | 5 +++++
trace/control-target.c | 9 +++++++++
trace/control.c | 23 ++++++++++-------------
trace/control.h | 3 +++
trace/event-internal.h | 1 +
5 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/stubs/trace-control.c b/stubs/trace-control.c
index fe59836..3740c38 100644
--- a/stubs/trace-control.c
+++ b/stubs/trace-control.c
@@ -11,6 +11,11 @@
#include "trace/control.h"
+void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
+{
+ trace_event_set_state_dynamic(ev, state);
+}
+
void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
{
TraceEventID id;
diff --git a/trace/control-target.c b/trace/control-target.c
index 74c029a..4ee3733 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -13,6 +13,15 @@
#include "translate-all.h"
+void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
+{
+ TraceEventID id = trace_event_get_id(ev);
+ assert(trace_event_get_state_static(ev));
+ /* Ignore "vcpu" property, since no vCPUs have been created yet */
+ trace_events_enabled_count += state - trace_events_dstate[id];
+ trace_events_dstate[id] = state;
+}
+
void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
{
CPUState *vcpu;
diff --git a/trace/control.c b/trace/control.c
index d173c09..f6e06cc 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -31,8 +31,6 @@ int trace_events_enabled_count;
* - true : Integral counting the number of vCPUs that have this event enabled.
*/
uint16_t trace_events_dstate[TRACE_EVENT_COUNT];
-/* Marks events for late vCPU state init */
-static bool trace_events_dstate_init[TRACE_EVENT_COUNT];
QemuOptsList qemu_trace_opts = {
.name = "trace",
@@ -142,10 +140,7 @@ static void do_trace_enable_events(const char *line_buf)
TraceEvent *ev = NULL;
while ((ev = trace_event_pattern(line_ptr, ev)) != NULL) {
if (trace_event_get_state_static(ev)) {
- /* start tracing */
- trace_event_set_state_dynamic(ev, enable);
- /* mark for late vCPU init */
- trace_events_dstate_init[ev->id] = true;
+ trace_event_set_state_dynamic_init(ev, enable);
}
}
} else {
@@ -157,10 +152,7 @@ static void do_trace_enable_events(const char *line_buf)
error_report("WARNING: trace event '%s' is not traceable",
line_ptr);
} else {
- /* start tracing */
- trace_event_set_state_dynamic(ev, enable);
- /* mark for late vCPU init */
- trace_events_dstate_init[ev->id] = true;
+ trace_event_set_state_dynamic_init(ev, enable);
}
}
}
@@ -275,9 +267,14 @@ void trace_init_vcpu_events(void)
{
TraceEvent *ev = NULL;
while ((ev = trace_event_pattern("*", ev)) != NULL) {
- if (trace_event_is_vcpu(ev) &&
- trace_event_get_state_static(ev) &&
- trace_events_dstate_init[ev->id]) {
+ if (trace_event_is_vcpu(ev) && trace_event_get_state_dynamic(ev)) {
+ TraceEventID id = trace_event_get_id(ev);
+ /* check preconditions */
+ assert(trace_events_dstate[id] == 1);
+ /* disable early-init state ... */
+ trace_events_dstate[id] = 0;
+ trace_events_enabled_count--;
+ /* ... and properly re-enable */
trace_event_set_state_dynamic(ev, true);
}
}
diff --git a/trace/control.h b/trace/control.h
index 0413b28..27a16fc 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -274,6 +274,9 @@ char *trace_opt_parse(const char *optarg);
*
* Re-synchronize initial event state with vCPUs (which can be created after
* trace_init_events()).
+ *
+ * Precondition: event states won't be changed between trace_enable_events() and
+ * trace_init_vcpu_events() (e.g., through QMP).
*/
void trace_init_vcpu_events(void);
diff --git a/trace/event-internal.h b/trace/event-internal.h
index 5d8fa97..074faf6 100644
--- a/trace/event-internal.h
+++ b/trace/event-internal.h
@@ -29,5 +29,6 @@ typedef struct TraceEvent {
const bool sstate;
} TraceEvent;
+void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state);
#endif /* TRACE__EVENT_INTERNAL_H */
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH v4 2/2] trace: Avoid implicit bool->integer conversions
2016-08-12 15:33 [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization Lluís Vilanova
2016-08-12 15:33 ` [Qemu-devel] [PATCH v4 1/2] trace: Remove 'trace_events_dstate_init' Lluís Vilanova
@ 2016-08-12 15:33 ` Lluís Vilanova
2016-08-18 8:06 ` Daniel P. Berrange
2016-08-17 15:41 ` [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Lluís Vilanova @ 2016-08-12 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, 'Daniel P. Berrange', Stefan Hajnoczi
An explicit if/else is clearer than arithmetic assuming #true is 1,
while the compiler should be able to generate just as optimal code.
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
stubs/trace-control.c | 17 +++++++++++++++--
trace/control-target.c | 31 ++++++++++++++++++++++++++-----
2 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/stubs/trace-control.c b/stubs/trace-control.c
index 3740c38..2dfcd9f 100644
--- a/stubs/trace-control.c
+++ b/stubs/trace-control.c
@@ -19,10 +19,23 @@ 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);
- trace_events_enabled_count += state - trace_events_dstate[id];
- trace_events_dstate[id] = state;
+ /*
+ * 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];
+ if (state_pre != state) {
+ if (state) {
+ trace_events_enabled_count++;
+ trace_events_dstate[id] = 1;
+ } else {
+ trace_events_enabled_count--;
+ trace_events_dstate[id] = 0;
+ }
+ }
}
void trace_event_set_vcpu_state_dynamic(CPUState *vcpu,
diff --git a/trace/control-target.c b/trace/control-target.c
index 4ee3733..72081e2 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -16,10 +16,22 @@
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));
- /* Ignore "vcpu" property, since no vCPUs have been created yet */
- trace_events_enabled_count += state - trace_events_dstate[id];
- trace_events_dstate[id] = state;
+ /*
+ * 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];
+ if (state_pre != state) {
+ if (state) {
+ trace_events_enabled_count++;
+ trace_events_dstate[id] = 1;
+ } else {
+ trace_events_enabled_count--;
+ trace_events_dstate[id] = 0;
+ }
+ }
}
void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
@@ -31,9 +43,18 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
trace_event_set_vcpu_state_dynamic(vcpu, ev, state);
}
} else {
+ /* Without the "vcpu" property, dstate can only be 1 or 0 */
TraceEventID id = trace_event_get_id(ev);
- trace_events_enabled_count += state - trace_events_dstate[id];
- trace_events_dstate[id] = state;
+ bool state_pre = trace_events_dstate[id];
+ if (state_pre != state) {
+ if (state) {
+ trace_events_enabled_count++;
+ trace_events_dstate[id] = 1;
+ } else {
+ trace_events_enabled_count--;
+ trace_events_dstate[id] = 0;
+ }
+ }
}
}
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v4 2/2] trace: Avoid implicit bool->integer conversions
2016-08-12 15:33 ` [Qemu-devel] [PATCH v4 2/2] trace: Avoid implicit bool->integer conversions Lluís Vilanova
@ 2016-08-18 8:06 ` Daniel P. Berrange
0 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrange @ 2016-08-18 8:06 UTC (permalink / raw)
To: Lluís Vilanova; +Cc: qemu-devel, Paolo Bonzini, Stefan Hajnoczi
On Fri, Aug 12, 2016 at 05:33:46PM +0200, Lluís Vilanova wrote:
> An explicit if/else is clearer than arithmetic assuming #true is 1,
> while the compiler should be able to generate just as optimal code.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> stubs/trace-control.c | 17 +++++++++++++++--
> trace/control-target.c | 31 ++++++++++++++++++++++++++-----
> 2 files changed, 41 insertions(+), 7 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
>
> diff --git a/stubs/trace-control.c b/stubs/trace-control.c
> index 3740c38..2dfcd9f 100644
> --- a/stubs/trace-control.c
> +++ b/stubs/trace-control.c
> @@ -19,10 +19,23 @@ 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);
> - trace_events_enabled_count += state - trace_events_dstate[id];
> - trace_events_dstate[id] = state;
> + /*
> + * 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];
> + if (state_pre != state) {
> + if (state) {
> + trace_events_enabled_count++;
> + trace_events_dstate[id] = 1;
> + } else {
> + trace_events_enabled_count--;
> + trace_events_dstate[id] = 0;
> + }
> + }
> }
>
> void trace_event_set_vcpu_state_dynamic(CPUState *vcpu,
> diff --git a/trace/control-target.c b/trace/control-target.c
> index 4ee3733..72081e2 100644
> --- a/trace/control-target.c
> +++ b/trace/control-target.c
> @@ -16,10 +16,22 @@
> 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));
> - /* Ignore "vcpu" property, since no vCPUs have been created yet */
> - trace_events_enabled_count += state - trace_events_dstate[id];
> - trace_events_dstate[id] = state;
> + /*
> + * 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];
> + if (state_pre != state) {
> + if (state) {
> + trace_events_enabled_count++;
> + trace_events_dstate[id] = 1;
> + } else {
> + trace_events_enabled_count--;
> + trace_events_dstate[id] = 0;
> + }
> + }
> }
>
> void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
> @@ -31,9 +43,18 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
> trace_event_set_vcpu_state_dynamic(vcpu, ev, state);
> }
> } else {
> + /* Without the "vcpu" property, dstate can only be 1 or 0 */
> TraceEventID id = trace_event_get_id(ev);
> - trace_events_enabled_count += state - trace_events_dstate[id];
> - trace_events_dstate[id] = state;
> + bool state_pre = trace_events_dstate[id];
> + if (state_pre != state) {
> + if (state) {
> + trace_events_enabled_count++;
> + trace_events_dstate[id] = 1;
> + } else {
> + trace_events_enabled_count--;
> + trace_events_dstate[id] = 0;
> + }
> + }
> }
> }
>
>
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 :|
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization
2016-08-12 15:33 [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization Lluís Vilanova
2016-08-12 15:33 ` [Qemu-devel] [PATCH v4 1/2] trace: Remove 'trace_events_dstate_init' Lluís Vilanova
2016-08-12 15:33 ` [Qemu-devel] [PATCH v4 2/2] trace: Avoid implicit bool->integer conversions Lluís Vilanova
@ 2016-08-17 15:41 ` Stefan Hajnoczi
2016-08-18 10:59 ` Stefan Hajnoczi
2016-08-18 13:07 ` Stefan Hajnoczi
4 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2016-08-17 15:41 UTC (permalink / raw)
To: Lluís Vilanova
Cc: qemu-devel, Paolo Bonzini, 'Daniel P. Berrange'
[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]
On Fri, Aug 12, 2016 at 05:33:35PM +0200, Lluís Vilanova wrote:
> Removes the need for 'trace_events_dstate_init' and does a little cleanup in how
> state values are modified (to avoid implicit conversions from bool).
>
> Changes in v2
> =============
>
> * Fix late-init state value [Daniel P. Berrange].
>
> Changes in v3
> =============
>
> * Avoid implicit conversions from bool to integers (second patch) [Daniel
> P. Berrange].
>
> Changes in v4
> =============
>
> * Correctly implement idempotent state changes.
> * Clarify when state changes are idempotent [Daniel P. Berrange].
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>
> Lluís Vilanova (2):
> trace: Remove 'trace_events_dstate_init'
> trace: Avoid implicit bool->integer conversions
>
>
> stubs/trace-control.c | 22 ++++++++++++++++++++--
> trace/control-target.c | 34 ++++++++++++++++++++++++++++++++--
> trace/control.c | 23 ++++++++++-------------
> trace/control.h | 3 +++
> trace/event-internal.h | 1 +
> 5 files changed, 66 insertions(+), 17 deletions(-)
>
>
> To: qemu-devel@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: 'Daniel P. Berrange' <berrange@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
I will wait for Daniel Berrange to review before merging since he had
comments on previous revisions.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization
2016-08-12 15:33 [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization Lluís Vilanova
` (2 preceding siblings ...)
2016-08-17 15:41 ` [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization Stefan Hajnoczi
@ 2016-08-18 10:59 ` Stefan Hajnoczi
2016-08-18 13:07 ` Stefan Hajnoczi
4 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2016-08-18 10:59 UTC (permalink / raw)
To: Lluís Vilanova; +Cc: qemu-devel, Paolo Bonzini, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]
On Fri, Aug 12, 2016 at 05:33:35PM +0200, Lluís Vilanova wrote:
> Removes the need for 'trace_events_dstate_init' and does a little cleanup in how
> state values are modified (to avoid implicit conversions from bool).
>
> Changes in v2
> =============
>
> * Fix late-init state value [Daniel P. Berrange].
>
> Changes in v3
> =============
>
> * Avoid implicit conversions from bool to integers (second patch) [Daniel
> P. Berrange].
>
> Changes in v4
> =============
>
> * Correctly implement idempotent state changes.
> * Clarify when state changes are idempotent [Daniel P. Berrange].
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>
> Lluís Vilanova (2):
> trace: Remove 'trace_events_dstate_init'
> trace: Avoid implicit bool->integer conversions
>
>
> stubs/trace-control.c | 22 ++++++++++++++++++++--
> trace/control-target.c | 34 ++++++++++++++++++++++++++++++++--
> trace/control.c | 23 ++++++++++-------------
> trace/control.h | 3 +++
> trace/event-internal.h | 1 +
> 5 files changed, 66 insertions(+), 17 deletions(-)
>
>
> To: qemu-devel@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: 'Daniel P. Berrange' <berrange@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
>
Thanks, applied to my tracing-next tree:
https://github.com/stefanha/qemu/commits/tracing-next
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization
2016-08-12 15:33 [Qemu-devel] [PATCH v4 0/2] trace: Simplify late initialization Lluís Vilanova
` (3 preceding siblings ...)
2016-08-18 10:59 ` Stefan Hajnoczi
@ 2016-08-18 13:07 ` Stefan Hajnoczi
4 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2016-08-18 13:07 UTC (permalink / raw)
To: Lluís Vilanova; +Cc: qemu-devel, Paolo Bonzini, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]
On Fri, Aug 12, 2016 at 05:33:35PM +0200, Lluís Vilanova wrote:
> Removes the need for 'trace_events_dstate_init' and does a little cleanup in how
> state values are modified (to avoid implicit conversions from bool).
>
> Changes in v2
> =============
>
> * Fix late-init state value [Daniel P. Berrange].
>
> Changes in v3
> =============
>
> * Avoid implicit conversions from bool to integers (second patch) [Daniel
> P. Berrange].
>
> Changes in v4
> =============
>
> * Correctly implement idempotent state changes.
> * Clarify when state changes are idempotent [Daniel P. Berrange].
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>
> Lluís Vilanova (2):
> trace: Remove 'trace_events_dstate_init'
> trace: Avoid implicit bool->integer conversions
>
>
> stubs/trace-control.c | 22 ++++++++++++++++++++--
> trace/control-target.c | 34 ++++++++++++++++++++++++++++++++--
> trace/control.c | 23 ++++++++++-------------
> trace/control.h | 3 +++
> trace/event-internal.h | 1 +
> 5 files changed, 66 insertions(+), 17 deletions(-)
>
>
> To: qemu-devel@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: 'Daniel P. Berrange' <berrange@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
The Travis CI system is reporting the following failure:
GTESTER check-qtest-aarch64
qemu-system-aarch64: ./trace/control-internal.h:77: trace_event_get_state_dynamic: Assertion `trace_event_get_state_static(ev)' failed.
https://travis-ci.org/stefanha/qemu/jobs/153235649
Please send a new revision that fixes the issue.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread