[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

    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.


 "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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]