Re: [Qemu-devel] [PATCH 00/12] trace: [tcg] Allow tracing guest events in TCG-generated code

[Richard Henderson <rth@twiddle.net>]

On 02/04/2014 12:33 PM, Lluís Vilanova wrote:
> Richard Henderson writes:
> 
>> On 01/31/2014 08:09 AM, Lluís Vilanova wrote:
>>> Adds the base ability to specify which events in the "trace-events" file may be
>>> used to trace guest activity in the TCG code (using the "tcg" event propery).
>>>
>>> Such events generate an extra set of tracing functions that can be called during
>>> TCG code generation and will automatically redirect a call to the appropriate
>>> backend-dependent tracing functions when the guest code is executed.
>>>
>>> Files generating guest code (TCG) must include "trace-tcg.h". Files declaring
>>> per-target helpers ("${target}/helper.h") must include
>>> "trace/generated-helpers.h".
>>>
>>> The flow of the generated routines is:
>>>
>>>
>>> [At translation time]
>>>
>>> * trace_${name}_tcg(bool, TCGv)
>>> Declared: "trace/generated-tcg-tracers.h"
>>> Defined : "trace/generated-tcg-tracers.h"
>>>
>>> * gen_helper_trace_${name}_tcg(bool, TCGv)
>>> Declared: "trace/generated-helpers.h"
>>> Defined : "trace/generated-helpers.h"
>>>
>>> Automatically transforms all the arguments and allocates them into the
>>> appropriate TCG temporary values (which are also freed). Provides a more
>>> streamlined interface by allowing events in "trace-events" to take a mix of
>>> tracing-supported types and TCG types.
>>>
>>> * gen_helper_trace_${name}_tcg_proxy(TCGi32, TCGv)
>>> Declared: "trace/generated-helpers.h"
>>> Defined : "trace/generated-helpers.h" (using helper machinery)
>>>
>>> The actual TCG helper function, created using QEMU's TCG helper machinery.
> 
>> I suppose I have no major objection to the feature, although frankly it's
>> not especially exciting.  I can't really imagine ever wanting to bulk trace
>> all of the helpers.  Tracing specific helpers on a target-by-target basis,
>> sure.  But that can be done just as easily as adding tracing code to any
>> other bit of C.
> 
> I'm not sure I understand this comment. The patch does not add helper tracing
> capabilities, but generates a "trace_foo_tcg" routine that can be called during
> TCG code generation, and will call "trace_foo" when that TCG code is
> executed.

Ah, see, I told you I was probably reading the patches wrong.  With all the
helpers.h changes, I thought this was somehow related to tracing the existing
helpers.  But the existance of trace_foo_tcg is triggered by the trace-events file?

>> I would strongly suggest this is backward.  One should perform the check for
>> the tracepoint being enabled at translation time before emitting the call to
>> the helper in the first place.
> 
> Right, the thing is that dynamically enabling/disabling such events will not
> immediately show up in the trace if I do the check at translation time
> (trace_foo_tcg), since QEMU will execute the cached TCG translations. I see the
> following solutions to ensure traces are accurate:
> 
> * Delay the check until execution time.
> 
> * Check at translation time; flush translation cache when the tracing state of
>   a TCG event changes.
> 
> * Check at translation time; use multiple translation caches, one for each
>   possible tracing state of all the TCG-enabled events.
> 
> This series implements the first approach, since it's correct and much
> simpler.
> 
> Other patches I did not send implement the third approach, which is quite
> efficient if one is dynamically switching the tracing state while executing
> mostly-cached code (e.g., profiling the accesses).

How often do events change state?  My guess is exceedingly rarely.
And by "rare" I mean something well under once per minute.  ;-)

At which point, option 2 would be the best bet, I think.

> I can wait for a later series to send the third option, or even implement the
> second, but I just wanted to keep this one as simple as possible. Also,
> implementing any of these two last approaches would provide minimal overheads on
> builds that have such events enabled at compile time.

Fair enough.


r~