Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files

[Lluís Vilanova]

Daniel P Berrange writes:

> On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>> 
>> > I previously split the global trace-events file up into one file
>> > per-subdirectory to avoid merge conflict hell.
>> [...]
>> 
>> Sorry, I could not find the message where the infrastructure is modified to
>> provide this. But I think there's a more efficient way to provide modular
>> auto-generated tracing code without the hierarchical indexing you proposed.

> [snip]

>> struct TraceEvent ___trace_events[] = {
>> {
>> .name = "eventname",
>> .sstate = 1,
>> .dstate = ___trace_eventname_dstate;
>> }
>> }
>> 
>> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];

> Life would be simpler if we had the 'bool dstate' as part of the
> TraceEvent struct, but doing so would essentially be reverting this
> previous change:

>   commit 585ec7273e6fdab902b2128bc6c2a8136aafef04
>   Author: Paolo Bonzini <address@hidden>
>   Date:   Wed Oct 28 07:06:27 2015 +0100

>     trace: track enabled events in a separate array
    
>     This is more cache friendly on the fast path, where we already have
>     the event id available.

> I asked Paolo about this previously and he indicated it was a notable
> performance improvement, so we can't put dstate back into the TraceEvent
> struct :-(

Sorry, there was a typo in my example code that led to this misunderstanding.

I meant this for the global auto-generated .c:

  struct TraceEvent {
      /* ... */
      bool *dstate;
  };

  bool ___TRACE_EVENTNAME_DSTATE;

  struct TraceEvent ___trace_events[] = {
      {
          .name = "eventname",
          .sstate = 1,
          .dstate = &___TRACE_EVENTNAME_DSTATE;
      }
  }

  TraceEvent *TRACE_EVENTNAME = &___trace_events[...];


If you look at the modified macro I pasted for "trace/control-internal.h":


  #define trace_event_get_state_dynamic_by_id(id) \
      (unlikely(trace_events_enabled_count) && \
       (___ ## id ## _DSTATE))

We're retaining the fast-path performance of the seggregated boolean dstate
array, while the event descriptor contains a pointer to the per-event dstate
global boolean variable in case you want to get/set the dstate based on an event
pointer.

Cheers,
  Lluis