From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47128) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ai2CF-0004eA-6y for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:56:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ai2CD-0000yO-Fs for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:56:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42657) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ai2CD-0000y8-AU for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:56:45 -0400 References: <56EEF805.8040008@freebsd.org> <87vb4grya5.fsf@linaro.org> <56F00ABC.9030700@freebsd.org> <87oaa7sv02.fsf@linaro.org> From: Paolo Bonzini Message-ID: <56F019B9.4080803@redhat.com> Date: Mon, 21 Mar 2016 16:56:41 +0100 MIME-Version: 1.0 In-Reply-To: <87oaa7sv02.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= , Sean Bruno Cc: Peter Maydell , QEMU Developers On 21/03/2016 16:36, Alex Benn=C3=A9e wrote: > > 341 /* The icount_warp_timer is rescheduled soon after vm_clock_= warp_start > > 342 * changes from -1 to another value, so the race here is oka= y. > > 343 */ > > 344 if (atomic_read(&vm_clock_warp_start) =3D=3D -1) { > > 345 return; > > 346 } > > 347 > Odd, the comments say that vm_clock_warp start is protected by the > seqlock, and in fact every other access to it is a plain access. Yes, the comment says why this is safe. The change from -1 to positive is here: if (vm_clock_warp_start =3D=3D -1 || vm_clock_warp_start > clock)= { vm_clock_warp_start =3D clock; } seqlock_write_unlock(&timers_state.vm_clock_seqlock); timer_mod_anticipate(icount_warp_timer, clock + deadline); If we get a race we must be like this: icount_warp_rt qemu_start_warp_timer -------------- --------------------- read -1 write to vm_clock_warp_start unlock timer_mod_anticipate (*) As soon as you reach (*) the timer is rescheduled and will read a value other than -1. > It seems to me the code should probably just be: >=20 > seqlock_write_lock(&timers_state.vm_clock_seqlock); > if (vm_clock_warp_start !=3D=3D -1 && runstate_is_running()) { > .. do stuff .. > } > vm_clock_warp_start =3D -1; > seqlock_write_unlock(&timers_state.vm_clock_seqlock); >=20 > if (we_did_stuff && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { > qemu_clock_notify(QEMU_CLOCK_VIRTUAL); > } Yes, you can make it like that, or even better wrap the read with a seqlock_read_begin/seqlock_read_retry loop. The condition will often be false and it's pointless to lock/unlock the mutex for that. Paolo