| Message ID | 1425988051-65333-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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()); }
"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(-)