[Qemu-devel] [PATCH] tcg/monitor: remove "info profile"

[Qemu-devel] [PATCH] tcg/monitor: remove "info profile"

[Paolo Bonzini]

"info profile" is not great in several ways:

1) half of it only works for TCG, but doesn't say this anywhere.

2) the division by get_ticks_per_sec() doesn't work since the unit of
measurement is clock cycles rather than nanoseconds.  (Broken since 2006
by Fabrice).

3) you really can get the same information from "top" now that we have
VCPU threads.

4) It declares non-existing extern variables qemu_time_start and
tlb_flush_time, the latter of which has never existed _at all_ in the
code base.

Let's remove and leave the remaining TCG dump/profiling functionality that
is keyed off --enable-profiler.  This "fixes" the conflict between the
qemu_time() function and the qemu_time variable used by "info profile".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c               |  9 ---------
 include/qemu/timer.h |  4 ----
 monitor.c            | 28 ----------------------------
 vl.c                 |  9 ---------
 4 files changed, 50 deletions(-)

diff --git a/cpus.c b/cpus.c
index 0fac143..b241b59 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1318,13 +1318,7 @@ static int tcg_cpu_exec(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
     int ret;
-#ifdef CONFIG_PROFILER
-    int64_t ti;
-#endif
 
-#ifdef CONFIG_PROFILER
-    ti = profile_getclock();
-#endif
     if (use_icount) {
         int64_t count;
         int64_t deadline;
@@ -1352,9 +1346,6 @@ static int tcg_cpu_exec(CPUArchState *env)
         cpu->icount_extra = count;
     }
     ret = cpu_exec(env);
-#ifdef CONFIG_PROFILER
-    qemu_time += profile_getclock() - ti;
-#endif
     if (use_icount) {
         /* Fold pending instructions back into the
            instruction counter, and clear the interrupt flag.  */
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index eba8b21..cc74a30 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -1001,10 +1001,6 @@ static inline int64_t profile_getclock(void)
 {
     return cpu_get_real_ticks();
 }
-
-extern int64_t qemu_time, qemu_time_start;
-extern int64_t tlb_flush_time;
-extern int64_t dev_time;
 #endif
 
 #endif
diff --git a/monitor.c b/monitor.c
index c86a89e..ab593df 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1972,27 +1972,6 @@ static void hmp_info_numa(Monitor *mon, const QDict *qdict)
     g_free(node_mem);
 }
 
-#ifdef CONFIG_PROFILER
-
-int64_t qemu_time;
-int64_t dev_time;
-
-static void hmp_info_profile(Monitor *mon, const QDict *qdict)
-{
-    monitor_printf(mon, "async time  %" PRId64 " (%0.3f)\n",
-                   dev_time, dev_time / (double)get_ticks_per_sec());
-    monitor_printf(mon, "qemu time   %" PRId64 " (%0.3f)\n",
-                   qemu_time, qemu_time / (double)get_ticks_per_sec());
-    qemu_time = 0;
-    dev_time = 0;
-}
-#else
-static void hmp_info_profile(Monitor *mon, const QDict *qdict)
-{
-    monitor_printf(mon, "Internal profiler not compiled\n");
-}
-#endif
-
 /* Capture support */
 static QLIST_HEAD (capture_list_head, CaptureState) capture_head;
 
@@ -2766,13 +2745,6 @@ static mon_cmd_t info_cmds[] = {
         .mhandler.cmd = hmp_info_usbhost,
     },
     {
-        .name       = "profile",
-        .args_type  = "",
-        .params     = "",
-        .help       = "show profiling information",
-        .mhandler.cmd = hmp_info_profile,
-    },
-    {
         .name       = "capture",
         .args_type  = "",
         .params     = "",
diff --git a/vl.c b/vl.c
index b47e223..aeb2125 100644
--- a/vl.c
+++ b/vl.c
@@ -1784,18 +1784,9 @@ static void main_loop(void)
 {
     bool nonblocking;
     int last_io = 0;
-#ifdef CONFIG_PROFILER
-    int64_t ti;
-#endif
     do {
         nonblocking = !kvm_enabled() && !xen_enabled() && last_io > 0;
-#ifdef CONFIG_PROFILER
-        ti = profile_getclock();
-#endif
         last_io = main_loop_wait(nonblocking);
-#ifdef CONFIG_PROFILER
-        dev_time += profile_getclock() - ti;
-#endif
     } while (!main_loop_should_exit());
 }
 
-- 
2.3.0

Re: [Qemu-devel] [PATCH] tcg/monitor: remove "info profile"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> "info profile" is not great in several ways:
>
> 1) half of it only works for TCG, but doesn't say this anywhere.
>
> 2) the division by get_ticks_per_sec() doesn't work since the unit of
> measurement is clock cycles rather than nanoseconds.  (Broken since 2006
> by Fabrice).
>
> 3) you really can get the same information from "top" now that we have
> VCPU threads.
>
> 4) It declares non-existing extern variables qemu_time_start and
> tlb_flush_time, the latter of which has never existed _at all_ in the
> code base.
>
> Let's remove and leave the remaining TCG dump/profiling functionality that
> is keyed off --enable-profiler.  This "fixes" the conflict between the
> qemu_time() function and the qemu_time variable used by "info profile".

I'd appreciate a brief hint at what the "remaining TCG dump/profiling
functionality" is here.

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpus.c               |  9 ---------
>  include/qemu/timer.h |  4 ----
>  monitor.c            | 28 ----------------------------
>  vl.c                 |  9 ---------
>  4 files changed, 50 deletions(-)
[...]
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index eba8b21..cc74a30 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -1001,10 +1001,6 @@ static inline int64_t profile_getclock(void)
   #ifdef CONFIG_PROFILER
   static inline int64_t profile_getclock(void)
>  {
>      return cpu_get_real_ticks();
>  }
> -
> -extern int64_t qemu_time, qemu_time_start;
> -extern int64_t tlb_flush_time;
> -extern int64_t dev_time;
>  #endif
>  
>  #endif

I can't help to wonder what wrapping profile_getclock() around
cpu_get_real_ticks() buys us.

[...]

With or without the suggested commit message improvement:

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] tcg/monitor: remove "info profile"

[Paolo Bonzini]

On 11/03/2015 09:13, Markus Armbruster wrote:
> > Let's remove and leave the remaining TCG dump/profiling functionality that
> > is keyed off --enable-profiler.  This "fixes" the conflict between the
> > qemu_time() function and the qemu_time variable used by "info profile".
> 
> I'd appreciate a brief hint at what the "remaining TCG dump/profiling
> functionality" is here.

Some stuff that apparently is only reachable from gdb via "call foo()".
 I honestly haven't looked in depth, it's probably not the time to think
about removal either.

Paolo