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

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

[Lluís]

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.


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

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

[Daniel P. Berrange]

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.

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 :|

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

[Lluís]

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.

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".


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

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 :|

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

[Lluís]

Daniel P Berrange writes:

> On Mon, Nov 08, 2010 at 04:55:10PM +0100, Lluís wrote:
>> 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. 


Then I'll cook a simple patch for that later.

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