[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] trace: Provide a per-event status define for
From: |
Lluís Vilanova |
Subject: |
Re: [Qemu-devel] [PATCH v2] trace: Provide a per-event status define for conditional compilation |
Date: |
Thu, 08 Dec 2011 19:31:13 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.0.92 (gnu/linux) |
Stefan Hajnoczi writes:
> 2011/12/6 Lluís Vilanova <address@hidden>:
>> 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