From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39080) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bomsg-0002hU-V5 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 03:32:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bomsc-0005Dd-OQ for qemu-devel@nongnu.org; Tue, 27 Sep 2016 03:32:45 -0400 Received: from mail-vk0-f52.google.com ([209.85.213.52]:36040) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bomsc-0005D7-KE for qemu-devel@nongnu.org; Tue, 27 Sep 2016 03:32:42 -0400 Received: by mail-vk0-f52.google.com with SMTP id d8so5210503vka.3 for ; Tue, 27 Sep 2016 00:32:41 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1474291685-24226-2-git-send-email-stefanha@redhat.com> References: <1474291685-24226-1-git-send-email-stefanha@redhat.com> <1474291685-24226-2-git-send-email-stefanha@redhat.com> From: Ladi Prosek Date: Tue, 27 Sep 2016 09:32:40 +0200 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 1/3] virtio: add virtio_detach_element() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel , "Michael S. Tsirkin" On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi wrote: > During device reset or similar situations a VirtQueueElement needs to be > freed without pushing it onto the used ring or rewinding the virtqueue. > Extract a new function to do this. > > Later patches add virtio_detach_element() calls to existing device so > that scatter-gather lists are unmapped and vq->inuse goes back to zero > during device reset. Currently some devices don't bother and simply > call g_free(elem) which is not a clean way to throw away a > VirtQueueElement. > > Signed-off-by: Stefan Hajnoczi > --- > hw/virtio/virtio.c | 27 +++++++++++++++++++++++++-- > include/hw/virtio/virtio.h | 2 ++ > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index fcf3358..adcef45 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -264,12 +264,35 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem, > 0, elem->out_sg[i].iov_len); > } > > +/* virtqueue_detach_element: > + * @vq: The #VirtQueue > + * @elem: The #VirtQueueElement > + * @len: number of bytes written > + * > + * Detach the element from the virtqueue. This function is suitable for device > + * reset or other situations where a #VirtQueueElement is simply freed and will > + * not be pushed or discarded. > + */ > +void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem, > + unsigned int len) > +{ > + vq->inuse--; > + virtqueue_unmap_sg(vq, elem, len); > +} > + > +/* virtqueue_discard: > + * @vq: The #VirtQueue > + * @elem: The #VirtQueueElement > + * @len: number of bytes written > + * > + * Pretend the most recent element wasn't popped from the virtqueue. The next > + * call to virtqueue_pop() will refetch the element. > + */ > void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len) > { > vq->last_avail_idx--; > - vq->inuse--; > - virtqueue_unmap_sg(vq, elem, len); > + virtqueue_detach_element(vq, elem, len); Random comment, not directly related to this change. Would it be worth adding an assert to this function that elem->index and vq->last_avail_idx match? In other words, enforce the "most recent" qualifier mentioned in the comment. As more virtqueue_* functions are added and the complexity goes up, it is easy to get confused. Also, I think that naming this function virtqueue_unpop instead of virtqueue_discard would help. > } > > /* virtqueue_rewind: > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index f05559d..ad1e2d6 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -152,6 +152,8 @@ void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num); > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > void virtqueue_flush(VirtQueue *vq, unsigned int count); > +void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem, > + unsigned int len); > void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > bool virtqueue_rewind(VirtQueue *vq, unsigned int num); > -- > 2.7.4 > Reviewed-by: Ladi Prosek Thanks! Ladi