Re: [Qemu-devel] [RFC] tracing: consistent usage of "disable" in "trace-events"

[Daniel P. Berrange]

On Mon, Nov 08, 2010 at 04:55:10PM +0100, Lluís wrote:
> Daniel P Berrange writes:
> 
> > On Mon, Nov 08, 2010 at 03:42:15PM +0100, Lluís wrote:
> >> On the current implementation, the "disable" keyword in "trace-events"
> >> has different semantics, depending on the backend:
> >> 
> >> * nop    : ignored (not a problem)
> >> * simple : enables tracing, but sets dynamic state to disable
> >> * ust    : disables tracing (uses nop backend)
> >> * dtrace : same as simple
> >> 
> >> Would it be possible to just use nop whenever the event is disabled in
> >> trace-events? If you agree I can cook the patch, as it's pretty simple.
> 
> > I don't particularly see the point of the 'disable' keyword existing at
> > all, unless there are performance implications for a particular trace 
> > backend. For the DTrace backend I strip & ignore the disable keyword
> > because probes that are compiled in, reduce to a inline conditional 
> > check that has no serious overhead when no trace client is active.
> 
> I think the same is applicable to the UST backend.
> 
> But it is not true when tracing guest events (e.g., memory
> accesses). Not because of the backend, but because of the frequency of
> appearance of such events.
> 
> In this case, the auto-generated code is on the lines of (I haven't yet
> posted the patch series producing this):
> 
>   [called during TCG code generation -- e.g., translate.c]
> 
>   #define TRACE_CURR_CPU_STATE_SET (cpu_single_env->trace_state_set)
>   #define trace_guest_vmem_cpu_event 0 // number of per-CPU trace event
>    
>   static inline void trace_gen_guest_vmem (TCGv_i64 addr, uint32_t size, 
> uint32_t write)
>   {
>      if (TRACE_CURR_CPU_STATE_SET & (1 << trace_guest_vmem_cpu_event)) {
>         gen_helper_proxy_guest_vmem(addr, tcg_const_i32(size), 
> tcg_const_i32(write));
>      }
>   }
> 
>   [extra helper functions -- declared at helper.h]
>    
>   void helper_proxy_guest_vmem (uint64_t addr, uint32_t size, uint32_t write)
>   {
>      trace_guest_vmem(addr, size, write);
>   }
> 
> (*) A state set is a bitset with the events that have been declared with
>     the "gen" keyword in "trace-events".
> 
> This code has indeed a performance cost, so I opted to follow the
> approach taken by the UST backend ("disable" produces a trace event with
> "nop"). When I opted for this, only simple and ust where in 'tracetool'.
> 
> In any case, there might appear other events that could have performance
> implications, although I understand the ease of usage of having all
> trace events available by default.

Ok, I agree that if we have tracepoints in the kind of places in TCG
you describe, then this could have measurable performance impact.

> That's why I would rather declare all trace-events without the "disable"
> keyword, and leave it only on those events that are known to have a high
> frequency, as no backend should have so poor performance as to force
> events to "disappear".

This sounds like a reasonable plan to me. 

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|