[Qemu-devel] [RFC PATCH V2 0/5] icount: Implement delay algorithm between guest and host clocks

[Qemu-devel] [RFC PATCH V2 0/5] icount: Implement delay algorithm between guest and host clocks

[Sebastian Tanase]

The icount option already implemented in QEMU allows the guest to run at a theoretical
frequency of 1/(2^N) GHz (N is the icount parameter). The goal of this patch is to have a
real guest frequency close to the one imposed by using the icount option.

The main idea behind the algorithm is that we compare the virtual monotonic clock and the
host monotonic clock. For big icounts (on our test machine, an i5 CPU @ 3.10GHz, icounts
starting at 6) the guest clock will be ahead of the host clock. In this case, we try to
sleep QEMU for the difference between the 2 clocks. Therefore, the guest would have
executed for a period almost equally to the one imposed by icount. We should point out
that the algorithm works only for those icounts that allow the guest clock to be in front
of the host clock.

The first patch adds the 'align' parameter for icount and changes the icount option from
'-icount [N|auto]' to '-icount [icount=][N|auto][,align=on|off]'.

The second patch exports 'icount_time_shift' so that it can be used in places other than
cpus.c; we need it in cpu-exec.c for calculating for how long we want QEMU to sleep.

The third patch implements the algorithm used for calculating the delay we want to sleep.
It uses the number of instructions executed by the virtual cpu and also the icount_time_shift.
All the parts in cpu-exec.c are only compiled for soft-mmu architectures otherwise the code
will not compile for all architectures (linux-user and soft-mmu).

The forth patch updates the algorithm to take into account an initial offset between clocks.
The offset can be 0 (for example on ARM) or another value (typically it is almost equal
to the realtime clock) on other architectures (for example on x86).

The fifth and final patch prints to the console whenever the guest clock runs behind the host
clock. The fastest printing speed is every 2 seconds, and we only print if the align option
is enabled.

v1 -> v2

* Signal errors in configure_icount() as suggested by Paolo Bonzini (patch 1).
* Update qtest.c to take into account the new configure_icount() (patch 1).
* Surround delay code in cpu-exec.c with !defined(CONFIG_USER_ONLY) (patch 3).
* Clean up delay code in cpu-exec.c as suggested by Paolo Bonzini
  and Juan Quintela (patch 3).
* Take into account initial offset between real and virtual clocks (patch 4).

Sebastian Tanase (5):
  icount: Add 'align' and 'icount' options
  icount: Make icount_time_shift available everywhere
  cpu_exec: Add sleeping algorithm
  icount_warp: Take into account initial offset between clocks
  cpu_exec: Print to console if the guest is late

 cpu-exec.c                | 108 +++++++++++++++++++++++++++++++++++++++++++++-
 cpus.c                    |  38 ++++++++++++++--
 include/qapi/qmp/qerror.h |   9 ++++
 include/qemu-common.h     |   6 ++-
 qemu-options.hx           |  19 ++++++--
 qtest.c                   |  19 +++++++-
 vl.c                      |  43 +++++++++++++++---
 7 files changed, 225 insertions(+), 17 deletions(-)

-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V2 1/5] icount: Add 'align' and 'icount' options

[Sebastian Tanase]

The align option is used for activating the align algorithm
in order to synchronise the host clock and the guest clock.
Therefore we slightly modified the existing icount option.
Thus, the new way to specify an icount is
'-icount [icount=][N|auto][,align]'

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
---
 cpus.c                    | 19 ++++++++++++++++++-
 include/qapi/qmp/qerror.h |  9 +++++++++
 include/qemu-common.h     |  4 +++-
 qemu-options.hx           | 19 ++++++++++++++++---
 qtest.c                   | 19 +++++++++++++++++--
 vl.c                      | 43 ++++++++++++++++++++++++++++++++++++-------
 6 files changed, 99 insertions(+), 14 deletions(-)

diff --git a/cpus.c b/cpus.c
index dd7ac13..b78a418 100644
--- a/cpus.c
+++ b/cpus.c
@@ -438,11 +438,24 @@ static const VMStateDescription vmstate_timers = {
     }
 };
 
-void configure_icount(const char *option)
+void configure_icount(QemuOpts *opts, Error **errp)
 {
+    const char *option;
+
     seqlock_init(&timers_state.vm_clock_seqlock, NULL);
     vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
+    option = qemu_opt_get(opts, "icount");
+    icount_align_option = qemu_opt_get_bool(opts, "align", false);
+    /* When using -icount, it is mandatory to specify an icount value:
+       -icount icount=N|auto */
     if (!option) {
+        error_set(errp, QERR_ICOUNT_MISSING);
+        return;
+    }
+    /* When using -icount icount, the icount option will be
+       misinterpreted as a boolean */
+    if (strcmp(option, "on") == 0 || strcmp(option, "off") == 0) {
+        error_set(errp, QERR_INVALID_ICOUNT);
         return;
     }
 
@@ -452,6 +465,10 @@ void configure_icount(const char *option)
         icount_time_shift = strtol(option, NULL, 0);
         use_icount = 1;
         return;
+    } else if (icount_align_option) {
+        /* Do not allow icount auto and align on */
+        error_set(errp, QERR_INVALID_ALIGN, option);
+        return;
     }
 
     use_icount = 2;
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 902d1a7..ac307d0 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -91,6 +91,15 @@ void qerror_report_err(Error *err);
 #define QERR_FEATURE_DISABLED \
     ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
 
+#define QERR_ICOUNT_MISSING \
+    ERROR_CLASS_GENERIC_ERROR, "Icount option missing"
+
+#define QERR_INVALID_ALIGN \
+    ERROR_CLASS_GENERIC_ERROR, "Icount %s and align on are incompatible"
+
+#define QERR_INVALID_ICOUNT \
+    ERROR_CLASS_GENERIC_ERROR, "Icount must be a number or auto"
+
 #define QERR_INVALID_BLOCK_FORMAT \
     ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
 
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 66ceceb..288b460 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -41,6 +41,7 @@
 #include <assert.h>
 #include <signal.h>
 #include "glib-compat.h"
+#include "qemu/option.h"
 
 #ifdef _WIN32
 #include "sysemu/os-win32.h"
