[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 4/6] trace: Add per-vCPU tracing states for even

From: Lluís Vilanova
Subject: Re: [Qemu-devel] [PATCH 4/6] trace: Add per-vCPU tracing states for events with the 'vcpu' property
Date: Fri, 10 Jun 2016 19:52:16 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]