[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event f
From: |
Lluís Vilanova |
Subject: |
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files |
Date: |
Fri, 09 Sep 2016 15:16:50 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Daniel P Berrange writes:
> On Fri, Sep 09, 2016 at 01:03:51PM +0200, Lluís Vilanova wrote:
>> 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.
> The various _DSTATE variables are still arbitrarily scattered in
> memory, as opposed to in a contiguous cache friendly array, which
> is one of the goals of Paolo's original change.
> That said, I'm unclear on how much of the performance win from
> Paolo's change came from eliminating the struct field de-reference,
> vs having the contiguous array. I'm guessing most of the win is
> from the former, the latter only being important if we hit multiple
> related tracepoints on close succession.
The latter can also be achieved by packing them all together in a single
section, but I don't know if that's acceptable within portable QEMU code. For
example:
bool ___TRACE_EVENTNAME_DSTATE __attribute__((section("dstate_array")))
Cheers,
Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files, Daniel P. Berrange, 2016/09/13