Stefan Hajnoczi writes: > 2011/12/6 Lluís Vilanova <vilan...@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 -- "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