Re: [Qemu-devel] [PATCH V5 4/6] cpu_exec: Add sleeping algorithm

[Paolo Bonzini <pbonzini@redhat.com>]

Il 25/07/2014 11:56, Sebastian Tanase ha scritto:
> The goal is to sleep qemu whenever the guest clock
> is in advance compared to the host clock (we use
> the monotonic clocks). The amount of time to sleep
> is calculated in the execution loop in cpu_exec.
> 
> At first, we tried to approximate at each for loop the real time elapsed
> while searching for a TB (generating or retrieving from cache) and
> executing it. We would then approximate the virtual time corresponding
> to the number of virtual instructions executed. The difference between
> these 2 values would allow us to know if the guest is in advance or delayed.
> However, the function used for measuring the real time
> (qemu_clock_get_ns(QEMU_CLOCK_REALTIME)) proved to be very expensive.
> We had an added overhead of 13% of the total run time.
> 
> Therefore, we modified the algorithm and only take into account the
> difference between the 2 clocks at the begining of the cpu_exec function.
> During the for loop we try to reduce the advance of the guest only by
> computing the virtual time elapsed and sleeping if necessary. The overhead
> is thus reduced to 3%. Even though this method still has a noticeable
> overhead, it no longer is a bottleneck in trying to achieve a better
> guest frequency for which the guest clock is faster than the host one.
> 
> As for the the alignement of the 2 clocks, with the first algorithm
> the guest clock was oscillating between -1 and 1ms compared to the host clock.
> Using the second algorithm we notice that the guest is 5ms behind the host, which
> is still acceptable for our use case.
> 
> The tests where conducted using fio and stress. The host machine in an i5 CPU at
> 3.10GHz running Debian Jessie (kernel 3.12). The guest machine is an arm versatile-pb
> built with buildroot.
> 
> Currently, on our test machine, the lowest icount we can achieve that is suitable for
> aligning the 2 clocks is 6. However, we observe that the IO tests (using fio) are
> slower than the cpu tests (using stress).
> 
> Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
> Tested-by: Camille Bégué <camille.begue@openwide.fr>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpu-exec.c           | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  cpus.c               | 17 ++++++++++
>  include/qemu/timer.h |  1 +
>  3 files changed, 109 insertions(+)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 38e5f02..1a725b6 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -22,6 +22,84 @@
>  #include "tcg.h"
>  #include "qemu/atomic.h"
>  #include "sysemu/qtest.h"
> +#include "qemu/timer.h"
> +
> +/* -icount align implementation. */
> +
> +typedef struct SyncClocks {
> +    int64_t diff_clk;
> +    int64_t original_instr_counter;
> +} SyncClocks;
> +
> +#if !defined(CONFIG_USER_ONLY)
> +/* Allow the guest to have a max 3ms advance.
> + * The difference between the 2 clocks could therefore
> + * oscillate around 0.
> + */
> +#define VM_CLOCK_ADVANCE 3000000
> +
> +static int64_t delay_host(int64_t diff_clk)
> +{
> +    if (diff_clk > VM_CLOCK_ADVANCE) {
> +#ifndef _WIN32
> +        struct timespec sleep_delay, rem_delay;
> +        sleep_delay.tv_sec = diff_clk / 1000000000LL;
> +        sleep_delay.tv_nsec = diff_clk % 1000000000LL;
> +        if (nanosleep(&sleep_delay, &rem_delay) < 0) {
> +            diff_clk -= (sleep_delay.tv_sec - rem_delay.tv_sec) * 1000000000LL;
> +            diff_clk -= sleep_delay.tv_nsec - rem_delay.tv_nsec;
> +        } else {
> +            diff_clk = 0;
> +        }
> +#else
> +        Sleep(diff_clk / SCALE_MS);
> +        diff_clk = 0;
> +#endif
> +    }
> +    return diff_clk;
> +}
> +
> +static int64_t instr_to_vtime(int64_t instr_counter, const CPUState *cpu)
> +{
> +    int64_t instr_exec_time;
> +    instr_exec_time = instr_counter -
> +                      (cpu->icount_extra +
> +                       cpu->icount_decr.u16.low);
> +    instr_exec_time = instr_exec_time << icount_time_shift;
> +
> +    return instr_exec_time;
> +}
> +
> +static void align_clocks(SyncClocks *sc, const CPUState *cpu)
> +{
> +    if (!icount_align_option) {
> +        return;
> +    }
> +    sc->diff_clk += instr_to_vtime(sc->original_instr_counter, cpu);
> +    sc->original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
> +    sc->diff_clk = delay_host(sc->diff_clk);
> +}

Just two comments:

1) perhaps s/original/last/ in original_instr_counter?

2) I think I prefer this to be written like:

    instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
    instr_exec_time = sc->original_instr_counter - instr_counter;
    sc->original_instr_counter = instr_counter
    sc->diff_clk += instr_exec_time << icount_time_shift;
    sc->diff_clk = delay_host(sc->diff_clk);

If you agree, I can do it when applying the patches.

Thanks for your persistence, I'm very happy with this version!

As a follow up, do you think it's possible to modify the places where
you run align_clocks, so that you sleep with the iothread mutex *not* taken?

Paolo