[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 08/18] trace: remove the TraceEventID and Tra
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v3 08/18] trace: remove the TraceEventID and TraceEventVCPUID enums |
Date: |
Tue, 20 Sep 2016 14:29:16 +0100 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
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 <address@hidden>
> > ---
> > 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 :|
- [Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]' array, (continued)
- [Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]' array, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 06/18] trace: break circular dependancy in event-internal.h, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 12/18] trace: dynamically allocate trace_dstate in CPUState, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 08/18] trace: remove the TraceEventID and TraceEventVCPUID enums, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 09/18] trace: emit name <-> ID mapping in simpletrace header, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 15/18] trace: rename _read_events to read_events, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 07/18] trace: give each trace event a named TraceEvent struct, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 10/18] trace: don't abort qemu if ftrace can't be initialized, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 11/18] trace: provide mechanism for registering trace events, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 17/18] trace: pass trace-events to tracetool as a positional param, Daniel P. Berrange, 2016/09/19