@@ -105,8 +106,9 @@ static inline char *realpath(const char *path, char *resolved_path)
 #endif
 
 /* icount */
-void configure_icount(const char *option);
+void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
+extern int icount_align_option;
 
 #include "qemu/osdep.h"
 #include "qemu/bswap.h"
diff --git a/qemu-options.hx b/qemu-options.hx
index d0714c4..5e7c956 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2898,11 +2898,11 @@ re-inject them.
 ETEXI
 
 DEF("icount", HAS_ARG, QEMU_OPTION_icount, \
-    "-icount [N|auto]\n" \
+    "-icount [icount=N|auto][,align=on|off]\n" \
     "                enable virtual instruction counter with 2^N clock ticks per\n" \
-    "                instruction\n", QEMU_ARCH_ALL)
+    "                instruction and enable aligning the host and virtual clocks\n", QEMU_ARCH_ALL)
 STEXI
-@item -icount [@var{N}|auto]
+@item -icount [icount=@var{N}|auto][,align=on|off]
 @findex -icount
 Enable virtual instruction counter.  The virtual cpu will execute one
 instruction every 2^@var{N} ns of virtual time.  If @code{auto} is specified
@@ -2913,6 +2913,19 @@ Note that while this option can give deterministic behavior, it does not
 provide cycle accurate emulation.  Modern CPUs contain superscalar out of
 order cores with complex cache hierarchies.  The number of instructions
 executed often has little or no correlation with actual performance.
+
+@option{align=on} will activate the delay algorithm which will try to
+to synchronise the host clock and the virtual clock. The goal is to
+have a guest running at the real frequency imposed by
+icount. Whenever the guest clock is behind the host clock and if
+@option{align=on} is specified then we print a messsage to the user
+to inform about the delay.
+You must provide a value for @option{icount} other than @code{auto}
+for the align option to be taken into account.
+Currently this option does not work with @code{auto}.
+Note: The sync algorithm will work on those icounts on which
+the guest clock runs ahead of the host clock. Typically this happens
+when the icount value is high (how high depends on the host machine).
 ETEXI
 
 DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
diff --git a/qtest.c b/qtest.c
index 04a6dc1..9b6c29b 100644
--- a/qtest.c
+++ b/qtest.c
@@ -19,6 +19,9 @@
 #include "hw/irq.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpus.h"
+#include "qemu/config-file.h"
+#include "qemu/option.h"
+#include "qemu/error-report.h"
 
 #define MAX_IRQ 256
 
@@ -509,10 +512,22 @@ static void qtest_event(void *opaque, int event)
     }
 }
 
-int qtest_init_accel(MachineClass *mc)
+static void configure_qtest_icount(const char *options)
 {
-    configure_icount("0");
+    QemuOpts *opts  = qemu_opts_parse(qemu_find_opts("icount"), options, 0);
+    Error *errp = NULL;
+    configure_icount(opts, &errp);
+    qemu_opts_del(opts);
+    if (errp != NULL) {
+        error_report("%s", error_get_pretty(errp));
+        error_free(errp);
+        exit(1);
+    }
+}
 
+int qtest_init_accel(MachineClass *mc)
+{
+    configure_qtest_icount("icount=0");
     return 0;
 }
 
diff --git a/vl.c b/vl.c
index ac0e3d7..1bd467a 100644
--- a/vl.c
+++ b/vl.c
@@ -182,6 +182,7 @@ uint8_t *boot_splash_filedata;
 size_t boot_splash_filedata_size;
 uint8_t qemu_extra_params_fw[2];
 
+int icount_align_option;
 typedef struct FWBootEntry FWBootEntry;
 
 struct FWBootEntry {
@@ -524,6 +525,21 @@ static QemuOptsList qemu_mem_opts = {
     },
 };
 
+static QemuOptsList qemu_icount_opts = {
+    .name = "icount",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_icount_opts.head),
+    .desc = {
+        {
+            .name = "icount",
+            .type = QEMU_OPT_STRING,
+        }, {
+            .name = "align",
+            .type = QEMU_OPT_BOOL,
+        },
+        { /* end of list */ }
+    },
+};
+
 /**
  * Get machine options
  *
@@ -2957,13 +2973,12 @@ int main(int argc, char **argv, char **envp)
 {
     int i;
     int snapshot, linux_boot;
-    const char *icount_option = NULL;
     const char *initrd_filename;
     const char *kernel_filename, *kernel_cmdline;
     const char *boot_order;
     DisplayState *ds;
     int cyls, heads, secs, translation;
-    QemuOpts *hda_opts = NULL, *opts, *machine_opts;
+    QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
     QemuOptsList *olist;
     int optind;
     const char *optarg;
@@ -2991,6 +3006,7 @@ int main(int argc, char **argv, char **envp)
     const char *trace_file = NULL;
     const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
                                         1024 * 1024;
+    Error *icount_err = NULL;
 
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
@@ -3024,6 +3040,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_realtime_opts);
     qemu_add_opts(&qemu_msg_opts);
     qemu_add_opts(&qemu_name_opts);
+    qemu_add_opts(&qemu_icount_opts);
 
     runstate_init();
 
@@ -3833,7 +3850,11 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_icount:
-                icount_option = optarg;
+                icount_opts = qemu_opts_parse(qemu_find_opts("icount"),
+                                              optarg, 0);
+                if (!icount_opts) {
+                    exit(1);
+                }
                 break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
@@ -4301,11 +4322,19 @@ int main(int argc, char **argv, char **envp)
     qemu_spice_init();
 #endif
 
-    if (icount_option && (kvm_enabled() || xen_enabled())) {
-        fprintf(stderr, "-icount is not allowed with kvm or xen\n");
-        exit(1);
+    if (icount_opts) {
+        if (kvm_enabled() || xen_enabled()) {
+            fprintf(stderr, "-icount is not allowed with kvm or xen\n");
+            exit(1);
+        }
+        configure_icount(icount_opts, &icount_err);
+        qemu_opts_del(icount_opts);
+        if (icount_err != NULL) {
+            error_report("%s", error_get_pretty(icount_err));
+            error_free(icount_err);
+            exit(1);
+        }
     }
-    configure_icount(icount_option);
 
     /* clean up network at qemu process termination */
     atexit(&net_cleanup);
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V2 2/5] icount: Make icount_time_shift available everywhere

