From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAykn-0002qi-LA for qemu-devel@nongnu.org; Thu, 09 Jun 2016 08:08:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAyki-0002h3-J1 for qemu-devel@nongnu.org; Thu, 09 Jun 2016 08:08:04 -0400 Received: from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241]:36077) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAyki-0002gm-8Q for qemu-devel@nongnu.org; Thu, 09 Jun 2016 08:08:00 -0400 Received: by mail-wm0-x241.google.com with SMTP id m124so10129240wme.3 for ; Thu, 09 Jun 2016 05:08:00 -0700 (PDT) Date: Thu, 9 Jun 2016 13:07:56 +0100 From: Stefan Hajnoczi Message-ID: <20160609120756.GA15369@stefanha-x1.localdomain> References: <145641255678.30097.2919142707547689749.stgit@localhost> <145641258612.30097.7127731954660712163.stgit@localhost> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="n8g4imXOkfNTN/H1" Content-Disposition: inline In-Reply-To: <145641258612.30097.7127731954660712163.stgit@localhost> 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: =?iso-8859-1?Q?Llu=EDs?= Vilanova Cc: qemu-devel@nongnu.org, Eduardo Habkost , Riku Voipio , Blue Swirl , Stefan Hajnoczi , Paolo Bonzini , Andreas =?iso-8859-1?Q?F=E4rber?= --n8g4imXOkfNTN/H1 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 25, 2016 at 04:03:06PM +0100, Llu=EDs Vilanova wrote: @@ -332,6 +334,16 @@ struct CPUState { > struct KVMState *kvm_state; > struct kvm_run *kvm_run; > =20 > +#define TRACE_VCPU_DSTATE_TYPE uint32_t > + TRACE_VCPU_DSTATE_TYPE trace_dstate; Please use typedef instead of #define. > + /* > + * Ensure 'trace_dstate' can encode event states as a bitmask. This = limits > + * the number of events with the 'vcpu' property and *without* the > + * 'disabled' property. > + */ > + bool too_many_vcpu_events[ > + TRACE_VCPU_EVENT_COUNT > sizeof(TRACE_VCPU_DSTATE_TYPE)*8 ? -1 := 0]; Why limit ourselves to a scalar when "qemu/bitops.h" and "qemu/bitmap.h" provide functions for arbitrary length bitmaps? See DECLARE_BITMAP(), set_bit(), clear_bit(), test_bit(). > @@ -61,7 +69,7 @@ static inline bool trace_event_get_state_static(TraceEv= ent *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[i= d]; > + 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? > +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[id]; > + trace_events_dstate[id] =3D state; > + } > +} 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. 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. > + > +void trace_event_set_cpu_state_dynamic(CPUState *cpu, > + TraceEvent *ev, bool state) > +{ > + TraceEventID id; > + TraceEventVCPUID cpu_id; > + TRACE_VCPU_DSTATE_TYPE bit; > + bool state_pre; > + assert(cpu !=3D NULL); > + assert(ev !=3D NULL); > + assert(trace_event_get_state_static(ev)); > + assert(trace_event_get_cpu_id(ev) !=3D trace_event_cpu_count()); > + id =3D trace_event_get_id(ev); > + cpu_id =3D trace_event_get_cpu_id(ev); > + bit =3D ((TRACE_VCPU_DSTATE_TYPE)1) << cpu_id; > + state_pre =3D cpu->trace_dstate & bit; > + if ((state_pre =3D=3D 0) !=3D (state =3D=3D 0)) { Simpler expression: if (state_pre !=3D state) > @@ -21,7 +21,10 @@ > #include "qemu/error-report.h" > =20 > int trace_events_enabled_count; > -bool trace_events_dstate[TRACE_EVENT_COUNT]; > +/* With the 'vcpu' property, counts how many vCPUs have it enabled. */ > +size_t trace_events_dstate[TRACE_EVENT_COUNT]; The number of cpus has type int (see CPUState *qemu_get_cpu(int index)). Why did you choose size_t? --n8g4imXOkfNTN/H1 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJXWVwbAAoJEJykq7OBq3PIDIgH/RvzneleP14rBEMF6MpvhLlp rGTEyxVOUfAP2zwNeOEfUoocfsSJNFq+JGrqxDcH9pVMhooIY7cdIh4r4xvyL6Da /jzl2FmTxCD2oNe2UK4ztUU3T6i8E2mCVA1xaFfnZihJjEHAUton6j/O2ZaleV7e owQEigrvVp0/lmS0P3r5NzlRbSKh61MQZGY8A5B1gE0ARZYcr+vOQrEhyyHTNhcc dGkkUgkxhzOzJ9k9n6Gz+RkMOBmeaRM1/mziVPqQpXHkS64smWtdnaaao2y5Ii2M cajc0TzdpJXcMry/Rvh3R3sh2srRlhlx6CEqBSk9rHVEQXaZ8nfAcW3hTR8p608= =waJe -----END PGP SIGNATURE----- --n8g4imXOkfNTN/H1--