From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51435) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XAcW0-0007zY-HI for qemu-devel@nongnu.org; Fri, 25 Jul 2014 06:14:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XAcVp-0007nc-An for qemu-devel@nongnu.org; Fri, 25 Jul 2014 06:14:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34463) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XAcVp-0007nK-2f for qemu-devel@nongnu.org; Fri, 25 Jul 2014 06:14:05 -0400 Message-ID: <53D22DD8.10601@redhat.com> Date: Fri, 25 Jul 2014 12:13:44 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1406282193-9664-1-git-send-email-sebastian.tanase@openwide.fr> <1406282193-9664-5-git-send-email-sebastian.tanase@openwide.fr> In-Reply-To: <1406282193-9664-5-git-send-email-sebastian.tanase@openwide.fr> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V5 4/6] cpu_exec: Add sleeping algorithm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sebastian Tanase , qemu-devel@nongnu.org Cc: kwolf@redhat.com, peter.maydell@linaro.org, jeremy.rosen@openwide.fr, aliguori@amazon.com, wenchaoqemu@gmail.com, quintela@redhat.com, mst@redhat.com, stefanha@redhat.com, armbru@redhat.com, lcapitulino@redhat.com, michael@walle.cc, camille.begue@openwide.fr, alex@alex.org.uk, crobinso@redhat.com, pierre.lemagourou@openwide.fr, afaerber@suse.de, rth@twiddle.net 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. >=20 > At first, we tried to approximate at each for loop the real time elapse= d > 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 del= ayed. > 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. >=20 > Therefore, we modified the algorithm and only take into account the > difference between the 2 clocks at the begining of the cpu_exec functio= n. > 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 overh= ead > 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. >=20 > 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 h= ost, which > is still acceptable for our use case. >=20 > 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 ar= m versatile-pb > built with buildroot. >=20 > Currently, on our test machine, the lowest icount we can achieve that i= s suitable for > aligning the 2 clocks is 6. However, we observe that the IO tests (usin= g fio) are > slower than the cpu tests (using stress). >=20 > Signed-off-by: Sebastian Tanase > Tested-by: Camille B=C3=A9gu=C3=A9 > Signed-off-by: Paolo Bonzini > --- > cpu-exec.c | 91 ++++++++++++++++++++++++++++++++++++++++++++= ++++++++ > cpus.c | 17 ++++++++++ > include/qemu/timer.h | 1 + > 3 files changed, 109 insertions(+) >=20 > 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 =3D diff_clk / 1000000000LL; > + sleep_delay.tv_nsec =3D diff_clk % 1000000000LL; > + if (nanosleep(&sleep_delay, &rem_delay) < 0) { > + diff_clk -=3D (sleep_delay.tv_sec - rem_delay.tv_sec) * 10= 00000000LL; > + diff_clk -=3D sleep_delay.tv_nsec - rem_delay.tv_nsec; > + } else { > + diff_clk =3D 0; > + } > +#else > + Sleep(diff_clk / SCALE_MS); > + diff_clk =3D 0; > +#endif > + } > + return diff_clk; > +} > + > +static int64_t instr_to_vtime(int64_t instr_counter, const CPUState *c= pu) > +{ > + int64_t instr_exec_time; > + instr_exec_time =3D instr_counter - > + (cpu->icount_extra + > + cpu->icount_decr.u16.low); > + instr_exec_time =3D 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 +=3D instr_to_vtime(sc->original_instr_counter, cpu); > + sc->original_instr_counter =3D cpu->icount_extra + cpu->icount_dec= r.u16.low; > + sc->diff_clk =3D 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 =3D cpu->icount_extra + cpu->icount_decr.u16.low; instr_exec_time =3D sc->original_instr_counter - instr_counter; sc->original_instr_counter =3D instr_counter sc->diff_clk +=3D instr_exec_time << icount_time_shift; sc->diff_clk =3D 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* tak= en? Paolo