| Message ID | 147093188968.24979.8739712285459220697.stgit@fimbulvetr.bsc.es |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 11, 2016 at 06:11:29PM +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 | 9 +++++++-- > trace/control-target.c | 18 ++++++++++++++---- > 2 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/stubs/trace-control.c b/stubs/trace-control.c > index 3740c38..099c2b5 100644 > --- a/stubs/trace-control.c > +++ b/stubs/trace-control.c > @@ -21,8 +21,13 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) > TraceEventID id; > 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; > + if (state) { > + trace_events_enabled_count++; > + trace_events_dstate[id] = 1; > + } else { > + trace_events_enabled_count--; SHouldn't this be trace_events_enabled_count -= trace_events_dstate[id]; as it is possible to call this method for a trace event that is a per-vcpu event > + 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..4e2e727 100644 > --- a/trace/control-target.c > +++ b/trace/control-target.c > @@ -18,8 +18,13 @@ 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; > + 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) > @@ -32,8 +37,13 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) > } > } else { > TraceEventID id = trace_event_get_id(ev); > - trace_events_enabled_count += state - trace_events_dstate[id]; > - trace_events_dstate[id] = state; > + if (state) { > + trace_events_enabled_count++; > + trace_events_dstate[id] = 1; > + } else { > + trace_events_enabled_count--; > + trace_events_dstate[id] = 0; > + } > } > } > > Regards, Daniel
Daniel P Berrange writes: > On Thu, Aug 11, 2016 at 06:11:29PM +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 | 9 +++++++-- >> trace/control-target.c | 18 ++++++++++++++---- >> 2 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/stubs/trace-control.c b/stubs/trace-control.c >> index 3740c38..099c2b5 100644 >> --- a/stubs/trace-control.c >> +++ b/stubs/trace-control.c >> @@ -21,8 +21,13 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) >> TraceEventID id; >> 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; >> + if (state) { >> + trace_events_enabled_count++; >> + trace_events_dstate[id] = 1; >> + } else { >> + trace_events_enabled_count--; > SHouldn't this be trace_events_enabled_count -= trace_events_dstate[id]; > as it is possible to call this method for a trace event that is a per-vcpu > event The "stub" code will never have vCPUs around, it's used by qemu-img and the like (any program without "target" code). I can send a v4 explicitly commenting that if you think that's necessary. Thanks, Lluis
On Thu, Aug 11, 2016 at 06:41:36PM +0200, Lluís Vilanova wrote: > Daniel P Berrange writes: > > > On Thu, Aug 11, 2016 at 06:11:29PM +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 | 9 +++++++-- > >> trace/control-target.c | 18 ++++++++++++++---- > >> 2 files changed, 21 insertions(+), 6 deletions(-) > >> > >> diff --git a/stubs/trace-control.c b/stubs/trace-control.c > >> index 3740c38..099c2b5 100644 > >> --- a/stubs/trace-control.c > >> +++ b/stubs/trace-control.c > >> @@ -21,8 +21,13 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) > >> TraceEventID id; > >> 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; > >> + if (state) { > >> + trace_events_enabled_count++; > >> + trace_events_dstate[id] = 1; > >> + } else { > >> + trace_events_enabled_count--; > > > SHouldn't this be trace_events_enabled_count -= trace_events_dstate[id]; > > as it is possible to call this method for a trace event that is a per-vcpu > > event > > The "stub" code will never have vCPUs around, it's used by qemu-img and the like > (any program without "target" code). I just picked the first example of that in this file - you've got the same pattern in the other 2 places you change Regards, Daniel
Daniel P Berrange writes: > On Thu, Aug 11, 2016 at 06:41:36PM +0200, Lluís Vilanova wrote: >> Daniel P Berrange writes: >> >> > On Thu, Aug 11, 2016 at 06:11:29PM +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 | 9 +++++++-- >> >> trace/control-target.c | 18 ++++++++++++++---- >> >> 2 files changed, 21 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/stubs/trace-control.c b/stubs/trace-control.c >> >> index 3740c38..099c2b5 100644 >> >> --- a/stubs/trace-control.c >> >> +++ b/stubs/trace-control.c >> >> @@ -21,8 +21,13 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) >> >> TraceEventID id; >> >> 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; >> >> + if (state) { >> >> + trace_events_enabled_count++; >> >> + trace_events_dstate[id] = 1; >> >> + } else { >> >> + trace_events_enabled_count--; >> >> > SHouldn't this be trace_events_enabled_count -= trace_events_dstate[id]; >> > as it is possible to call this method for a trace event that is a per-vcpu >> > event >> >> The "stub" code will never have vCPUs around, it's used by qemu-img and the like >> (any program without "target" code). > I just picked the first example of that in this file - you've got the same > pattern in the other 2 places you change In the "control-target.c" file, there's two places where this pattern also appears. In trace_event_set_state_dynamic(), the pattern only appears on events that do *not* have the "vcpu" property, and so the counter is always either 1 or 0. In trace_event_set_state_dynamic_init() there's the "special" case where we assume there won't be multiple enables of a single event with "vcpu" (as the comment says); this is what let's us get rid of the dstate_init array. What I just saw is wrong is that this code looses the ability to make enable/disable operations idempotent. I'll send a v4 with that fixed. Thanks, Lluis
diff --git a/stubs/trace-control.c b/stubs/trace-control.c index 3740c38..099c2b5 100644 --- a/stubs/trace-control.c +++ b/stubs/trace-control.c @@ -21,8 +21,13 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) TraceEventID id; 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; + 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..4e2e727 100644 --- a/trace/control-target.c +++ b/trace/control-target.c @@ -18,8 +18,13 @@ 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; + 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) @@ -32,8 +37,13 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) } } else { TraceEventID id = trace_event_get_id(ev); - trace_events_enabled_count += state - trace_events_dstate[id]; - trace_events_dstate[id] = state; + if (state) { + trace_events_enabled_count++; + trace_events_dstate[id] = 1; + } else { + trace_events_enabled_count--; + trace_events_dstate[id] = 0; + } } }
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 | 9 +++++++-- trace/control-target.c | 18 ++++++++++++++---- 2 files changed, 21 insertions(+), 6 deletions(-)