[Sebastian Tanase]

icount_time_shift is used for calculting the delay
qemu has to sleep in order to synchronise the
host and guest clocks. Therefore, we need it in
cpu-exec.c.

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
---
 cpus.c                | 8 ++++++--
 include/qemu-common.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index b78a418..bbe8743 100644
--- a/cpus.c
+++ b/cpus.c
@@ -103,8 +103,12 @@ static bool all_cpu_threads_idle(void)
 /* Compensate for varying guest execution speed.  */
 static int64_t qemu_icount_bias;
 static int64_t vm_clock_warp_start;
-/* Conversion factor from emulated instructions to virtual clock ticks.  */
-static int icount_time_shift;
+/* Conversion factor from emulated instructions to virtual clock ticks.
+ * icount_time_shift is defined as extern in include/qemu-common.h because
+ * it is used (in cpu-exec.c) for calculating the delay for sleeping
+ * qemu in order to align the host and virtual clock.
+ */
+int icount_time_shift;
 /* Arbitrarily pick 1MIPS as the minimum allowable speed.  */
 #define MAX_ICOUNT_SHIFT 10
 
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 288b460..60478fa 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -108,6 +108,7 @@ static inline char *realpath(const char *path, char *resolved_path)
 /* icount */
 void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
+extern int icount_time_shift;
 extern int icount_align_option;
 
 #include "qemu/osdep.h"
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V2 3/5] cpu_exec: Add sleeping algorithm

[Sebastian Tanase]

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.

Basically, using QEMU_CLOCK_REALTIME, we calculate
the real time duration of the execution (meaning
generating TBs and executing them) and using the
the fields icount_decr.u16.low and icount_extra
(from the CPUState structure) shifted by icount_time_shift
we calculate the theoretical virtual time elapsed.
Having these 2 values, we can determine if the
guest is in advance and sleep.

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
---

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).
---
 cpu-exec.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 38e5f02..ccc2c0e 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -22,6 +22,7 @@
 #include "tcg.h"
 #include "qemu/atomic.h"
 #include "sysemu/qtest.h"
+#include "qemu/timer.h"
 
 void cpu_loop_exit(CPUState *cpu)
 {
@@ -209,6 +210,55 @@ static void cpu_handle_debug_exception(CPUArchState *env)
     }
 }
 
+#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)
+{
+    struct timespec sleep_delay, rem_delay;
+    if (diff_clk > VM_CLOCK_ADVANCE) {
+        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;
+        }
+    }
+    return diff_clk;
+}
+
+static int64_t update_clock_difference(int64_t diff_clk,
+                                    int64_t *instr_counter,
+                                    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;
+    *instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
+
+    return diff_clk + instr_exec_time;
+}
+
+static int64_t align_clocks(int64_t diff_clk, int64_t *oic, CPUState *cpu)
+{
+    if (icount_align_option) {
+        diff_clk = update_clock_difference(diff_clk, oic, cpu);
+        diff_clk = delay_host(diff_clk);
+    }
+
+    return diff_clk;
+}
+
+#endif /* CONFIG USER ONLY */
+
 /* main execution loop */
 
 volatile sig_atomic_t exit_request;
@@ -227,6 +277,10 @@ int cpu_exec(CPUArchState *env)
     TranslationBlock *tb;
     uint8_t *tc_ptr;
     uintptr_t next_tb;
+#if !(defined(CONFIG_USER_ONLY))
+    /* Delay algorithm */
+    int64_t diff_clk, original_instr_counter;
+#endif
     /* This must be volatile so it is not trashed by longjmp() */
     volatile bool have_tb_lock = false;
 
@@ -282,7 +336,17 @@ int cpu_exec(CPUArchState *env)
 #error unsupported target CPU
 #endif
     cpu->exception_index = -1;
-
+#if !(defined(CONFIG_USER_ONLY))
+    if (icount_align_option) {
+        /* Calculate difference between guest clock and host clock.
+           This delay includes the delay of the last cycle, so
+           what we have to do is sleep until it is 0. As for the
+           advance/delay we gain here, we try to fix it next time. */
+        diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
+                   qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+        original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
+    }
+#endif
     /* prepare setjmp context for exception handling */
     for(;;) {
         if (sigsetjmp(cpu->jmp_env, 0) == 0) {
@@ -672,6 +736,13 @@ int cpu_exec(CPUArchState *env)
                             if (insns_left > 0) {
                                 /* Execute remaining instructions.  */
                                 cpu_exec_nocache(env, insns_left, tb);
+#if !(defined(CONFIG_USER_ONLY))
+                                /* We can ignore the return value
+                                   because we exit anyway. */
+                                align_clocks(diff_clk,
+                                             &original_instr_counter,
+                                             cpu);
+#endif
                             }
                             cpu->exception_index = EXCP_INTERRUPT;
                             next_tb = 0;
@@ -684,6 +755,11 @@ int cpu_exec(CPUArchState *env)
                     }
                 }
                 cpu->current_tb = NULL;
+#if !(defined(CONFIG_USER_ONLY))
+                diff_clk = align_clocks(diff_clk,
+                                        &original_instr_counter,
+                                        cpu);
+#endif
                 /* reset soft MMU for next block (it can currently
                    only be set by a memory fault) */
             } /* for(;;) */
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V2 4/5] icount_warp: Take into account initial offset between clocks

[Sebastian Tanase]

On some target architectures (for example x86)  the virtual and real
clocks are not aligned at startup so when qemu_icount_bias is first updated
we have to take into account the offset between the 2 clocks to
properly compute if the guest is ahead or behind the host.
On architectures where we don't have this problem (for example
ARM) the offset will always be 0.

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
---
 cpu-exec.c            |  2 +-
 cpus.c                | 11 +++++++++++
 include/qemu-common.h |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index ccc2c0e..5a3bc5e 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -343,7 +343,7 @@ int cpu_exec(CPUArchState *env)
            what we have to do is sleep until it is 0. As for the
            advance/delay we gain here, we try to fix it next time. */
         diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
-                   qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+                   qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + clocks_offset;
         original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
     }
 #endif
diff --git a/cpus.c b/cpus.c
index bbe8743..818231b 100644
--- a/cpus.c
+++ b/cpus.c
@@ -62,6 +62,7 @@
 #endif /* CONFIG_LINUX */
 
 static CPUState *next_cpu;
