| Message ID | 20111206163815.9767.50412.stgit@ginnungagap.bsc.es |
|---|---|
| State | New |
| Headers | show |
2011/12/6 Lluís Vilanova <vilanova@ac.upc.edu>: > Adds a 'TRACE_${NAME}_ENABLED' preprocessor define for each tracing event in > "trace.h". I think we should take it a step further: support TRACE_${NAME}_ENABLED at run-time. This means skipping the trace event code if the event is currently disabled. If the user enables it at runtime then the if (TRACE_${NAME}_ENABLED) will evaluate to true and we execute the code. SystemTap has support for this and it would be easy to do runtime support for stderr and simple (where we can test trace_list[event_id].state). The SystemTap mechanism is a macro, e.g. QEMU_${NAME}_ENABLED(), which fits perfectly with what you have posted. What do you think? (The difference is that your patch plus compiler dead-code elimination would completely remove the code when built without a trace event. What I'm suggesting becomes a test and branch to skip over the code which always gets compiled in.) Stefan
Stefan Hajnoczi writes: > 2011/12/6 Lluís Vilanova <vilanova@ac.upc.edu>: >> Adds a 'TRACE_${NAME}_ENABLED' preprocessor define for each tracing event in >> "trace.h". > I think we should take it a step further: support > TRACE_${NAME}_ENABLED at run-time. This means skipping the trace > event code if the event is currently disabled. If the user enables it > at runtime then the if (TRACE_${NAME}_ENABLED) will evaluate to true > and we execute the code. > SystemTap has support for this and it would be easy to do runtime > support for stderr and simple (where we can test > trace_list[event_id].state). > The SystemTap mechanism is a macro, e.g. QEMU_${NAME}_ENABLED(), which > fits perfectly with what you have posted. > What do you think? I think both are useful. In fact, AFAIR Blue Swirl did already propose a hybrid mechanism like you say, but I just didn't find the time to do it then as well as forgot about it up until now. My ideal hybrid solution would have three forms: * Event is disabled in "trace-events" (which is then in fact using the nop backend): static inline bool trace_${name}_enabled(void) { return false; } * Event is enabled in "trace-events" (e.g., using SystemTap): static inline bool trace_${name}_enabled(void) { return QEMU_${NAME}_ENABLED(); } tracetool should generate extra per-backend code here. * Event is enabled and instrumented: Let the user decide, including returning a constant "true" to eliminate all run-time checks. The next logical extension is to have a name/number based generic interface to get the state: event_t trace_event_get_id(char *name); char* trace_event_get_name(event_t event); bool trace_event_get_state(event_t event); bool trace_event_name_get_state(char *name) { return trace_event_get_state(trace_event_get_id(name)); } with the corresponding setter counterparts: bool trace_event_set_state(event_t e); bool trace_event_name_set_state(char *name) { return trace_event_set_state(trace_event_get_id(name)); } This needs some extra backend-specific code that I cannot do right now. Besides, this loses the ability to inline constant checks unless "trace_${name}_enabled" is maintained: static inline bool trace_${name}_enabled(void) { return TRACE_${NAME}_ENABLED && /* in current patch */ /* trace_event_get_state? */; /* should be optional */ } > (The difference is that your patch plus compiler dead-code elimination > would completely remove the code when built without a trace event. > What I'm suggesting becomes a test and branch to skip over the code > which always gets compiled in.) The problem here is how to have the three options available. You could have a new "instrument" backend, but that would then require the user to do all the work. As it is right now (the instrumentation), the user can simply put ad-hoc code in between, but still use the regular QEMU backends when necessary. Of course, one could argue this constant-based optimization is just not necessary at all. I just don't have numbers. Lluis
On Tue, Dec 06, 2011 at 05:38:15PM +0100, Lluís Vilanova wrote: > 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 | 9 ++++++++- > 2 files changed, 48 insertions(+), 7 deletions(-) Thanks, applied to the tracing tree: http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/tracing Stefan
diff --git a/docs/tracing.txt b/docs/tracing.txt index ea29f2c..a92716f 100644 --- a/docs/tracing.txt +++ b/docs/tracing.txt @@ -98,12 +98,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 @@ -234,3 +228,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 use the macro 'TRACE_${EVENT_NAME}_ENABLED' to +guard such computations and 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..701b517 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 NAME enabled begin="lineto$1_begin_$backend" process_line="lineto$1_$backend" end="lineto$1_end_$backend" @@ -534,8 +534,15 @@ 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") + NAME=$(echo $name | tr '[:lower:]' '[:upper:]') + 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 | 9 ++++++++- 2 files changed, 48 insertions(+), 7 deletions(-)