[Qemu-devel] [PATCH] tracetool: Omit useless QEMU_*_ENABLED() check

[Qemu-devel] [PATCH] tracetool: Omit useless QEMU_*_ENABLED() check

[Stefan Hajnoczi]

SystemTap provides a "semaphore" that can optionally be tested before
executing a trace event.  The purpose of this mechanism is to skip
expensive tracing code when the trace event is disabled.

For example, some applications may have trace events that format or
convert strings for trace events.  This expensive processing should only
be done in the case where the trace event is enabled.

Since QEMU's generated trace events never have such special-purpose
code, there is no reason to add the semaphore check.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 scripts/tracetool |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/scripts/tracetool b/scripts/tracetool
index 4c9951d..ffe3eba 100755
--- a/scripts/tracetool
+++ b/scripts/tracetool
@@ -415,9 +415,7 @@ linetoh_dtrace()
     # Define an empty function for the trace event
     cat <<EOF
 static inline void trace_$name($args) {
-    if (QEMU_${nameupper}_ENABLED()) {
-        QEMU_${nameupper}($argnames);
-    }
+    QEMU_${nameupper}($argnames);
 }
 EOF
 }
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] tracetool: Omit useless QEMU_*_ENABLED() check

[Masami Hiramatsu]

(2011/09/27 17:00), Stefan Hajnoczi wrote:
> SystemTap provides a "semaphore" that can optionally be tested before
> executing a trace event.  The purpose of this mechanism is to skip
> expensive tracing code when the trace event is disabled.
> 
> For example, some applications may have trace events that format or
> convert strings for trace events.  This expensive processing should only
> be done in the case where the trace event is enabled.
> 
> Since QEMU's generated trace events never have such special-purpose
> code, there is no reason to add the semaphore check.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Thanks Stefan,

The systemtap sdt.h just makes a space with NOP for inserting
a breakpoint. However, ENABLED() check adds another static
variable checking for each tracepoint. So without any clear
reason, ENABLED() should not be used.

I've checked that this can remove useless conditional jumps from
the code.

Without this patch:
0000000000420130 <vm_state_notify>:
[snip]
  420146:       48 89 44 24 08          mov    %rax,0x8(%rsp)
  42014b:       31 c0                   xor    %eax,%eax
 				 // Here, this check a semaphore
  42014d:       66 83 3d 0f da 58 00    cmpw   $0x0,0x58da0f(%rip)
  420154:       00
				// And a conditional jump
  420155:       75 3e                   jne    420195 <vm_state_notify+0x65>
  420157:       48 8b 1d 4a df 58 00    mov    0x58df4a(%rip),%rbx
[snip]
  420192:       41 5c                   pop    %r12
  420194:       c3                      retq
  420195:       90                      nop                 // jump to here
  420196:       eb bf                   jmp    420157 <vm_state_notify+0x27>

With this patch:
0000000000420130 <vm_state_notify>:
[snip]
  420146:       48 89 44 24 08          mov    %rax,0x8(%rsp)
  42014b:       31 c0                   xor    %eax,%eax
  42014d:       90                      nop	        // We have just one NOP
  42014e:       48 8b 1d 13 d7 58 00    mov    0x58d713(%rip),%rbx
  420155:       48 85 db                test   %rbx,%rbx

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

> ---
>  scripts/tracetool |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/tracetool b/scripts/tracetool
> index 4c9951d..ffe3eba 100755
> --- a/scripts/tracetool
> +++ b/scripts/tracetool
> @@ -415,9 +415,7 @@ linetoh_dtrace()
>      # Define an empty function for the trace event
>      cat <<EOF
>  static inline void trace_$name($args) {
> -    if (QEMU_${nameupper}_ENABLED()) {
> -        QEMU_${nameupper}($argnames);
> -    }
> +    QEMU_${nameupper}($argnames);
>  }
>  EOF
>  }


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [Qemu-devel] [PATCH] tracetool: Omit useless QEMU_*_ENABLED() check

[Stefan Hajnoczi]

On Tue, Sep 27, 2011 at 9:00 AM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> SystemTap provides a "semaphore" that can optionally be tested before
> executing a trace event.  The purpose of this mechanism is to skip
> expensive tracing code when the trace event is disabled.
>
> For example, some applications may have trace events that format or
> convert strings for trace events.  This expensive processing should only
> be done in the case where the trace event is enabled.
>
> Since QEMU's generated trace events never have such special-purpose
> code, there is no reason to add the semaphore check.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  scripts/tracetool |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)

Oops, this never got merged.  I have added it to my tracing queue and
it will be part of the next pull request:
https://github.com/stefanha/qemu/tree/tracing

Stefan