+int64_t clocks_offset = -1;
 
 bool cpu_is_stopped(CPUState *cpu)
 {
@@ -336,6 +337,16 @@ static void icount_warp_rt(void *opaque)
             warp_delta = MIN(warp_delta, delta);
         }
         qemu_icount_bias += warp_delta;
+        /* On x86 target architecture, the PIT reset function (called
+           by qemu_system_reset) will end up calling qemu_clock_warp
+           and then icount_warp_rt changing vm_clock_warp_start from 0 (initial
+           value) to -1. This in turn will make us skip the initial offset
+           between the real and virtual clocks (initially virtual clock is 0).
+           Therefore we save it in clocks_offset. On ARM, we don't have this
+           problem and clocks_offset will be 0. */
+        if (clocks_offset == -1 && icount_align_option) {
+            clocks_offset = vm_clock_warp_start;
+        }
     }
     vm_clock_warp_start = -1;
     seqlock_write_unlock(&timers_state.vm_clock_seqlock);
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 60478fa..fb2aab7 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -110,6 +110,7 @@ void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
 extern int icount_time_shift;
 extern int icount_align_option;
+extern int64_t clocks_offset;
 
 #include "qemu/osdep.h"
 #include "qemu/bswap.h"
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V2 5/5] cpu_exec: Print to console if the guest is late

[Sebastian Tanase]

If the align option is enabled, we print to the user whenever
the guest clock is behind the host clock in order for he/she
to have a hint about the actual performance. The maximum
print interval is 2s so as not to spam the console.

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
---
 cpu-exec.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 5a3bc5e..a386592 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -216,6 +216,8 @@ static void cpu_handle_debug_exception(CPUArchState *env)
  * oscillate around 0.
  */
 #define VM_CLOCK_ADVANCE 3000000
+#define THRESHOLD_REDUCE 1.5
+#define MAX_DELAY_PRINT_RATE 2
 
 static int64_t delay_host(int64_t diff_clk)
 {
@@ -257,6 +259,29 @@ static int64_t align_clocks(int64_t diff_clk, int64_t *oic, CPUState *cpu)
     return diff_clk;
 }
 
+static void print_delay(int64_t diff_clk, int64_t realtime_clock)
+{
+    static float threshold_delay;
+    static int64_t last_realtime_clock;
+
+    if ((realtime_clock - last_realtime_clock) / 1000000000LL
+        >= MAX_DELAY_PRINT_RATE) {
+        if (-diff_clk / (float)1000000000LL > threshold_delay) {
+            threshold_delay = (-diff_clk / 1000000000LL) + 1;
+            printf("Warning: The guest is late by %.1f to %.1f seconds\n",
+                   threshold_delay - 1,
+                   threshold_delay);
+        } else if (-diff_clk / (float)1000000000LL <
+                   (threshold_delay - THRESHOLD_REDUCE)) {
+            threshold_delay = (-diff_clk / 1000000000LL) + 1;
+                   printf("Warning: The guest has reduced the delay and is now "
+                   "late by %.1f to %.1f seconds\n",
+                   threshold_delay - 1,
+                   threshold_delay);
+        }
+        last_realtime_clock = realtime_clock;
+    }
+}
 #endif /* CONFIG USER ONLY */
 
 /* main execution loop */
@@ -278,6 +303,8 @@ int cpu_exec(CPUArchState *env)
     uint8_t *tc_ptr;
     uintptr_t next_tb;
 #if !(defined(CONFIG_USER_ONLY))
+    /* Print delay control */
+    static int64_t realtime_clock;
     /* Delay algorithm */
     int64_t diff_clk, original_instr_counter;
 #endif
@@ -342,9 +369,12 @@ int cpu_exec(CPUArchState *env)
            This delay includes the delay of the last cycle, so
            what we have to do is sleep until it is 0. As for the
            advance/delay we gain here, we try to fix it next time. */
+        realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
         diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
-                   qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + clocks_offset;
+                   realtime_clock + clocks_offset;
         original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
+        /* Print (every 2s max) if the guest is behind the host */
+        print_delay(diff_clk, realtime_clock);
     }
 #endif
     /* prepare setjmp context for exception handling */
-- 
2.0.0.rc2

Re: [Qemu-devel] [RFC PATCH V2 1/5] icount: Add 'align' and 'icount' options

[Paolo Bonzini]

Il 13/06/2014 11:40, Sebastian Tanase ha scritto:
> The align option is used for activating the align algorithm
> in order to synchronise the host clock and the guest clock.
> Therefore we slightly modified the existing icount option.
> Thus, the new way to specify an icount is
> '-icount [icount=][N|auto][,align]'
>
> Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
> Tested-by: Camille Bégué <camille.begue@openwide.fr>
> ---
>  cpus.c                    | 19 ++++++++++++++++++-
>  include/qapi/qmp/qerror.h |  9 +++++++++
>  include/qemu-common.h     |  4 +++-
>  qemu-options.hx           | 19 ++++++++++++++++---
>  qtest.c                   | 19 +++++++++++++++++--
>  vl.c                      | 43 ++++++++++++++++++++++++++++++++++++-------
>  6 files changed, 99 insertions(+), 14 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index dd7ac13..b78a418 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -438,11 +438,24 @@ static const VMStateDescription vmstate_timers = {
>      }
>  };
>
> -void configure_icount(const char *option)
> +void configure_icount(QemuOpts *opts, Error **errp)
>  {
> +    const char *option;
> +
>      seqlock_init(&timers_state.vm_clock_seqlock, NULL);
>      vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
> +    option = qemu_opt_get(opts, "icount");
> +    icount_align_option = qemu_opt_get_bool(opts, "align", false);
> +    /* When using -icount, it is mandatory to specify an icount value:
> +       -icount icount=N|auto */
>      if (!option) {
> +        error_set(errp, QERR_ICOUNT_MISSING);
> +        return;
> +    }
> +    /* When using -icount icount, the icount option will be
> +       misinterpreted as a boolean */
> +    if (strcmp(option, "on") == 0 || strcmp(option, "off") == 0) {
> +        error_set(errp, QERR_INVALID_ICOUNT);
>          return;
>      }
>
> @@ -452,6 +465,10 @@ void configure_icount(const char *option)
>          icount_time_shift = strtol(option, NULL, 0);
>          use_icount = 1;
>          return;
> +    } else if (icount_align_option) {
> +        /* Do not allow icount auto and align on */
> +        error_set(errp, QERR_INVALID_ALIGN, option);

There's no need to define new QERRs anymore (unfortunately a lot of code 
does that stil).  You can use error_setg and include the error string here.

> +        return;
>      }
>
>      use_icount = 2;
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 902d1a7..ac307d0 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -91,6 +91,15 @@ void qerror_report_err(Error *err);
>  #define QERR_FEATURE_DISABLED \
>      ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>
> +#define QERR_ICOUNT_MISSING \
> +    ERROR_CLASS_GENERIC_ERROR, "Icount option missing"
> +
> +#define QERR_INVALID_ALIGN \
> +    ERROR_CLASS_GENERIC_ERROR, "Icount %s and align on are incompatible"
> +
> +#define QERR_INVALID_ICOUNT \
> +    ERROR_CLASS_GENERIC_ERROR, "Icount must be a number or auto"
> +
>  #define QERR_INVALID_BLOCK_FORMAT \
>      ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 66ceceb..288b460 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -41,6 +41,7 @@
>  #include <assert.h>
>  #include <signal.h>
>  #include "glib-compat.h"
> +#include "qemu/option.h"
>
>  #ifdef _WIN32
>  #include "sysemu/os-win32.h"
> @@ -105,8 +106,9 @@ static inline char *realpath(const char *path, char *resolved_path)
>  #endif
>
>  /* icount */
> -void configure_icount(const char *option);
> +void configure_icount(QemuOpts *opts, Error **errp);
>  extern int use_icount;
> +extern int icount_align_option;
>
>  #include "qemu/osdep.h"
>  #include "qemu/bswap.h"
> diff --git a/qemu-options.hx b/qemu-options.hx
> index d0714c4..5e7c956 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2898,11 +2898,11 @@ re-inject them.
>  ETEXI
>
>  DEF("icount", HAS_ARG, QEMU_OPTION_icount, \
> -    "-icount [N|auto]\n" \
> +    "-icount [icount=N|auto][,align=on|off]\n" \
>      "                enable virtual instruction counter with 2^N clock ticks per\n" \
> -    "                instruction\n", QEMU_ARCH_ALL)
> +    "                instruction and enable aligning the host and virtual clocks\n", QEMU_ARCH_ALL)
>  STEXI
> -@item -icount [@var{N}|auto]
> +@item -icount [icount=@var{N}|auto][,align=on|off]
>  @findex -icount
>  Enable virtual instruction counter.  The virtual cpu will execute one
>  instruction every 2^@var{N} ns of virtual time.  If @code{auto} is specified
> @@ -2913,6 +2913,19 @@ Note that while this option can give deterministic behavior, it does not
>  provide cycle accurate emulation.  Modern CPUs contain superscalar out of
>  order cores with complex cache hierarchies.  The number of instructions
>  executed often has little or no correlation with actual performance.
> +
> +@option{align=on} will activate the delay algorithm which will try to
> +to synchronise the host clock and the virtual clock. The goal is to
> +have a guest running at the real frequency imposed by
> +icount. Whenever the guest clock is behind the host clock and if
> +@option{align=on} is specified then we print a messsage to the user
> +to inform about the delay.
> +You must provide a value for @option{icount} other than @code{auto}
> +for the align option to be taken into account.
> +Currently this option does not work with @code{auto}.
> +Note: The sync algorithm will work on those icounts on which
> +the guest clock runs ahead of the host clock. Typically this happens
> +when the icount value is high (how high depends on the host machine).
>  ETEXI
>
>  DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
> diff --git a/qtest.c b/qtest.c
> index 04a6dc1..9b6c29b 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -19,6 +19,9 @@
>  #include "hw/irq.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpus.h"
> +#include "qemu/config-file.h"
> +#include "qemu/option.h"
> +#include "qemu/error-report.h"
>
>  #define MAX_IRQ 256
>
> @@ -509,10 +512,22 @@ static void qtest_event(void *opaque, int event)
>      }
>  }
>
> -int qtest_init_accel(MachineClass *mc)
> +static void configure_qtest_icount(const char *options)
>  {
> -    configure_icount("0");
> +    QemuOpts *opts  = qemu_opts_parse(qemu_find_opts("icount"), options, 0);
> +    Error *errp = NULL;
> +    configure_icount(opts, &errp);

You can just pass &error_abort here.

> +    qemu_opts_del(opts);
> +    if (errp != NULL) {
> +        error_report("%s", error_get_pretty(errp));
> +        error_free(errp);
> +        exit(1);
> +    }
> +}
>
> +int qtest_init_accel(MachineClass *mc)
> +{
> +    configure_qtest_icount("icount=0");

No need to add "icount=" here if...

>      return 0;
>  }
>
> diff --git a/vl.c b/vl.c
> index ac0e3d7..1bd467a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -182,6 +182,7 @@ uint8_t *boot_splash_filedata;
>  size_t boot_splash_filedata_size;
>  uint8_t qemu_extra_params_fw[2];
>
> +int icount_align_option;
>  typedef struct FWBootEntry FWBootEntry;
>
>  struct FWBootEntry {
> @@ -524,6 +525,21 @@ static QemuOptsList qemu_mem_opts = {
>      },
>  };
>
> +static QemuOptsList qemu_icount_opts = {
> +    .name = "icount",

... you add this here:

     .implied_opt_name = "icount",

You will also need:

     .merge_lists = true,

so that "-icount 0 -icount align=on" also works.

In fact, what could be a better name for the implied suboption?  I 
thought of speed and shift.  "speed" is more understandable while 
"shift" is more accurate and matches the code.

Otherwise the idea is okay.

Thanks!

Paolo

> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_icount_opts.head),
> +    .desc = {
> +        {
> +            .name = "icount",
> +            .type = QEMU_OPT_STRING,
> +        }, {
> +            .name = "align",
> +            .type = QEMU_OPT_BOOL,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  /**
>   * Get machine options
>   *
> @@ -2957,13 +2973,12 @@ int main(int argc, char **argv, char **envp)
>  {
>      int i;
>      int snapshot, linux_boot;
> -    const char *icount_option = NULL;
>      const char *initrd_filename;
>      const char *kernel_filename, *kernel_cmdline;
>      const char *boot_order;
>      DisplayState *ds;
>      int cyls, heads, secs, translation;
> -    QemuOpts *hda_opts = NULL, *opts, *machine_opts;
> +    QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
>      QemuOptsList *olist;
>      int optind;
>      const char *optarg;
> @@ -2991,6 +3006,7 @@ int main(int argc, char **argv, char **envp)
>      const char *trace_file = NULL;
>      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>                                          1024 * 1024;
> +    Error *icount_err = NULL;
>
>      atexit(qemu_run_exit_notifiers);
>      error_set_progname(argv[0]);
> @@ -3024,6 +3040,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_realtime_opts);
>      qemu_add_opts(&qemu_msg_opts);
>      qemu_add_opts(&qemu_name_opts);
> +    qemu_add_opts(&qemu_icount_opts);
>
>      runstate_init();
>
> @@ -3833,7 +3850,11 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_icount:
> -                icount_option = optarg;
> +                icount_opts = qemu_opts_parse(qemu_find_opts("icount"),
> +                                              optarg, 0);
> +                if (!icount_opts) {
> +                    exit(1);
> +                }
>                  break;
>              case QEMU_OPTION_incoming:
>                  incoming = optarg;
> @@ -4301,11 +4322,19 @@ int main(int argc, char **argv, char **envp)
>      qemu_spice_init();
>  #endif
>
> -    if (icount_option && (kvm_enabled() || xen_enabled())) {
> -        fprintf(stderr, "-icount is not allowed with kvm or xen\n");
> -        exit(1);
> +    if (icount_opts) {
> +        if (kvm_enabled() || xen_enabled()) {
> +            fprintf(stderr, "-icount is not allowed with kvm or xen\n");
> +            exit(1);
> +        }
> +        configure_icount(icount_opts, &icount_err);
> +        qemu_opts_del(icount_opts);
> +        if (icount_err != NULL) {
> +            error_report("%s", error_get_pretty(icount_err));
> +            error_free(icount_err);
> +            exit(1);
> +        }
>      }
> -    configure_icount(icount_option);
>
>      /* clean up network at qemu process termination */
>      atexit(&net_cleanup);
>

Re: [Qemu-devel] [RFC PATCH V2 3/5] cpu_exec: Add sleeping algorithm

[Paolo Bonzini]

Il 13/06/2014 11:40, 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.
>
> Basically, using QEMU_CLOCK_REALTIME, we calculate
> the real time duration of the execution (meaning
> generating TBs and executing them) and using the
> the fields icount_decr.u16.low and icount_extra
> (from the CPUState structure) shifted by icount_time_shift
> we calculate the theoretical virtual time elapsed.
> Having these 2 values, we can determine if the
> guest is in advance and sleep.
>
> Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
> Tested-by: Camille Bégué <camille.begue@openwide.fr>
> ---
>
> 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).

You can place this text in the commit message.

> ---
>  cpu-exec.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 38e5f02..ccc2c0e 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -22,6 +22,7 @@
>  #include "tcg.h"
>  #include "qemu/atomic.h"
>  #include "sysemu/qtest.h"
> +#include "qemu/timer.h"
>
>  void cpu_loop_exit(CPUState *cpu)
>  {
> @@ -209,6 +210,55 @@ static void cpu_handle_debug_exception(CPUArchState *env)
>      }
>  }
>
> +#if !(defined(CONFIG_USER_ONLY))

No need for ( ) here around defined.

> +/* 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)
> +{
> +    struct timespec sleep_delay, rem_delay;
> +    if (diff_clk > VM_CLOCK_ADVANCE) {
> +        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;
> +        }
> +    }
> +    return diff_clk;
> +}
> +
> +static int64_t update_clock_difference(int64_t diff_clk,
> +                                    int64_t *instr_counter,
> +                                    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;
> +    *instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
> +
> +    return diff_clk + instr_exec_time;
> +}
> +
> +static int64_t align_clocks(int64_t diff_clk, int64_t *oic, CPUState *cpu)
> +{
> +    if (icount_align_option) {
> +        diff_clk = update_clock_difference(diff_clk, oic, cpu);
> +        diff_clk = delay_host(diff_clk);
> +    }
> +
> +    return diff_clk;
> +}
> +#endif /* CONFIG USER ONLY */
> +
>  /* main execution loop */
>
>  volatile sig_atomic_t exit_request;
> @@ -227,6 +277,10 @@ int cpu_exec(CPUArchState *env)
>      TranslationBlock *tb;
>      uint8_t *tc_ptr;
>      uintptr_t next_tb;
> +#if !(defined(CONFIG_USER_ONLY))
> +    /* Delay algorithm */
> +    int64_t diff_clk, original_instr_counter;
> +#endif

Perhaps these could become a struct too (an abstract data type with init 
and realign operations), with a dummy implementation for 
CONFIG_USER_ONLY.  The compiler will remove the dead code.

>      /* This must be volatile so it is not trashed by longjmp() */
>      volatile bool have_tb_lock = false;
>
> @@ -282,7 +336,17 @@ int cpu_exec(CPUArchState *env)
>  #error unsupported target CPU
>  #endif
>      cpu->exception_index = -1;
> -
> +#if !(defined(CONFIG_USER_ONLY))
> +    if (icount_align_option) {
> +        /* Calculate difference between guest clock and host clock.
> +           This delay includes the delay of the last cycle, so
> +           what we have to do is sleep until it is 0. As for the
> +           advance/delay we gain here, we try to fix it next time. */
> +        diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
> +                   qemu_clock_get_ns(QEMU_CLOCK_REALTIME);

This doesn't work yet, does it?

Let's look for a less hacky solution to the problem you fix in patch 4, 
and squash it in this patch.

Paolo

> +        original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
> +    }
> +#endif
>      /* prepare setjmp context for exception handling */
>      for(;;) {
>          if (sigsetjmp(cpu->jmp_env, 0) == 0) {
> @@ -672,6 +736,13 @@ int cpu_exec(CPUArchState *env)
>                              if (insns_left > 0) {
>                                  /* Execute remaining instructions.  */
>                                  cpu_exec_nocache(env, insns_left, tb);
> +#if !(defined(CONFIG_USER_ONLY))
> +                                /* We can ignore the return value
> +                                   because we exit anyway. */
> +                                align_clocks(diff_clk,
> +                                             &original_instr_counter,
> +                                             cpu);
> +#endif
>                              }
>                              cpu->exception_index = EXCP_INTERRUPT;
>                              next_tb = 0;
> @@ -684,6 +755,11 @@ int cpu_exec(CPUArchState *env)
>                      }
>                  }
>                  cpu->current_tb = NULL;
> +#if !(defined(CONFIG_USER_ONLY))
> +                diff_clk = align_clocks(diff_clk,
> +                                        &original_instr_counter,
> +                                        cpu);
> +#endif
>                  /* reset soft MMU for next block (it can currently
>                     only be set by a memory fault) */
>              } /* for(;;) */
>

