From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34090) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bB0lz-0004Rh-Pr for qemu-devel@nongnu.org; Thu, 09 Jun 2016 10:17:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bB0lv-0001og-Fw for qemu-devel@nongnu.org; Thu, 09 Jun 2016 10:17:26 -0400 Received: from roura.ac.upc.edu ([147.83.33.10]:48191 helo=roura.ac.upc.es) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bB0lv-0001oZ-2z for qemu-devel@nongnu.org; Thu, 09 Jun 2016 10:17:23 -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> Date: Thu, 09 Jun 2016 16:17:11 +0200 In-Reply-To: <20160609120756.GA15369@stefanha-x1.localdomain> (Stefan Hajnoczi's message of "Thu, 9 Jun 2016 13:07:56 +0100") Message-ID: <871t4630rc.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: Eduardo Habkost , Riku Voipio , qemu-devel@nongnu.org, Blue Swirl , Stefan Hajnoczi , Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= Stefan Hajnoczi writes: > On Thu, Feb 25, 2016 at 04:03:06PM +0100, Llu=C3=ADs 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(). Oh, I wasn't aware of the bitmap functions. Nice catch. >> @@ -61,7 +69,7 @@ static inline bool trace_event_get_state_static(TraceE= vent *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 type= s are not boolean (for a maybe-false sense of future-proofing). I can leave it as= it was if it bothers you. >> +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. Yes, it's a bit messy. I'll add some proper documentation about how this is interpreted. >> + >> +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? It just sounds proper to me to use size_t, since the state can never be neg= ative (it's either interpreted as a boolean or as an unsigned counter, depending = on the "vcpu" property). Thanks, Lluis