Re: [Qemu-devel] [PATCH v3 08/18] trace: remove the TraceEventID and TraceEventVCPUID enums

["Daniel P. Berrange" <berrange@redhat.com>]

On Mon, Sep 19, 2016 at 07:48:59PM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
> 
> > The TraceEventID and TraceEventVCPUID enums constants are
> > no longer actually used for anything critical.
> 
> > 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.
> 
> > The TRACE_EVENT_VCPU_COUNT limit is used as a magic value
> 
> It's TRACE_VCPU_EVENT_COUNT.
> 
> 
> > 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.
> 
> > The only other usages of the enum values can be replaced
> > by accesing the id/vcpu_id fields via the named TraceEvent
> > structs.
> 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  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(-)
> 
> > 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)) {',
> 
> You should use Event.api() here too.
> 
> 
> >          '        return; /* Trace Buffer Full, Event Dropped ! */',
> >          '    }',
> >          cond=cond,
> > 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))
>  
> > +    next_id = 0
> > +    next_vcpu_id = 0
> >      for e in events:
> > +        id = next_id
> > +        next_id += 1
> >          if "vcpu" in e.properties:
> > -            vcpu_id = "TRACE_VCPU_" + e.name.upper()
> > +            vcpu_id = next_vcpu_id
> > +            next_vcpu_id += 1
> >          else:
> > -            vcpu_id = "TRACE_VCPU_EVENT_COUNT"
> > +            vcpu_id = "TRACE_VCPU_EVENT_NONE"
> >          out('TraceEvent %(event)s = {',
> >              '  .id = %(id)s,',
> >              '  .vcpu_id = %(vcpu_id)s,',
> > @@ -41,17 +46,18 @@ def generate(events, backend):
> >              '  .dstate = &%(dstate)s ',
> >              '};',
> >              event = "TRACE_" + e.name.upper() + "_EV",
> > -            id = "TRACE_" + e.name.upper(),
> > +            id = id,
> >              vcpu_id = vcpu_id,
> >              name = e.name,
> >              sstate = "TRACE_%s_ENABLED" % e.name.upper(),
> >              dstate = e.api(e.QEMU_DSTATE))
>  
> > -    out('TraceEvent *trace_events[TRACE_EVENT_COUNT] = {')
> > +    out('TraceEvent *trace_events[] = {')
>  
> >      for e in events:
> >          out('&%(event)s,',
> >              event = "TRACE_" + e.name.upper() + "_EV")
>  
> > -    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())
>  
> > -    # 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))
>  
> > -    # per-vCPU event identifiers
> > -    out('typedef enum {')
> > -
> > +    numvcpu = 0
> >      for e in events:
> >          if "vcpu" in e.properties:
> > -            out('    TRACE_VCPU_%s,' % e.name.upper())
> > +            numvcpu += 1
>  
> > -    out('    TRACE_VCPU_EVENT_COUNT',
> > -        '} TraceEventVCPUID;')
> > +    out("#define TRACE_VCPU_EVENT_COUNT %d" % numvcpu)
>  
> >      # 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 = next(iter(e.args))[1]
> >              cond = "trace_event_get_vcpu_state(%(cpu)s,"\
> > -                   " TRACE_%(id)s,"\
> > -                   " TRACE_VCPU_%(id)s)"\
> > +                   " TRACE_%(id)s)"\
> >                     % dict(
> >                         cpu=trace_cpu,
> >                         id=e.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 char *str)
> >      return strchr(str, '*') != NULL;
> >  }
>  
> > -static inline TraceEventID trace_event_get_id(TraceEvent *ev)
> > +static inline uint32_t trace_event_get_id(TraceEvent *ev)
> 
> Return should be size_t here too if the iterator patch uses size_t, right?

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(TraceEvent *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 the given
> >   * vCPU.
> >   *
> >   * If the event has the disabled property, the check will have no performance
> >   * impact.
> > - *
> > - * As a down side, you must always use an immediate #TraceEventID value.
> >   */
> > -#define trace_event_get_vcpu_state(vcpu, id, vcpu_id)                   \
> > -    ((id ##_ENABLED) && trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, vcpu_id))
> > +#define trace_event_get_vcpu_state(vcpu, id)                            \
> > +    ((id ##_ENABLED) && trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, id ## _EV.vcpu_id))
> 
> You should directly use the vCPU id value to keep performance at the same
> level.
> 
> 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 events with
> the vcpu property, which will typically be limited to a single header (with the
> "guest_" events for now).
> 
> You can pre-calculate the .id and .vcpu_id values when reading all events (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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|