Re: [Qemu-devel] [PATCH v2 4/6] trace: remove global 'uint16 dstate[]' array

[Daniel P. Berrange]

On Thu, Sep 15, 2016 at 12:56:57AM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
> 
> > Instead of having a global dstate array, declare a single
> > 'uint16 TRACE_${EVENT_NAME}_DSTATE' variable for each
> > trace event. Record a pointer to this variable in the
> > TraceEvent struct too.
> 
> > By turning trace_event_get_state_dynamic_by_id into a
> > macro, this still hits the fast path, and cache affinity
> > is ensured by declaring all the uint16 vars adjacent to
> > each other.
> 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  scripts/tracetool/format/events_c.py |  6 +++++-
> >  scripts/tracetool/format/events_h.py |  3 +++
> >  stubs/trace-control.c                |  9 ++++-----
> >  trace/control-internal.h             | 14 ++++----------
> >  trace/control-target.c               | 20 ++++++++------------
> >  trace/control.c                      | 11 ++---------
> >  trace/event-internal.h               |  6 ++++++
> >  7 files changed, 32 insertions(+), 37 deletions(-)
> 
> > diff --git a/scripts/tracetool/format/events_c.py 
> > b/scripts/tracetool/format/events_c.py
> > index 4012063..a2f457f 100644
> > --- a/scripts/tracetool/format/events_c.py
> > +++ b/scripts/tracetool/format/events_c.py
> > @@ -25,6 +25,9 @@ def generate(events, backend):
> >          '#include "trace/control.h"',
> >          '')
>  
> > +    for e in events:
> > +        out('uint16_t TRACE_%s_DSTATE;' % e.name.upper())
> > +
> >      out('TraceEvent trace_events[TRACE_EVENT_COUNT] = {')
>  
> >      for e in events:
> 
> I would emit an "obviously non-public" variable name, like
> ___TRACE_%s_dstate. The "TRACE_%s" is only necesary for consistency with the
> "public name" and macro trickery on the fast path.
> 
> To make naming consistency easier to track, you can use Event.api(), which 
> needs
> only a small extension ("scripts/tracetool/__init__.py"):
> 
>     QEMU_TRACE               = "trace_%(name)s"
>     QEMU_TRACE_TCG           = QEMU_TRACE + "_tcg"
> 
>     QEMU_DSTATE              = "___TRACE_%(NAME)s_dstate"
> 
>     def api(self, fmt=None):
>         if fmt is None:
>             fmt = Event.QEMU_TRACE
>         return fmt % {"name": self.name, "NAME": self.name.upper()}
> 
> Then change all the places where you generate the dstate symbol name to
> something like:
> 
>     out('uint16_t ' + e.api(e.QEMU_DSTATE)) + ';')

Ah interesting, I hadn't noticed the Event.api() method.

> > @@ -54,18 +53,13 @@ static inline bool 
> > trace_event_get_state_static(TraceEvent *ev)
> >      return ev->sstate;
> >  }
>  
> > -static inline bool trace_event_get_state_dynamic_by_id(TraceEventID id)
> > -{
> > -    /* it's on fast path, avoid consistency checks (asserts) */
> > -    return unlikely(trace_events_enabled_count) && trace_events_dstate[id];
> > -}
> > +/* it's on fast path, avoid consistency checks (asserts) */
> > +#define trace_event_get_state_dynamic_by_id(id) \
> > +    (unlikely(trace_events_enabled_count) && id ## _DSTATE)
>  
> >  static inline bool trace_event_get_state_dynamic(TraceEvent *ev)
> >  {
> > -    TraceEventID id;
> > -    assert(trace_event_get_state_static(ev));
> > -    id = trace_event_get_id(ev);
> > -    return trace_event_get_state_dynamic_by_id(id);
> > +    return unlikely(trace_events_enabled_count) && *ev->dstate;
> 
> This one is not on the fast path, so there's no need for the first part of the
> AND (shouldn't hurt performance to keep it either).

Yep, I just figured it'd be nice to keep the two methods having the
same logic even if this second one isn't so performance critical.

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 :|