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

["Lluís Vilanova" <vilanova@ac.upc.edu>]

Stefan Hajnoczi writes:

> 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 :).

I'll refrain from pushing my manias into QEMU :)


[...]
>> > 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.

Fair point. But now I feel tempted to change both trace_events_dstate and
trace_events_enabled_count into unsigned int... it burns me when I see signed
types used only on their positives by design.

But don't worry, I'll change trace_events_dstate into int :)


Thanks!
  Lluis