| Message ID | 20110921183838.9431.70792.stgit@ginnungagap.bsc.es |
|---|---|
| State | New |
| Headers | show |
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 > > > >
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
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.
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
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
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(-)