Patchwork trace: Provide a per-event status define for conditional compilation

login
register
mail settings
Submitter Lluís Vilanova
Date Sept. 21, 2011, 6:38 p.m.
Message ID <20110921183838.9431.70792.stgit@ginnungagap.bsc.es>
Download mbox | patch
Permalink /patch/115836/
State New
Headers show

Comments

Lluís Vilanova - Sept. 21, 2011, 6:38 p.m.
Adds a 'trace_${name}_enabled' preprocessor define for each tracing event in
"trace.h".

This lets the user conditionally compile code with a relatively high execution
cost that is only necessary when producing the tracing information for an event
that is enabled.

Note that events using this define will probably have the "disable" property by
default, in order to avoid such costs on regular builds.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 docs/tracing.txt  |   46 ++++++++++++++++++++++++++++++++++++++++------
 scripts/tracetool |    8 +++++++-
 2 files changed, 47 insertions(+), 7 deletions(-)
Blue Swirl - Sept. 24, 2011, 7:51 a.m.
2011/9/21 Lluís Vilanova <vilanova@ac.upc.edu>:
> Adds a 'trace_${name}_enabled' preprocessor define for each tracing event in
> "trace.h".
>
> This lets the user conditionally compile code with a relatively high execution
> cost that is only necessary when producing the tracing information for an event
> that is enabled.
>
> Note that events using this define will probably have the "disable" property by
> default, in order to avoid such costs on regular builds.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  docs/tracing.txt  |   46 ++++++++++++++++++++++++++++++++++++++++------
>  scripts/tracetool |    8 +++++++-
>  2 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/docs/tracing.txt b/docs/tracing.txt
> index b36b54b..5bee61e 100644
> --- a/docs/tracing.txt
> +++ b/docs/tracing.txt
> @@ -103,12 +103,6 @@ respectively.  This ensures portability between 32- and 64-bit platforms.
>  4. Name trace events after their function.  If there are multiple trace events
>    in one function, append a unique distinguisher at the end of the name.
>
> -5. If specific trace events are going to be called a huge number of times, this
> -   might have a noticeable performance impact even when the trace events are
> -   programmatically disabled. In this case you should declare the trace event
> -   with the "disable" property, which will effectively disable it at compile
> -   time (using the "nop" backend).
> -
>  == Generic interface and monitor commands ==
>
>  You can programmatically query and control the dynamic state of trace events
> @@ -229,3 +223,43 @@ probes:
>                       --target-type system \
>                       --target-arch x86_64 \
>                       <trace-events >qemu.stp
> +
> +== 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.

> +
> +In addition, there might be cases where relatively complex computations must be
> +performed to generate values that are only used as arguments for a trace
> +function. In these cases you can guard such computations to avoid its
> +compilation when the event is disabled:
> +
> +    #include "trace.h"  /* needed for trace event prototype */
> +
> +    void *qemu_vmalloc(size_t size)
> +    {
> +        void *ptr;
> +        size_t align = QEMU_VMALLOC_ALIGN;
> +
> +        if (size < align) {
> +            align = getpagesize();
> +        }
> +        ptr = qemu_memalign(align, size);
> +        if (trace_qemu_vmalloc_enabled) { /* preprocessor macro */

Macros should be uppercase to avoid confusion.

> +            void *complex;
> +            /* some complex computations to produce the 'complex' value */
> +            trace_qemu_vmalloc(size, ptr, complex);
> +        }
> +        return ptr;
> +    }
> 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.)?

> +        fi
> +        if [ "$1" = "h" ]; then
> +            name=$(get_name "$str")
> +            echo "#define trace_${name}_enabled ${enabled}"
>         fi
>     done
>
>
>
>
Lluís Vilanova - Sept. 25, 2011, 5:39 p.m.
Blue Swirl writes:

> 2011/9/21 Lluís Vilanova <vilanova@ac.upc.edu>:
[...]
>> +== 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.

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.


Lluis
Blue Swirl - Sept. 25, 2011, 8:03 p.m.
2011/9/25 Lluís Vilanova <vilanova@ac.upc.edu>:
> Blue Swirl writes:
>
>> 2011/9/21 Lluís Vilanova <vilanova@ac.upc.edu>:
> [...]
>>> +== 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.
Lluís Vilanova - Sept. 25, 2011, 10:10 p.m.
Blue Swirl writes:

> 2011/9/25 Lluís Vilanova <vilanova@ac.upc.edu>:
>> Blue Swirl writes:
>> 
>>> 2011/9/21 Lluís Vilanova <vilanova@ac.upc.edu>:
>> [...]
>>>> +== 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

Patch

diff --git a/docs/tracing.txt b/docs/tracing.txt
index b36b54b..5bee61e 100644
--- a/docs/tracing.txt
+++ b/docs/tracing.txt
@@ -103,12 +103,6 @@  respectively.  This ensures portability between 32- and 64-bit platforms.
 4. Name trace events after their function.  If there are multiple trace events
    in one function, append a unique distinguisher at the end of the name.
 
-5. If specific trace events are going to be called a huge number of times, this
-   might have a noticeable performance impact even when the trace events are
-   programmatically disabled. In this case you should declare the trace event
-   with the "disable" property, which will effectively disable it at compile
-   time (using the "nop" backend).
-
 == Generic interface and monitor commands ==
 
 You can programmatically query and control the dynamic state of trace events
@@ -229,3 +223,43 @@  probes:
                       --target-type system \
                       --target-arch x86_64 \
                       <trace-events >qemu.stp
+
+== 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).
+
+In addition, there might be cases where relatively complex computations must be
+performed to generate values that are only used as arguments for a trace
+function. In these cases you can guard such computations to avoid its
+compilation when the event is disabled:
+
+    #include "trace.h"  /* needed for trace event prototype */
+    
+    void *qemu_vmalloc(size_t size)
+    {
+        void *ptr;
+        size_t align = QEMU_VMALLOC_ALIGN;
+    
+        if (size < align) {
+            align = getpagesize();
+        }
+        ptr = qemu_memalign(align, size);
+        if (trace_qemu_vmalloc_enabled) { /* preprocessor macro */
+            void *complex;
+            /* some complex computations to produce the 'complex' value */
+            trace_qemu_vmalloc(size, ptr, complex);
+        }
+        return ptr;
+    }
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
+        fi
+        if [ "$1" = "h" ]; then
+            name=$(get_name "$str")
+            echo "#define trace_${name}_enabled ${enabled}"
         fi
     done