Re: [Qemu-devel] [PATCH] trace: Provide a per-event status define for conditional compilation

[Blue Swirl]

2011/9/25 Lluís Vilanova <address@hidden>:
> Blue Swirl writes:
>
>> 2011/9/21 Lluís Vilanova <address@hidden>:
> [...]
>>> +== Trace event properties ==
>>> +
>>> +Each event in the "trace-events" file can be prefixed with a 
>>> space-separated
>>> +list of zero or more of the following event properties.
>>> +
>>> +=== "disable" ===
>>> +
>>> +If a specific trace event is going to be invoked a huge number of times, 
>>> this
>>> +might have a noticeable performance impact even when the event is
>>> +programmatically disabled.
>>> +
>>> +In this case you should declare such event with the "disable" property. 
>>> This
>>> +will effectively disable the event at compile time (by using the "nop" 
>>> backend),
>>> +thus having no performance impact at all on regular builds (i.e., unless 
>>> you
>>> +edit the "trace-events" file).
>
>> Nack, by default events should have no performance impact.
>
> Sure, but some events can have an impact. Then it is advisable to ship QEMU 
> with
> the "disable" property on these events, and (if necesary) use the
> trace_*_enabled (or TRACE_*_ENABLED) define to eliminate all possible sources 
> of
> performance degradation from that event.

Maybe it was not so good idea to remove the 'disable' property from
all trace-events.

> For example, the patches I sent long ago for instrumenting guest code using 
> TCG
> now use QEMU's tracing infrastructure. These events are disabled by default, 
> but
> some of them perform some computations to produce their arguments. E.g., one 
> of
> the events has code for computing the set of used/defined registers for each
> instruction, which is not for free.
>
>
> [...]
>>> diff --git a/scripts/tracetool b/scripts/tracetool
>>> index 4c9951d..97f9f1b 100755
>>> --- a/scripts/tracetool
>>> +++ b/scripts/tracetool
>>> @@ -519,7 +519,7 @@ linetostap_end_dtrace()
>>>  # Process stdin by calling begin, line, and end functions for the backend
>>>  convert()
>>>  {
>>> -    local begin process_line end str disable
>>> +    local begin process_line end str name enabled
>>>     begin="lineto$1_begin_$backend"
>>>     process_line="lineto$1_$backend"
>>>     end="lineto$1_end_$backend"
>>> @@ -534,8 +534,14 @@ convert()
>>>         # Process the line.  The nop backend handles disabled lines.
>>>         if has_property "$str" "disable"; then
>>>             "lineto$1_nop" "$str"
>>> +            enabled=0
>>>         else
>>>             "$process_line" "$str"
>>> +            enabled=1
>
>> Instead of '1', couldn't this be a function that checks if the trace
>> point is enabled during run time (from monitor etc.)?
>
> What you're proposing is the counterpart of 'trace_event_set_state' (in
> "trace/control.h"). This would miss the whole point of this patch, which is to
> be able to ship QEMU with relatively costly tracing events whose performance
> impact can be completely eliminated at compile time.

Not exactly, for '0' case, the code would be disabled.

For the trace_event_get_state() case, there would be a call to check
if the event is currently enabled. Then it would be possible to
control the additional code using monitor commands.

Case '1' (like you propose) could also make sense in some cases, then
the code would be always enabled.

Even more complex way could be that the distinction between '1' and
dynamical check could be determined by the caller (if that is the
right place). TRACE_*_ENABLED could be #defined as 0 or 1.
TRACE_*_CHECK_ENABLED() could perform a dynamical check or still
return 1 or 0 if configured to do so.