From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37949) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBPFV-0005VX-DV for qemu-devel@nongnu.org; Fri, 10 Jun 2016 12:25:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bBPFO-0007IK-V6 for qemu-devel@nongnu.org; Fri, 10 Jun 2016 12:25:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38683) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBPFO-0007Hi-Mj for qemu-devel@nongnu.org; Fri, 10 Jun 2016 12:25:26 -0400 Date: Fri, 10 Jun 2016 17:25:24 +0100 From: Stefan Hajnoczi Message-ID: <20160610162524.GF3855@stefanha-x1.localdomain> References: <145641255678.30097.2919142707547689749.stgit@localhost> <145641258612.30097.7127731954660712163.stgit@localhost> <20160609120756.GA15369@stefanha-x1.localdomain> <871t4630rc.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="2FkSFaIQeDFoAt0B" Content-Disposition: inline In-Reply-To: <871t4630rc.fsf@fimbulvetr.bsc.es> 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 , Eduardo Habkost , Riku Voipio , qemu-devel@nongnu.org, Blue Swirl , Paolo Bonzini , Andreas =?iso-8859-1?Q?F=E4rber?= --2FkSFaIQeDFoAt0B Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 09, 2016 at 04:17:11PM +0200, Llu=EDs Vilanova wrote: > >> @@ -61,7 +69,7 @@ static inline bool trace_event_get_state_static(Trac= eEvent *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_dstat= e[id]; > >> + return unlikely(trace_events_enabled_count) && (trace_events_dsta= te[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 ty= pes 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 !=3D NULL); > >> + assert(trace_event_get_state_static(ev)); > >> + if (trace_event_get_cpu_id(ev) !=3D trace_event_cpu_count()) { > >> + CPU_FOREACH(cpu) { > >> + trace_event_set_cpu_state_dynamic(cpu, ev, state); > >> + } > >> + } else { > >> + TraceEventID id =3D trace_event_get_id(ev); > >> + trace_events_enabled_count +=3D state - trace_events_dstate[i= d]; > >> + trace_events_dstate[id] =3D state; > >> + } > >> +} >=20 > > I find it a little confusing to use different semantics for > > trace_events_dstate[] elements depending on trace_event_get_cpu_id(ev) > > !=3D trace_event_cpu_count(). In other words, it either acts as a vcpu > > enabled counter or as an enable/disable flag. >=20 > > 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. >=20 > 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)). >=20 > > Why did you choose size_t? >=20 > It just sounds proper to me to use size_t, since the state can never be n= egative > (it's either interpreted as a boolean or as an unsigned counter, dependin= g 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. Stefan --2FkSFaIQeDFoAt0B Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJXWunzAAoJEJykq7OBq3PILI0IAIDVgSJTYjH+f2DZ0Dar9WNf 0Tsf6pCQSwuBU0vYGJXBB9hpjFpRoknwqQIV3kSHoXbnk3CrcKYCsyYE4Der6Hxs U3/ia4lm59nJLuTKwBzTOF5bzFRjObw7Z3bBmUazxT8XoGzMVT9v7G1TWfrbataO uf6roWQvNrCp8pvPIw3Ujkqxc7E1BVFoHesWYWSB/sy7rErvrlKtzOJZjMoQTf4a 6KuQNAc6RNx7jSQwLKA/1h+cZoAj3HAkzAsiyDbn/rmNc75CPFFgrKAMCbOS6+Rw iAUf79k3icmw2VeyhoL/7BLl3JqW7yxc+Bf0r8Dt1kE52XYXstbXcGgtR0GPRY8= =k+AZ -----END PGP SIGNATURE----- --2FkSFaIQeDFoAt0B--