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

[Lluís Vilanova]

Blue Swirl writes:

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

Well, all the events I've seen use values from the caller, so there's no
performance impact there. But of course the developer must have that in mind
when adding an event (that's why I wrote the original comment about performance
in tracing.txt).


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

Sounds a little bit over-engineered to me, but having the TRACE_*_ENABLED
define and a new 'trace_event_get_state' would make for the most complete
solution.

Note that this and 'trace_event_get_state' should be changed to work with an
enum/define instead of the current string (e.g., adding a 'trace_event_t
trace_event_num(char* name)').

Unfortunately, the implementation of 'trace_event_get_state' would be
backend-specific, and from what I recall it is not possible on some backends to
easily get the state of an event with just a couple of lines of code.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth