From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VoVAO-0000HI-MR for qemu-devel@nongnu.org; Thu, 05 Dec 2013 04:24:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VoVAI-00087T-N1 for qemu-devel@nongnu.org; Thu, 05 Dec 2013 04:24:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55091) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VoVAI-00087J-Dv for qemu-devel@nongnu.org; Thu, 05 Dec 2013 04:24:10 -0500 Date: Thu, 5 Dec 2013 10:24:06 +0100 From: Stefan Hajnoczi Message-ID: <20131205092406.GA9629@stefanha-thinkpad.redhat.com> References: <1381417639-22547-1-git-send-email-pbonzini@redhat.com> <1381417639-22547-4-git-send-email-pbonzini@redhat.com> <20131204140649.GA27759@stefanha-thinkpad.redhat.com> <529F690E.9020802@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <529F690E.9020802@redhat.com> Subject: Re: [Qemu-devel] [PATCH 3/4] dataplane: change vring API to use VirtQueueElement List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Stefan Hajnoczi , qemu-devel@nongnu.org On Wed, Dec 04, 2013 at 06:40:30PM +0100, Paolo Bonzini wrote: > Il 04/12/2013 15:06, Stefan Hajnoczi ha scritto: > > On Thu, Oct 10, 2013 at 05:07:18PM +0200, Paolo Bonzini wrote: > >> @@ -298,30 +278,31 @@ static void handle_notify(EventNotifier *e) > >> vring_disable_notification(s->vdev, &s->vring); > >> > >> for (;;) { > >> - head = vring_pop(s->vdev, &s->vring, iov, end, &out_num, &in_num); > >> - if (head < 0) { > >> + ret = vring_pop(s->vdev, &s->vring, &elem); > >> + if (ret < 0) { > >> + assert(elem == NULL); > >> break; /* no more requests */ > >> } > >> > >> - trace_virtio_blk_data_plane_process_request(s, out_num, in_num, > >> - head); > >> + trace_virtio_blk_data_plane_process_request(s, elem->out_num, > >> + elem->in_num, elem->index); > >> > >> - if (process_request(&s->ioqueue, iov, out_num, in_num, head) < 0) { > >> + if (process_request(&s->ioqueue, elem) < 0) { > >> vring_set_broken(&s->vring); > >> + vring_push(&s->vring, elem, 0); > > > > If we give up on the vring I don't think we should push the element > > back. It may cause the guest to panic. > > > > I guess what we really need here is to unmap scatter-gather buffers and > > delete elem. > > That's what already happens actually. vring_push has > > > + g_slice_free(VirtQueueElement, elem); > + > /* Don't touch vring if a fatal error occurred */ > if (vring->broken) { > return; > > in this patch and > > + for (i = 0; i < elem->out_num; i++) { > + vring_unmap(elem->out_sg[i].iov_base, false); > + } > + > + for (i = 0; i < elem->in_num; i++) { > + vring_unmap(elem->in_sg[i].iov_base, true); > + } > > g_slice_free(VirtQueueElement, elem); > > in the next one. > > Though I admit vring_push isn't such a great name and API. I can add > instead a vring_free_element function. Do you think vring_push should > call it, or should the caller do that? I think vring_push() should free the VirtQueueElement. We just need to expose vring_free_element() so that handle_notify() can call it without pushing bogus buffers back to the guest. Stefan