From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41140) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCNwa-0007sZ-8Y for qemu-devel@nongnu.org; Mon, 13 Jun 2016 05:14:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCNwW-00041z-2i for qemu-devel@nongnu.org; Mon, 13 Jun 2016 05:14:04 -0400 Received: from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241]:33901) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCNwV-00041Z-AP for qemu-devel@nongnu.org; Mon, 13 Jun 2016 05:14:00 -0400 Received: by mail-wm0-x241.google.com with SMTP id n184so13204975wmn.1 for ; Mon, 13 Jun 2016 02:13:59 -0700 (PDT) Sender: Paolo Bonzini References: <145641255678.30097.2919142707547689749.stgit@localhost> <145641258612.30097.7127731954660712163.stgit@localhost> From: Paolo Bonzini Message-ID: <30b46b30-fbc0-31a4-6210-657b205c121f@redhat.com> Date: Mon, 13 Jun 2016 11:13:52 +0200 MIME-Version: 1.0 In-Reply-To: <145641258612.30097.7127731954660712163.stgit@localhost> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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: =?UTF-8?Q?Llu=c3=ads_Vilanova?= , qemu-devel@nongnu.org Cc: Eduardo Habkost , Riku Voipio , Blue Swirl , Stefan Hajnoczi , =?UTF-8?Q?Andreas_F=c3=a4rber?= 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) != 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. On 25/02/2016 16:03, Lluís Vilanova wrote: > +static inline bool trace_event_get_cpu_state_dynamic(CPUState *cpu, > + TraceEvent *ev) > { > - int id = trace_event_get_id(ev); > + TraceEventVCPUID id; > + assert(cpu != NULL); > assert(ev != NULL); Please do not add more "!= NULL" asserts. In fact, we should remove the others; in this case the ev != NULL assertion is particularly pointless since it comes after a dereference. > assert(trace_event_get_state_static(ev)); > - trace_events_enabled_count += state - trace_events_dstate[id]; > - trace_events_dstate[id] = state; > + assert(trace_event_get_cpu_id(ev) != trace_event_cpu_count()); > + id = 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); > } > > #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 events. > + * > + * Copyright (C) 2014-2016 Lluís Vilanova > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * 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 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. 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. Paolo