From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36702) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9Wpr-0007ne-9f for qemu-devel@nongnu.org; Tue, 22 Jul 2014 05:58:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X9Wpm-0004Zx-Qo for qemu-devel@nongnu.org; Tue, 22 Jul 2014 05:58:15 -0400 Received: from zimbra3.corp.accelance.fr ([2001:4080:204::2:8]:42740) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9Wpm-0004Zk-I5 for qemu-devel@nongnu.org; Tue, 22 Jul 2014 05:58:10 -0400 Date: Tue, 22 Jul 2014 11:58:04 +0200 (CEST) From: Sebastian Tanase Message-ID: <601871010.20021638.1406023084319.JavaMail.root@openwide.fr> In-Reply-To: <53C67BA9.50009@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, peter maydell , aliguori@amazon.com, wenchaoqemu@gmail.com, quintela@redhat.com, qemu-devel@nongnu.org, mst@redhat.com, stefanha@redhat.com, armbru@redhat.com, lcapitulino@redhat.com, michael@walle.cc, camille begue , alex@alex.org.uk, crobinso@redhat.com, afaerber@suse.de, rth@twiddle.net ----- Mail original ----- > De: "Paolo Bonzini" > =C3=80: "Sebastian Tanase" , qemu-devel@non= gnu.org > Cc: aliguori@amazon.com, afaerber@suse.de, rth@twiddle.net, "peter maydel= l" , > michael@walle.cc, alex@alex.org.uk, stefanha@redhat.com, lcapitulino@redh= at.com, crobinso@redhat.com, > armbru@redhat.com, wenchaoqemu@gmail.com, quintela@redhat.com, kwolf@redh= at.com, mst@redhat.com, "camille begue" > > Envoy=C3=A9: Mercredi 16 Juillet 2014 15:18:33 > Objet: Re: [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit' >=20 > Il 16/07/2014 14:18, Sebastian Tanase ha scritto: > > - static int64_t clocks_offset; > > - if (!icount_align_option) { > > - return; > > + static int64_t realtime_clock_value; >=20 > Does this really need to be static? >=20 > > + if (icount_align_option || !realtime_clock_value) { > > + realtime_clock_value =3D > > qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > > } > > - sc->realtime_clock =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > > /* Compute offset between the 2 clocks. */ > > if (!clocks_offset) { > > - clocks_offset =3D sc->realtime_clock - > > + clocks_offset =3D realtime_clock_value - > > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > } >=20 > Isn't clocks_offset roughly the same as > -timers_state.cpu_clock_offset? > If so, you could be some simplification in the code. Feel free to > move the TimersState struct definition to include/sysemu/cpus.h and > make > timers_state public. -timers_state.cpu_clock_offset contains the offset between the real and vir= tual clocks. However, when using the value of the virtual clock (qemu_clock_get_ns(QEMU_= CLOCK_VIRTUAL)), qemu_icount_bias already includes this offset because, on ARM, qemu_clock_w= arp (which=20 then calls icount_warp_rt) is called for the first time in tcg_exec_all, ma= king qemu_icount_bias take the value of qemu_clock_get_ns(QEMU_CLOCK_REALTIME) static void icount_warp_rt(void *opaque) { if (atomic_read(&vm_clock_warp_start) =3D=3D -1) { return; } seqlock_write_lock(&timers_state.vm_clock_seqlock); if (runstate_is_running()) { int64_t clock =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); int64_t warp_delta; ... warp_delta =3D clock - vm_clock_warp_start; //vm_clock_warp_sta= rt is 0 the very first time qemu_icount_bias +=3D warp_delta; } vm_clock_warp_start =3D -1; seqlock_write_unlock(&timers_state.vm_clock_seqlock); if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { qemu_clock_notify(QEMU_CLOCK_VIRTUAL); } } Also, the first time qemu_clock_warp -> icount_warp_rt are called (on ARM),= qemu_clock_expired(QEMU_CLOCK_VIRTUAL)=20 is false because there are no active timers on the vm clock timer list; I'l= l explain why I bring this up below. A solution to not compute the initial offset in qemu_icount_bias would be t= o initialize=20 vm_clock_warp_start to -1. The result will be that qemu_icount_bias will s= tart counting when the vcpu goes from active to inactive. At that time, vm_clock_warp_start wi= ll already store the realtime clock=20 value and a timer on the real clock will be set to expire at clock + deadli= ne, making qemu_icount_bias increment by deadline. A consequence of initializing vm_clock_warp_start to -1 is also=20 the fact that we'll skip the first check for a virtual expired timer. As I = mentioned above, in ARM case,=20 it's not dangerous because there are no timers active the first time we per= form this check. However, this=20 is just a potential scenario and I cannot guarantee that on other target ar= chitectures there won't be=20 an expired timer pending the first time we check. I also tested on x86: qemu_clock_warp -> icount_warp_rt are first called on "pit_reset" after a v= irtual timer is set to expire. However, in this case, the qemu_clock_expired(QEMU_CLOCK_VIRTUAL) fails bec= ause the current virtual clock is 0 so the timer doesn't have to expire yet. In this case also the above soluti= on would work without breaking anything. So, do you think it is worth taking this solution into account or it will c= ause more harm than good? Sebastian >=20 > > + cpu_fprintf(f, "Host - Guest clock %ld(ms)\n", > > + (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - > > clocks_offset - > > + qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL))/SCALE_MS); >=20 > I think this is (cpu_get_clock() - cpu_get_icount()) / SCALE_MS. >=20 > Paolo >=20