Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Wed, Mar 16, 2016 at 06:27:48PM +0000, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > On Tue, Mar 15, 2016 at 01:56:47PM +0000, Dr. David Alan Gilbert wrote:
> > > > > This would put trace_foo() in generated-tracers-virtio-blk.h and
> > > > > trace_bar() in generated-tracers-memory.h.  Source files using tracing
> > > > > would need to include headers for relevant sections.
> > > > > 
> > > > > This way we can narrow the scope of tracing headers and prevent global
> > > > > recompilation.
> > > > > 
> > > > > The fly in the ointment is that trace/control.h defines enum
> > > > > TraceEventID, a global numbering of all trace events.  The enum is used
> > > > > in trace/contro.h APIs and also in the simpletrace file format.
> > > > > 
> > > > > If ./trace-event is modified the numbering of trace events could change.
> > > > > This would require global recompilation :(.
> > > > > 
> > > > > So in order to avoid global recompilation we need to eliminate enum
> > > > > TraceEventID.  Perhaps it's possible to use TraceEvent* instead of
> > > > > TraceEventID.  For the simpletrace backend we would continue to use a
> > > > > global ordering but that only affects generated-tracers.c and not header
> > > > > files (thankfully!).
> > > > 
> > > > I think the hardest part is going to be understanding all of the different
> > > > tracers and the things they need to know; like that thing about simpletrace
> > > > needing the single ordering;  I don't know what the other backends need
> > > > but I bet they've each got their own surprise.
> > > > 
> > > > (And yes, I'd love to split them up - I tend to use traceing quite a bit
> > > > in migration now and trying to do a bisect over a big migration patch
> > > > series takes ages mostly because of the trace.h changes)
> > > 
> > > Yes, this is the hard part.  The simpletrace.py pretty-printing script
> > > takes a trace-events file as input.  I suppose it could alphabetically
> > > sort the trace-events filenames from the command-line to produce a
> > > global ordering of trace event IDs.  Of course that doesn't work
> > > properly if the script is invoked with relative paths from a
> > > sub-directory...
> > > 
> > > Perhaps we need a new script during the QEMU build process that produces
> > > a trace-event-ids.csv file.  Then simpletrace.py could take that instead
> > > of processing ./trace-events.
> > 
> > Well that sounds easy - but is it easy to avoid using those fixed IDs
> > in any of the trace.h functions that are included everywhere (without
> > making them more expensive).
> 
> How about this:
> 
> Instead of using TraceEventID in tracing APIs, pass the TraceEvent
> pointer directly.  The tracetool.py code generator should produce
> "extern TraceEvent trace_event_foo;" definitions in headers so that the
> users have direct access to the TraceEvents.

OK, so I see TraceEvent has a TraceEventID field; so yes that works easily;
it turns out to be a little more expensive though since what was a:

   trace_events_dstate[id]

is now
   trace_events_dstate[te->id]

But hang on, what's the 'sstate' in TraceEvent; do we actually need two
state fields if we're passing a TraceEvent pointer around?

Dave

> Stefan


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK