From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49261) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aieqV-0000ZS-KU for qemu-devel@nongnu.org; Wed, 23 Mar 2016 05:12:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aieqS-0006Pk-Eb for qemu-devel@nongnu.org; Wed, 23 Mar 2016 05:12:55 -0400 Received: from e06smtp06.uk.ibm.com ([195.75.94.102]:57581) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aieqR-0006PU-Gb for qemu-devel@nongnu.org; Wed, 23 Mar 2016 05:12:52 -0400 Received: from localhost by e06smtp06.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 23 Mar 2016 09:12:49 -0000 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> <56F25D23.5070604@redhat.com> From: Christian Borntraeger Message-ID: <56F25E0B.2000105@de.ibm.com> Date: Wed, 23 Mar 2016 10:12:43 +0100 MIME-Version: 1.0 In-Reply-To: <56F25D23.5070604@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: Paolo Bonzini , 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 On 03/23/2016 10:08 AM, Paolo Bonzini wrote: > > > 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 = 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, true); >>> 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)); >> >> 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 Tu Bo, can you test with master + Cornelias 6 refactoring patches and nothing on top? > 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) { >> >> 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(VirtIOBlockDataPlane *s) >>> vblk->dataplane_started = false; >>> return; >>> } >>> - s->stopping = true; >>> + >>> trace_virtio_blk_data_plane_stop(s); >>> >>> aio_context_acquire(s->ctx); >>> @@ -274,5 +270,4 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) >>> k->set_guest_notifiers(qbus->parent, 1, false); >>> >>> vblk->dataplane_started = false; >>> - s->stopping = false; >>> } >>> >> >