From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KxRiZ-0002lF-KP for qemu-devel@nongnu.org; Tue, 04 Nov 2008 14:37:35 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KxRiX-0002j2-FR for qemu-devel@nongnu.org; Tue, 04 Nov 2008 14:37:35 -0500 Received: from [199.232.76.173] (port=48877 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KxRiX-0002ie-6v for qemu-devel@nongnu.org; Tue, 04 Nov 2008 14:37:33 -0500 Received: from fmmailgate01.web.de ([217.72.192.221]:40153) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KxRiW-00044a-NW for qemu-devel@nongnu.org; Tue, 04 Nov 2008 14:37:33 -0500 Message-ID: <4910A478.7020304@web.de> Date: Tue, 04 Nov 2008 20:37:28 +0100 From: Jan Kiszka MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] Fix alarm_timer race with select References: <490FFF23.5020704@web.de> <49105704.9070708@codemonkey.ws> <49105BE7.6030505@codemonkey.ws> <49108333.6080807@web.de> <49109F8A.8080903@codemonkey.ws> In-Reply-To: <49109F8A.8080903@codemonkey.ws> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig45972A891B8A3BDDC8C2140E" Sender: jan.kiszka@web.de Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig45972A891B8A3BDDC8C2140E Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Anthony Liguori wrote: > Jan Kiszka wrote: >> Anthony Liguori wrote: >> =20 >>> Anthony Liguori wrote: >>> =20 >>>> Jan Kiszka wrote: >>>> >>>> Perhaps we should move the alarm timer check rearming out of the mai= n >>>> loop and into a qemu_set_fd_handler2() handler? >>>> =20 >>> And now that I think about it, I see no reason why the timer expirati= on >>> checks couldn't be moved to the same handler. I'd have to look more >>> closely at how that would interact with icount though. >>> =20 >> >> I cannot yet follow you here. But I my impression is that this will be= >> an additional change that should come in a separate patch, no? >> >> Find the O_NONBLOCK remark addressed below. >> >> Jan >> >> ------------> >> >> Changing the default IO timeout to 5 s (#5578) made a race visible >> between the alarm_timer and select() in main_loop_wait(): If the timer= >> fired before select was able to block, the full select() timeout could= >> have been applied instead of returning immediately. Since #5578, this >> causes heavy problems to the Musicpal board emulation with stalls up t= o >> 5 s. >> >> The following patch introduces a pipe that is written to by >> host_alarm_handler and select()'ed in main_loop_wait(). This avoids >> prevents that select() blocks though a timer has fired and waits for >> processing. >> >> Signed-off-by: Jan Kiszka >> --- >> vl.c | 21 ++++++++++++++++++++- >> 1 file changed, 20 insertions(+), 1 deletion(-) >> >> Index: b/vl.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- a/vl.c >> +++ b/vl.c >> @@ -884,6 +884,7 @@ static void qemu_rearm_alarm_timer(struc >> #define MIN_TIMER_REARM_US 250 >> =20 >> static struct qemu_alarm_timer *alarm_timer; >> +static int alarm_timer_rfd, alarm_timer_wfd; >> =20 >> #ifdef _WIN32 >> =20 >> @@ -1303,12 +1304,15 @@ static void host_alarm_handler(int host_ >> qemu_get_clock(vm_clock))) || >> qemu_timer_expired(active_timers[QEMU_TIMER_REALTIME], >> qemu_get_clock(rt_clock))) { >> + CPUState *env =3D next_cpu; >> + char byte =3D 0; >> + >> #ifdef _WIN32 >> struct qemu_alarm_win32 *data =3D ((struct >> qemu_alarm_timer*)dwUser)->priv; >> SetEvent(data->host_alarm); >> #endif >> - CPUState *env =3D next_cpu; >> =20 >> + write(alarm_timer_wfd, &byte, sizeof(byte)); >> alarm_timer->flags |=3D ALARM_FLAG_EXPIRED; >> =20 >> if (env) { >> @@ -1673,6 +1677,15 @@ static void init_timer_alarm(void) >> { >> struct qemu_alarm_timer *t =3D NULL; >> int i, err =3D -1; >> + int fds[2]; >> + >> + if (pipe(fds) || fcntl(fds[0], F_SETFL, O_NONBLOCK) >> + || fcntl(fds[1], F_SETFL, O_NONBLOCK)) { >> + perror("creating timer pipe"); >> + exit(1); >> + } >> + alarm_timer_rfd =3D fds[0]; >> + alarm_timer_wfd =3D fds[1]; >> =20 >> for (i =3D 0; alarm_timers[i].name; i++) { >> t =3D &alarm_timers[i]; >> @@ -4427,6 +4440,7 @@ void main_loop_wait(int timeout) >> /* XXX: separate device handlers from system ones */ >> nfds =3D -1; >> FD_ZERO(&rfds); >> + FD_SET(alarm_timer_rfd, &rfds); >> =20 >=20 > Sorry, I missed this in the first patch. You have to take > alarm_timer_rfd into account when computing max_fd otherwise badness co= uld > result. Oh, yes. > While you're at it, please don't call pipe and fcntl() twice > from inside of an if() clause. ??? Don't see your concern yet. Jan --------------enig45972A891B8A3BDDC8C2140E Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.8 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkkQpHgACgkQniDOoMHTA+lhDgCggurwBMN2s7ArU/rm6CG2xrGu 37YAn1IC7NXz0qadYnr1adIQSpQH0TMD =uyJs -----END PGP SIGNATURE----- --------------enig45972A891B8A3BDDC8C2140E--