From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36220) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmLGm-0004mX-Mu for qemu-devel@nongnu.org; Tue, 20 Sep 2016 09:39:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmLGg-0007l7-Fi for qemu-devel@nongnu.org; Tue, 20 Sep 2016 09:39:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46360) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmLGb-0007Ct-MT for qemu-devel@nongnu.org; Tue, 20 Sep 2016 09:39:26 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D386B13590 for ; Tue, 20 Sep 2016 13:29:19 +0000 (UTC) Date: Tue, 20 Sep 2016 14:29:16 +0100 From: "Daniel P. Berrange" Message-ID: <20160920132916.GK25490@redhat.com> Reply-To: "Daniel P. Berrange" References: <1474296549-29171-1-git-send-email-berrange@redhat.com> <1474296549-29171-9-git-send-email-berrange@redhat.com> <87k2e77pbo.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87k2e77pbo.fsf@fimbulvetr.bsc.es> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 08/18] trace: remove the TraceEventID and TraceEventVCPUID enums List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, Stefan Hajnoczi On Mon, Sep 19, 2016 at 07:48:59PM +0200, Llu=C3=ADs Vilanova wrote: > Daniel P Berrange writes: >=20 > > The TraceEventID and TraceEventVCPUID enums constants are > > no longer actually used for anything critical. >=20 > > The TRACE_EVENT_COUNT limit is used to determine the size > > of the TraceEvents array, and can be removed if we just > > NULL terminate the array instead. >=20 > > The TRACE_EVENT_VCPU_COUNT limit is used as a magic value >=20 > It's TRACE_VCPU_EVENT_COUNT. >=20 >=20 > > for marking non-vCPU events, and also for declaring the > > size of the trace dstate mask in the CPUState struct. > > The former usage can be replaced by a dedicated constant > > TRACE_EVENT_VCPU_NONE, defined as (uint32_t)-1. For the > > latter usage, we can simply define a constant for the > > number of VCPUs, avoiding the need for the full enum. >=20 > > The only other usages of the enum values can be replaced > > by accesing the id/vcpu_id fields via the named TraceEvent > > structs. >=20 > > Signed-off-by: Daniel P. Berrange > > --- > > scripts/tracetool/backend/simple.py | 2 +- > > scripts/tracetool/format/events_c.py | 16 +++++++++++----- > > scripts/tracetool/format/events_h.py | 18 +++--------------- > > scripts/tracetool/format/h.py | 3 +-- > > trace/control-internal.h | 16 ++++++++-------- > > trace/control-target.c | 2 +- > > trace/control.c | 2 +- > > trace/control.h | 30 +++++++-------------------= ---- > > trace/event-internal.h | 6 ++++++ > > trace/simple.c | 6 +++--- > > trace/simple.h | 2 +- > > 11 files changed, 43 insertions(+), 60 deletions(-) >=20 > > diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/= backend/simple.py > > index 1bccada..971bd97 100644 > > --- a/scripts/tracetool/backend/simple.py > > +++ b/scripts/tracetool/backend/simple.py > > @@ -80,7 +80,7 @@ def generate_c(event): > > ' return;', > > ' }', > > '', > > - ' if (trace_record_start(&rec, %(event_id)s, %(size_str)s= )) {', > > + ' if (trace_record_start(&rec, %(event_id)s_EV.id, %(size= _str)s)) {', >=20 > You should use Event.api() here too. >=20 >=20 > > ' return; /* Trace Buffer Full, Event Dropped ! */', > > ' }', > > cond=3Dcond, > > diff --git a/scripts/tracetool/format/events_c.py b/scripts/tracetool= /format/events_c.py > > index 5f67a7b..07665df 100644 > > --- a/scripts/tracetool/format/events_c.py > > +++ b/scripts/tracetool/format/events_c.py > > @@ -28,11 +28,16 @@ def generate(events, backend): > > for e in events: > > out('uint16_t %s;' % e.api(e.QEMU_DSTATE)) > =20 > > + next_id =3D 0 > > + next_vcpu_id =3D 0 > > for e in events: > > + id =3D next_id > > + next_id +=3D 1 > > if "vcpu" in e.properties: > > - vcpu_id =3D "TRACE_VCPU_" + e.name.upper() > > + vcpu_id =3D next_vcpu_id > > + next_vcpu_id +=3D 1 > > else: > > - vcpu_id =3D "TRACE_VCPU_EVENT_COUNT" > > + vcpu_id =3D "TRACE_VCPU_EVENT_NONE" > > out('TraceEvent %(event)s =3D {', > > ' .id =3D %(id)s,', > > ' .vcpu_id =3D %(vcpu_id)s,', > > @@ -41,17 +46,18 @@ def generate(events, backend): > > ' .dstate =3D &%(dstate)s ', > > '};', > > event =3D "TRACE_" + e.name.upper() + "_EV", > > - id =3D "TRACE_" + e.name.upper(), > > + id =3D id, > > vcpu_id =3D vcpu_id, > > name =3D e.name, > > sstate =3D "TRACE_%s_ENABLED" % e.name.upper(), > > dstate =3D e.api(e.QEMU_DSTATE)) > =20 > > - out('TraceEvent *trace_events[TRACE_EVENT_COUNT] =3D {') > > + out('TraceEvent *trace_events[] =3D {') > =20 > > for e in events: > > out('&%(event)s,', > > event =3D "TRACE_" + e.name.upper() + "_EV") > =20 > > - out('};', > > + out(' NULL,', > > + '};', > > '') > > diff --git a/scripts/tracetool/format/events_h.py b/scripts/tracetool= /format/events_h.py > > index 1478e4c..35cbc91 100644 > > --- a/scripts/tracetool/format/events_h.py > > +++ b/scripts/tracetool/format/events_h.py > > @@ -28,27 +28,15 @@ def generate(events, backend): > > for e in events: > > out('extern TraceEvent TRACE_%s_EV;' % e.name.upper()) > =20 > > - # event identifiers > > - out('typedef enum {') > > - > > - for e in events: > > - out(' TRACE_%s,' % e.name.upper()) > > - > > - out(' TRACE_EVENT_COUNT', > > - '} TraceEventID;') > > - > > for e in events: > > out('extern uint16_t %s;' % e.api(e.QEMU_DSTATE)) > =20 > > - # per-vCPU event identifiers > > - out('typedef enum {') > > - > > + numvcpu =3D 0 > > for e in events: > > if "vcpu" in e.properties: > > - out(' TRACE_VCPU_%s,' % e.name.upper()) > > + numvcpu +=3D 1 > =20 > > - out(' TRACE_VCPU_EVENT_COUNT', > > - '} TraceEventVCPUID;') > > + out("#define TRACE_VCPU_EVENT_COUNT %d" % numvcpu) > =20 > > # static state > > for e in events: > > diff --git a/scripts/tracetool/format/h.py b/scripts/tracetool/format= /h.py > > index 3763e9a..64a6680 100644 > > --- a/scripts/tracetool/format/h.py > > +++ b/scripts/tracetool/format/h.py > > @@ -32,8 +32,7 @@ def generate(events, backend): > > if "vcpu" in e.properties: > > trace_cpu =3D next(iter(e.args))[1] > > cond =3D "trace_event_get_vcpu_state(%(cpu)s,"\ > > - " TRACE_%(id)s,"\ > > - " TRACE_VCPU_%(id)s)"\ > > + " TRACE_%(id)s)"\ > > % dict( > > cpu=3Dtrace_cpu, > > id=3De.name.upper()) > > diff --git a/trace/control-internal.h b/trace/control-internal.h > > index 52b6b72..c5bb53d 100644 > > --- a/trace/control-internal.h > > +++ b/trace/control-internal.h > > @@ -25,20 +25,20 @@ static inline bool trace_event_is_pattern(const c= har *str) > > return strchr(str, '*') !=3D NULL; > > } > =20 > > -static inline TraceEventID trace_event_get_id(TraceEvent *ev) > > +static inline uint32_t trace_event_get_id(TraceEvent *ev) >=20 > Return should be size_t here too if the iterator patch uses size_t, rig= ht? No, see my response in the previous patch for why ID values are not the same as iterator event array indexes. > > @@ -119,19 +106,16 @@ static const char * trace_event_get_name(TraceE= vent *ev); > > /** > > * trace_event_get_vcpu_state: > > * @vcpu: Target vCPU. > > - * @id: Event identifier (TraceEventID). > > - * @vcpu_id: Per-vCPU event identifier (TraceEventVCPUID). > > + * @id: Event identifier name. > > * > > * Get the tracing state of an event (both static and dynamic) for t= he given > > * vCPU. > > * > > * If the event has the disabled property, the check will have no pe= rformance > > * impact. > > - * > > - * As a down side, you must always use an immediate #TraceEventID va= lue. > > */ > > -#define trace_event_get_vcpu_state(vcpu, id, vcpu_id) = \ > > - ((id ##_ENABLED) && trace_event_get_vcpu_state_dynamic_by_vcpu_i= d(vcpu, vcpu_id)) > > +#define trace_event_get_vcpu_state(vcpu, id) = \ > > + ((id ##_ENABLED) && trace_event_get_vcpu_state_dynamic_by_vcpu_i= d(vcpu, id ## _EV.vcpu_id)) >=20 > You should directly use the vCPU id value to keep performance at the sa= me > level. >=20 > For that, I'd keep the TraceEventVCPUID enum, or some define for them. = Note that > changes would only affect files including the tracing headers for event= s with > the vcpu property, which will typically be limited to a single header (= with the > "guest_" events for now). >=20 > You can pre-calculate the .id and .vcpu_id values when reading all even= ts (e.g., > calculated in tracetool._read_events() and stored in the Event class). No, this entire series is about explicitly *NOT* having to pre-calculate event ID values at time of generation, as that inherantly involves re-generating all code when ID values change in any single trace-events file and I want to avoid the assumptions that all vcpu events are in the same file that my earlier posting had. If the performance problem is genuinely a problem we can switch to having a 'int TRACE_EVENT_XXXXX_VCPU_ID' variable per event and then having 'TraceEvent' struct contain a 'int *vcpu_id', but I think we should only do that if there's a measurable performance problem demonstrated. Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|