From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32879) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQms-0005xj-LX for qemu-devel@nongnu.org; Mon, 13 Jun 2016 08:16:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCQmo-00046R-Bk for qemu-devel@nongnu.org; Mon, 13 Jun 2016 08:16:13 -0400 Received: from roura.ac.upc.es ([147.83.33.10]:33188) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQmn-00046G-Vo for qemu-devel@nongnu.org; Mon, 13 Jun 2016 08:16:10 -0400 From: =?utf-8?Q?Llu=C3=ADs_Vilanova?= References: <145641255678.30097.2919142707547689749.stgit@localhost> <145641258612.30097.7127731954660712163.stgit@localhost> <30b46b30-fbc0-31a4-6210-657b205c121f@redhat.com> Date: Mon, 13 Jun 2016 14:15:58 +0200 In-Reply-To: <30b46b30-fbc0-31a4-6210-657b205c121f@redhat.com> (Paolo Bonzini's message of "Mon, 13 Jun 2016 11:13:52 +0200") Message-ID: <87lh29cmip.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: Paolo Bonzini Cc: qemu-devel@nongnu.org, Blue Swirl , Riku Voipio , Eduardo Habkost , Stefan Hajnoczi , Andreas =?utf-8?Q?F=C3=A4rber?= Paolo Bonzini writes: > First of all, a generic problem I see with your patches is that the > newly-introduced APIs are not providing a good abstraction. > If something is only used internally, as is the case for > trace_event_get_cpu_id, you don't need accessors. On the other hand, > when you have a repeated expression such as > trace_event_get_cpu_id(ev) !=3D trace_event_cpu_count() > then you need an API such as trace_event_is_vcpu(ev). > Another small ugliness is that you are using "vcpu" in trace-events and > in the generated files, but "cpu" in the C file. My suggestion is to > prefix functions with vcpu_trace_event if they refer to per-VCPU trace > events, and only use the VCPU ids in those functions. I'll fix these two. > On 25/02/2016 16:03, Llu=C3=ADs Vilanova wrote: >> +static inline bool trace_event_get_cpu_state_dynamic(CPUState *cpu, >> + TraceEvent *ev) >> { >> - int id =3D trace_event_get_id(ev); >> + TraceEventVCPUID id; >> + assert(cpu !=3D NULL); >> assert(ev !=3D NULL); > Please do not add more "!=3D NULL" asserts. In fact, we should remove the > others; in this case the ev !=3D NULL assertion is particularly pointless > since it comes after a dereference. And the asserts too. >> assert(trace_event_get_state_static(ev)); >> - trace_events_enabled_count +=3D state - trace_events_dstate[id]; >> - trace_events_dstate[id] =3D state; >> + assert(trace_event_get_cpu_id(ev) !=3D trace_event_cpu_count()); >> + id =3D trace_event_get_cpu_id(ev); >> + return trace_event_get_cpu_state_dynamic_by_cpu_id(cpu, id); > Based on the above suggestion regarding APIs: > assert(trace_event_is_vcpu(ev)); > return vcpu_trace_event_get_state_dynamic(cpu, ev->cpu_id); >> } >>=20 >> #endif /* TRACE__CONTROL_INTERNAL_H */ >> diff --git a/trace/control-stub.c b/trace/control-stub.c >> new file mode 100644 >> index 0000000..858b13e >> --- /dev/null >> +++ b/trace/control-stub.c >> @@ -0,0 +1,29 @@ >> +/* >> + * Interface for configuring and controlling the state of tracing event= s. >> + * >> + * Copyright (C) 2014-2016 Llu=C3=ADs Vilanova >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or l= ater. >> + * See the COPYING file in the top-level directory. >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "trace/control.h" > This is not a stub, in fact it has a bunch of duplicate code with > trace/control.c. > The actual stubs are trace_event_set_cpu_state_dynamic() (which I'd > rename to vcpu_trace_event_set_state_dynamic) and > vcpu_trace_event_set_state_dynamic_all that does a CPU_FOREACH. That follows the name convention of "sources compiled when there is not tar= get" (like some commandline tools). If there is another naming convention for su= ch cases, please let me know. > That said, I am skeptical about the benefit of the interfaces you are > adding. They add a lot of complication and overhead (especially > regarding the memory/cache overhead of the dstate array) without a clear > use case, in my opinion; all the processing you do at run-time is just > as well suited for later filtering. This should make tracing faster on the future with multi-threaded TCG, as w= ell as trace files much smaller if you're tracing something like memory accesses. Also, bear in mind this series was split from a much larger one f= or simplicity. The follow-up one provides much larger performance benefits by avoiding the generation of TCG code to call the tracing backend when a vCPU= is not traced. > I also believe that it's a bad idea to add "stuff" to trace-tool without > a user; unless I'm mistaken neither "vcpu" nor "tcg" trace events are > unused in qemu.git, and this means that the ~400 lines added in this > series are actually dead code. Events using these features are being added in parallel to this series, lik= e the recently accepted "guest_mem_before". I also have some events on my queue tracing bbl and instruction execution, as well as user-mode syscalls. Thanks, Lluis