Re: [Qemu-devel] [PATCH 4/6] trace: Add per-vCPU tracing states for events with the 'vcpu' property

[Stefan Hajnoczi]

On Thu, Jun 09, 2016 at 04:17:11PM +0200, Lluís Vilanova wrote:
> >> @@ -61,7 +69,7 @@ static inline bool 
> >> trace_event_get_state_static(TraceEvent *ev)
> >> 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];
> >> +    return unlikely(trace_events_enabled_count) && 
> >> (trace_events_dstate[id] > 0);
> 
> > typeof(trace_events_dstate[0]) is size_t, so trace_events_dstate[id] > 0
> > is equivalent to trace_events_dstate[id] (due to unsigned).  Why change
> > this line?
> 
> Sorry, I have a tendency to make this type of checks explicit when the types 
> are
> not boolean (for a maybe-false sense of future-proofing). I can leave it as it
> was if it bothers you.

When reviewing patches I try to understand each change.  When I don't
see a reason for a change I need to ask.

In general it's easier to leave code as-is unless there is a need to
change it.  But there are no hard rules :).

> >> +void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
> >> +{
> >> +    CPUState *cpu;
> >> +    assert(ev != NULL);
> >> +    assert(trace_event_get_state_static(ev));
> >> +    if (trace_event_get_cpu_id(ev) != trace_event_cpu_count()) {
> >> +        CPU_FOREACH(cpu) {
> >> +            trace_event_set_cpu_state_dynamic(cpu, ev, state);
> >> +        }
> >> +    } else {
> >> +        TraceEventID id = trace_event_get_id(ev);
> >> +        trace_events_enabled_count += state - trace_events_dstate[id];
> >> +        trace_events_dstate[id] = state;
> >> +    }
> >> +}
> 
> > I find it a little confusing to use different semantics for
> > trace_events_dstate[] elements depending on trace_event_get_cpu_id(ev)
> > != trace_event_cpu_count().  In other words, it either acts as a vcpu
> > enabled counter or as an enable/disable flag.
> 
> > That said, it's nice to preserve the non-cpu_id case since it was
> > written by Paolo as a performance optimization.  Changing it could
> > introduce a regression so I think your approach is okay.
> 
> Yes, it's a bit messy. I'll add some proper documentation about how this is
> interpreted.

Thanks!

> > The number of cpus has type int (see CPUState *qemu_get_cpu(int index)).
> 
> > Why did you choose size_t?
> 
> It just sounds proper to me to use size_t, since the state can never be 
> negative
> (it's either interpreted as a boolean or as an unsigned counter, depending on
> the "vcpu" property).

If you feel strongly about it, feel free to keep it.  Alternative
reasoning about the type:

int is the CPU index type used in qemu_get_cpu().  It is guaranteed to
be large enough for the vcpu count.  IMO there's no need to select a new
type, but there's more...

size_t is larger than necessary on 64-bit machines and has an impact on
the CPU cache performance that Paolo's optimization takes advantage of
(if you trigger adjacent trace event IDs they will probably already be
in cache).

size_t made me have to think hard when reading the "int += bool -
size_t" statement for updating trace_events_enabled_count.

If int is used then it's clear that int = (int)bool - int will be one of
[-1, 0, +1].

But with size_t you have to starting wondering whether the type coercion
is portable and works as expected:

int = (int)((size_t)bool - size_t);

In "6.3.1.3 Signed and unsigned integers" the C99 standard says:

  [If] the new type is signed and the value cannot be represented in
  it; either the result is implementation-defined or an
  implementation-defined signal is raised.

The size_t -> int conversion is therefore implementation-defined.  This
is not portable although QEMU probably does it in many places.

So for these reasons, I think int is the natural choice.

Stefan

signature.asc
Description: PGP signature