From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58177) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bkSw1-0008T3-8n for qemu-devel@nongnu.org; Thu, 15 Sep 2016 05:26:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bkSvy-0000cN-V5 for qemu-devel@nongnu.org; Thu, 15 Sep 2016 05:26:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33290) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bkSvy-0000cF-LR for qemu-devel@nongnu.org; Thu, 15 Sep 2016 05:26:18 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (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 2DE825455E for ; Thu, 15 Sep 2016 09:26:18 +0000 (UTC) Date: Thu, 15 Sep 2016 10:26:15 +0100 From: "Daniel P. Berrange" Message-ID: <20160915092615.GF26068@redhat.com> Reply-To: "Daniel P. Berrange" References: <1473872922-23449-1-git-send-email-berrange@redhat.com> <1473872922-23449-6-git-send-email-berrange@redhat.com> <87vaxyukq9.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87vaxyukq9.fsf@fimbulvetr.bsc.es> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 5/6] trace: remove use of event ID enums from APIs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, Stefan Hajnoczi , Paolo Bonzini On Thu, Sep 15, 2016 at 01:26:06AM +0200, Llu=C3=ADs Vilanova wrote: > Daniel P Berrange writes: >=20 > > Since there will shortly be multiple event groups allowed, > > we can no longer use the TraceEventID and TraceEventVCPUID > > enums in the trace control APIs. There will in fact be > > multiple distinct enums, and the enum values will only be > > required to be unique per group. >=20 > This patch serves no purpose without the event group patches. >=20 > Also, AFAIR TraceEventVCPUID still needs to be a flat space (they're al= l used as > bitmask indexes), so keeping the enum won't lose any re-compilation ben= efit. > > And without wanting to sound like a broken record, you can make the > "TRACE_${EVENTNAME}" IDs be global Event* variables (statically initial= ized in > "trace/generated-events.c"). That still allows using their names in the= macros, > avoids having a (two-level) tree of events, and eliminates the need for= the > Event::id member (and the trace_event_get_id() function). Regardless of whether its used in the public API, we still need to be able to assign a unique 32bit event ID to every event, because simpletrace currently needs to output that in its trace files. That said, i have been wondering if people are very tied to the current simple trace output format ? My ideal solution would be for us to dynamically assign id values to trace events when QEMU starts up. If we did this, hen in the simpletrace output file, we would have to insert a new record type which records an (event name, ID) value mapping, the first time any individual event type is emitted. Thus when simpletrace.py comes to load the trace data, instead of using the event ID to lookup the event directly, it would use the event ID to get the name from the trace data, and then lookup the event based on name. > [...] > > diff --git a/trace/simple.c b/trace/simple.c > > index 2f09daf..6e8013c 100644 > > --- a/trace/simple.c > > +++ b/trace/simple.c > > @@ -18,7 +18,7 @@ > > #include "trace/simple.h" > =20 > > /** Trace file header event ID */ > > -#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with Tr= aceEventIDs */ > > +#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with ev= ent IDs */ > =20 > > /** Trace file magic number */ > > #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL > > @@ -58,7 +58,7 @@ static char *trace_file_name; > =20 > > /* * Trace buffer entry */ > > typedef struct { > > - uint64_t event; /* TraceEventID */ > > + uint64_t event; /* event ID */ > > uint64_t timestamp_ns; > > uint32_t length; /* in bytes */ > > uint32_t pid; > > @@ -202,7 +202,7 @@ void trace_record_write_str(TraceBufferRecord *re= c, const char *s, uint32_t slen > rec-> rec_off =3D write_to_buffer(rec->rec_off, (void*)s, slen); > > } > =20 > > -int trace_record_start(TraceBufferRecord *rec, TraceEventID event, s= ize_t datasize) > > +int trace_record_start(TraceBufferRecord *rec, uint32_t event, size_= t datasize) > > { > > unsigned int idx, rec_off, old_idx, new_idx; > > uint32_t rec_len =3D sizeof(TraceRecord) + datasize; > > diff --git a/trace/simple.h b/trace/simple.h > > index 1e7de45..17ce472 100644 > > --- a/trace/simple.h > > +++ b/trace/simple.h > > @@ -33,7 +33,7 @@ typedef struct { > > * > > * @arglen number of bytes required for arguments > > */ > > -int trace_record_start(TraceBufferRecord *rec, TraceEventID id, size= _t arglen); > > +int trace_record_start(TraceBufferRecord *rec, uint32_t id, size_t a= rglen); > =20 > > /** > > * Append a 64-bit argument to a trace record >=20 > Not incorrect, but it's weird that the simple backend emits 64-bit iden= tifiers > while QEMU uses 32-bit ones. The trace_record_start method *is* given a 32-bit id value not a 64-bit v= alue. That comment you're quoting here is nothing todo with the ID values, it i= s about writing 64-bit parameter values. 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 :|