Re: [Qemu-devel] [RFC PATCH V2 4/5] icount_warp: Take into account initial offset between clocks

[Paolo Bonzini]

Il 13/06/2014 11:40, Sebastian Tanase ha scritto:
> +        /* On x86 target architecture, the PIT reset function (called
> +           by qemu_system_reset) will end up calling qemu_clock_warp
> +           and then icount_warp_rt changing vm_clock_warp_start from 0 (initial
> +           value) to -1. This in turn will make us skip the initial offset
> +           between the real and virtual clocks (initially virtual clock is 0).
> +           Therefore we save it in clocks_offset. On ARM, we don't have this
> +           problem and clocks_offset will be 0. */
> +        if (clocks_offset == -1 && icount_align_option) {
> +            clocks_offset = vm_clock_warp_start;
> +        }

Can we instead just do

clocks_offset = qemu_get_clock(QEMU_CLOCK_REALTIME);

in qemu_tcg_init_vcpu?

Paolo

Re: [Qemu-devel] [RFC PATCH V2 5/5] cpu_exec: Print to console if the guest is late

[Paolo Bonzini]

Il 13/06/2014 11:40, Sebastian Tanase ha scritto:
> If the align option is enabled, we print to the user whenever
> the guest clock is behind the host clock in order for he/she
> to have a hint about the actual performance. The maximum
> print interval is 2s so as not to spam the console.
>
> Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
> Tested-by: Camille Bégué <camille.begue@openwide.fr>
> ---
>  cpu-exec.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 5a3bc5e..a386592 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -216,6 +216,8 @@ static void cpu_handle_debug_exception(CPUArchState *env)
>   * oscillate around 0.
>   */
>  #define VM_CLOCK_ADVANCE 3000000
> +#define THRESHOLD_REDUCE 1.5
> +#define MAX_DELAY_PRINT_RATE 2
>
>  static int64_t delay_host(int64_t diff_clk)
>  {
> @@ -257,6 +259,29 @@ static int64_t align_clocks(int64_t diff_clk, int64_t *oic, CPUState *cpu)
>      return diff_clk;
>  }
>
> +static void print_delay(int64_t diff_clk, int64_t realtime_clock)
> +{
> +    static float threshold_delay;
> +    static int64_t last_realtime_clock;
> +
> +    if ((realtime_clock - last_realtime_clock) / 1000000000LL
> +        >= MAX_DELAY_PRINT_RATE) {
> +        if (-diff_clk / (float)1000000000LL > threshold_delay) {
> +            threshold_delay = (-diff_clk / 1000000000LL) + 1;
> +            printf("Warning: The guest is late by %.1f to %.1f seconds\n",
> +                   threshold_delay - 1,
> +                   threshold_delay);
> +        } else if (-diff_clk / (float)1000000000LL <
> +                   (threshold_delay - THRESHOLD_REDUCE)) {
> +            threshold_delay = (-diff_clk / 1000000000LL) + 1;
> +                   printf("Warning: The guest has reduced the delay and is now "
> +                   "late by %.1f to %.1f seconds\n",
> +                   threshold_delay - 1,
> +                   threshold_delay);
> +        }
> +        last_realtime_clock = realtime_clock;
> +    }
> +}

Perhaps stop printing the message after the first 50-100 times?

Paolo

>  #endif /* CONFIG USER ONLY */
>
>  /* main execution loop */
> @@ -278,6 +303,8 @@ int cpu_exec(CPUArchState *env)
>      uint8_t *tc_ptr;
>      uintptr_t next_tb;
>  #if !(defined(CONFIG_USER_ONLY))
> +    /* Print delay control */
> +    static int64_t realtime_clock;
>      /* Delay algorithm */
>      int64_t diff_clk, original_instr_counter;
>  #endif
> @@ -342,9 +369,12 @@ int cpu_exec(CPUArchState *env)
>             This delay includes the delay of the last cycle, so
>             what we have to do is sleep until it is 0. As for the
>             advance/delay we gain here, we try to fix it next time. */
> +        realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>          diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
> -                   qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + clocks_offset;
> +                   realtime_clock + clocks_offset;
>          original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
> +        /* Print (every 2s max) if the guest is behind the host */
> +        print_delay(diff_clk, realtime_clock);
>      }
>  #endif
>      /* prepare setjmp context for exception handling */
>

Re: [Qemu-devel] [RFC PATCH V2 0/5] icount: Implement delay algorithm between guest and host clocks

[Paolo Bonzini]

Il 13/06/2014 11:40, Sebastian Tanase ha scritto:
> * Signal errors in configure_icount() as suggested by Paolo Bonzini (patch 1).
> * Update qtest.c to take into account the new configure_icount() (patch 1).
> * Surround delay code in cpu-exec.c with !defined(CONFIG_USER_ONLY) (patch 3).
> * Clean up delay code in cpu-exec.c as suggested by Paolo Bonzini
>   and Juan Quintela (patch 3).
> * Take into account initial offset between real and virtual clocks (patch 4).

Looks great, thanks!  I pointed out a few nits, but I'm confident that 
we will be able to merge v3.

Paolo

