From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48405) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiemm-0006XH-It for qemu-devel@nongnu.org; Wed, 23 Mar 2016 05:09:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiemi-0005Mi-LN for qemu-devel@nongnu.org; Wed, 23 Mar 2016 05:09:04 -0400 References: <1458123018-18651-1-git-send-email-famz@redhat.com> <1458123018-18651-5-git-send-email-famz@redhat.com> <20160317150057.GP14062@stefanha-x1.localdomain> <56EAC82A.50907@redhat.com> <20160322125254.GB9749@ad.usersys.redhat.com> <56F18955.4060005@redhat.com> <20160323091009.64eb4cd8.cornelia.huck@de.ibm.com> From: Paolo Bonzini Message-ID: <56F25D23.5070604@redhat.com> Date: Wed, 23 Mar 2016 10:08:51 +0100 MIME-Version: 1.0 In-Reply-To: <20160323091009.64eb4cd8.cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: Kevin Wolf , Fam Zheng , qemu-block@nongnu.org, "Michael S. Tsirkin" , Stefan Hajnoczi , qemu-devel@nongnu.org, tubo@linux.vnet.ibm.com, Stefan Hajnoczi , borntraeger@de.ibm.com On 23/03/2016 09:10, Cornelia Huck wrote: >> - /* Kick right away to begin processing requests already in vring = */ >> - event_notifier_set(virtio_queue_get_host_notifier(s->vq)); >> + vblk->dataplane_started =3D true; >> >> - /* Get this show started by hooking up our callbacks */ >> + /* Get this show started by hooking up our callbacks. */ >> aio_context_acquire(s->ctx); >> virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, t= rue); >> aio_context_release(s->ctx); >> + atomic_dec(&s->starting); >> + >> + /* Kick right away to begin processing requests already in vring = */ >> + event_notifier_set(virtio_queue_get_host_notifier(s->vq)); >=20 > I'm wondering whether moving this event_notifier_set() masks something? > IOW, may we run into trouble if the event notifier is set from some > other path before the callbacks are set up properly? The reentrancy check should catch that... But: 1) the patch really makes no difference, your fix is enough for me 2) vblk->dataplane_started becomes true before the callbacks are set; that should be enough. 3) this matches what I tested, but it would of course be better if the assertions on s->starting suffice >> - if (!vblk->dataplane_started || s->stopping) { >> + if (!vblk->dataplane_started) { >=20 > No fear of reentrancy here? No, because this is only invoked from reset, hence only from the CPU thread and only under the BQL. On start, reentrancy happens between iothread (running outside BQL) and a CPU thread (running within BQL). Paolo >> return; >> } >> >> @@ -255,7 +251,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPla= ne *s) >> vblk->dataplane_started =3D false; >> return; >> } >> - s->stopping =3D true; >> + >> trace_virtio_blk_data_plane_stop(s); >> >> aio_context_acquire(s->ctx); >> @@ -274,5 +270,4 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPla= ne *s) >> k->set_guest_notifiers(qbus->parent, 1, false); >> >> vblk->dataplane_started =3D false; >> - s->stopping =3D false; >> } >> >=20