From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59125) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBQbg-0008CI-VL for qemu-devel@nongnu.org; Fri, 10 Jun 2016 13:52:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bBQba-0003H1-V0 for qemu-devel@nongnu.org; Fri, 10 Jun 2016 13:52:31 -0400 Received: from roura.ac.upc.es ([147.83.33.10]:35562) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBQba-0003Gp-K1 for qemu-devel@nongnu.org; Fri, 10 Jun 2016 13:52:26 -0400 From: =?utf-8?Q?Llu=C3=ADs_Vilanova?= References: <145641255678.30097.2919142707547689749.stgit@localhost> <145641258612.30097.7127731954660712163.stgit@localhost> <20160609120756.GA15369@stefanha-x1.localdomain> <871t4630rc.fsf@fimbulvetr.bsc.es> <20160610162524.GF3855@stefanha-x1.localdomain> Date: Fri, 10 Jun 2016 19:52:16 +0200 In-Reply-To: <20160610162524.GF3855@stefanha-x1.localdomain> (Stefan Hajnoczi's message of "Fri, 10 Jun 2016 17:25:24 +0100") Message-ID: <87bn39j5in.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/6] trace: Add per-vCPU tracing states for events with the 'vcpu' property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Stefan Hajnoczi , Eduardo Habkost , Riku Voipio , qemu-devel@nongnu.org, Blue Swirl , Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= Stefan Hajnoczi writes: > On Thu, Jun 09, 2016 at 04:17:11PM +0200, Llu=C3=ADs Vilanova wrote: >> >> @@ -61,7 +69,7 @@ static inline bool trace_event_get_state_static(Tra= ceEvent *ev) >> >> static inline bool trace_event_get_state_dynamic_by_id(TraceEventID i= d) >> >> { >> >> /* it's on fast path, avoid consistency checks (asserts) */ >> >> - return unlikely(trace_events_enabled_count) && trace_events_dsta= te[id]; >> >> + return unlikely(trace_events_enabled_count) && (trace_events_dst= ate[id] > 0); >>=20 >> > 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? >>=20 >> Sorry, I have a tendency to make this type of checks explicit when the t= ypes 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)= ). >>=20 >> > Why did you choose size_t? >>=20 >> 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, dependi= ng 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 +=3D bool - > size_t" statement for updating trace_events_enabled_count. > If int is used then it's clear that int =3D (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 =3D (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 sign= ed types used only on their positives by design. But don't worry, I'll change trace_events_dstate into int :) Thanks! Lluis