> Sebastian Tanase (5):
>   icount: Add 'align' and 'icount' options
>   icount: Make icount_time_shift available everywhere
>   cpu_exec: Add sleeping algorithm
>   icount_warp: Take into account initial offset between clocks
>   cpu_exec: Print to console if the guest is late
>
>  cpu-exec.c                | 108 +++++++++++++++++++++++++++++++++++++++++++++-
>  cpus.c                    |  38 ++++++++++++++--
>  include/qapi/qmp/qerror.h |   9 ++++
>  include/qemu-common.h     |   6 ++-
>  qemu-options.hx           |  19 ++++++--
>  qtest.c                   |  19 +++++++-
>  vl.c                      |  43 +++++++++++++++---
>  7 files changed, 225 insertions(+), 17 deletions(-)
>

Re: [Qemu-devel] [RFC PATCH V2 5/5] cpu_exec: Print to console if the guest is late

[Sebastian Tanase]

----- Mail original -----
> De: "Paolo Bonzini" <pbonzini@redhat.com>
> À: "Sebastian Tanase" <sebastian.tanase@openwide.fr>, qemu-devel@nongnu.org
> Cc: aliguori@amazon.com, afaerber@suse.de, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>,
> michael@walle.cc, alex@alex.org.uk, stefanha@redhat.com, lcapitulino@redhat.com, crobinso@redhat.com,
> armbru@redhat.com, wenchaoqemu@gmail.com, quintela@redhat.com, kwolf@redhat.com, mjt@tls.msk.ru, mst@redhat.com,
> "camille begue" <camille.begue@openwide.fr>, "pierre lemagourou" <pierre.lemagourou@openwide.fr>, "jeremy rosen"
> <jeremy.rosen@openwide.fr>
> Envoyé: Vendredi 13 Juin 2014 12:29:36
> Objet: Re: [RFC PATCH V2 5/5] cpu_exec: Print to console if the guest is late
> 
> Il 13/06/2014 11:40, Sebastian Tanase ha scritto:
> > If the align option is enabled, we print to the user whenever
> > the guest clock is behind the host clock in order for he/she
> > to have a hint about the actual performance. The maximum
> > print interval is 2s so as not to spam the console.
> >
> > Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
> > Tested-by: Camille Bégué <camille.begue@openwide.fr>
> > ---
> >  cpu-exec.c | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 5a3bc5e..a386592 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -216,6 +216,8 @@ static void
> > cpu_handle_debug_exception(CPUArchState *env)
> >   * oscillate around 0.
> >   */
> >  #define VM_CLOCK_ADVANCE 3000000
> > +#define THRESHOLD_REDUCE 1.5
> > +#define MAX_DELAY_PRINT_RATE 2
> >
> >  static int64_t delay_host(int64_t diff_clk)
> >  {
> > @@ -257,6 +259,29 @@ static int64_t align_clocks(int64_t diff_clk,
> > int64_t *oic, CPUState *cpu)
> >      return diff_clk;
> >  }
> >
> > +static void print_delay(int64_t diff_clk, int64_t realtime_clock)
> > +{
> > +    static float threshold_delay;
> > +    static int64_t last_realtime_clock;
> > +
> > +    if ((realtime_clock - last_realtime_clock) / 1000000000LL
> > +        >= MAX_DELAY_PRINT_RATE) {
> > +        if (-diff_clk / (float)1000000000LL > threshold_delay) {
> > +            threshold_delay = (-diff_clk / 1000000000LL) + 1;
> > +            printf("Warning: The guest is late by %.1f to %.1f
> > seconds\n",
> > +                   threshold_delay - 1,
> > +                   threshold_delay);
> > +        } else if (-diff_clk / (float)1000000000LL <
> > +                   (threshold_delay - THRESHOLD_REDUCE)) {
> > +            threshold_delay = (-diff_clk / 1000000000LL) + 1;
> > +                   printf("Warning: The guest has reduced the
> > delay and is now "
> > +                   "late by %.1f to %.1f seconds\n",
> > +                   threshold_delay - 1,
> > +                   threshold_delay);
> > +        }
> > +        last_realtime_clock = realtime_clock;
> > +    }
> > +}
> 
> Perhaps stop printing the message after the first 50-100 times?
> 
> Paolo
> 

Thank you very much for your feedback.
Maybe adding a monitor command (info drift) would be better ? Then
the user would not be spammed by printfs.

Sebastian Tanase

> >  #endif /* CONFIG USER ONLY */
> >
> >  /* main execution loop */
> > @@ -278,6 +303,8 @@ int cpu_exec(CPUArchState *env)
> >      uint8_t *tc_ptr;
> >      uintptr_t next_tb;
> >  #if !(defined(CONFIG_USER_ONLY))
> > +    /* Print delay control */
> > +    static int64_t realtime_clock;
> >      /* Delay algorithm */
> >      int64_t diff_clk, original_instr_counter;
> >  #endif
> > @@ -342,9 +369,12 @@ int cpu_exec(CPUArchState *env)
> >             This delay includes the delay of the last cycle, so
> >             what we have to do is sleep until it is 0. As for the
> >             advance/delay we gain here, we try to fix it next time.
> >             */
> > +        realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> >          diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
> > -                   qemu_clock_get_ns(QEMU_CLOCK_REALTIME) +
> > clocks_offset;
> > +                   realtime_clock + clocks_offset;
> >          original_instr_counter = cpu->icount_extra +
> >          cpu->icount_decr.u16.low;
> > +        /* Print (every 2s max) if the guest is behind the host */
> > +        print_delay(diff_clk, realtime_clock);
> >      }
> >  #endif
> >      /* prepare setjmp context for exception handling */
> >
> 
> 

Re: [Qemu-devel] [RFC PATCH V2 5/5] cpu_exec: Print to console if the guest is late

[Paolo Bonzini]

Il 13/06/2014 14:00, Sebastian Tanase ha scritto:
>> >
>> > Perhaps stop printing the message after the first 50-100 times?
>> >
>> > Paolo
>> >
> Thank you very much for your feedback.
> Maybe adding a monitor command (info drift) would be better ? Then
> the user would not be spammed by printfs.

I think some spamming is okay, as long as it's not going on forever. 
Perhaps even 10 times is enough (I'm not going to ask you to introduce a 
rate limiting like printk's :)).

Adding this to the monitor can be useful even separately from the stderr 
spam.  Perhaps you can add the current drift to "info jit" if you feel 
it is useful.  You are user #0 of the feature, so your guess is better 
than mine.

Paolo