[Qemu-devel] [PATCH 0/2] virtio: cosmetic tweaks

[Qemu-devel] [PATCH 0/2] virtio: cosmetic tweaks

[Ladi Prosek]

Just a trivial couple of patches addressing minor style issues.

Enforcing the correct order of operations (pop, unpop, push, rewind) with an assert,
which was another potential change discussed previously, would add a non-trivial
amount of code and is not worth the effort in my opinion. A simple linked-list of
VirtQueueElement would not be enough because these are not in the ownership of the
common virtio code. The effect of an out-of-order, yet valid, virtqueue_push would
need to be encoded in the linked list even though the element has been deallocated.
It's almost like maintaining a shadow available ring - doable but not justified at
the moment.

Ladi Prosek (2):
  virtio: rename virtqueue_discard to virtqueue_unpop
  virtio: make virtqueue_alloc_element static

[Qemu-devel] [PATCH 1/2] virtio: rename virtqueue_discard to virtqueue_unpop

[Ladi Prosek]

The function undoes the effect of virtqueue_pop and doesn't do anything
destructive or irreversible so virtqueue_unpop is a more fitting name.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/net/virtio-net.c        | 2 +-
 hw/virtio/virtio-balloon.c | 2 +-
 hw/virtio/virtio.c         | 6 +++---
 include/hw/virtio/virtio.h | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6b8ae2c..b3780f0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1171,7 +1171,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
          * must have consumed the complete packet.
          * Otherwise, drop it. */
         if (!n->mergeable_rx_bufs && offset < size) {
-            virtqueue_discard(q->rx_vq, elem, total);
+            virtqueue_unpop(q->rx_vq, elem, total);
             g_free(elem);
             return size;
         }
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 49a2f4a..a159fbf 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -463,7 +463,7 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
     if (s->stats_vq_elem != NULL) {
-        virtqueue_discard(s->svq, s->stats_vq_elem, 0);
+        virtqueue_unpop(s->svq, s->stats_vq_elem, 0);
         g_free(s->stats_vq_elem);
         s->stats_vq_elem = NULL;
     }
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 18ce333..12cef6c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -264,8 +264,8 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
                                   0, elem->out_sg[i].iov_len);
 }
 
-void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
-                       unsigned int len)
+void virtqueue_unpop(VirtQueue *vq, const VirtQueueElement *elem,
+                     unsigned int len)
 {
     vq->last_avail_idx--;
     vq->inuse--;
@@ -279,7 +279,7 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
  * Pretend that elements weren't popped from the virtqueue.  The next
  * virtqueue_pop() will refetch the oldest element.
  *
- * Use virtqueue_discard() instead if you have a VirtQueueElement.
+ * Use virtqueue_unpop() instead if you have a VirtQueueElement.
  *
  * Returns: true on success, false if @num is greater than the number of in use
  * elements.
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 888c8de..5d12434 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -155,8 +155,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_discard(VirtQueue *vq, const VirtQueueElement *elem,
-                       unsigned int len);
+void virtqueue_unpop(VirtQueue *vq, const VirtQueueElement *elem,
+                     unsigned int len);
 bool virtqueue_rewind(VirtQueue *vq, unsigned int num);
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx);
-- 
2.5.5

[Qemu-devel] [PATCH 2/2] virtio: make virtqueue_alloc_element static

[Ladi Prosek]

The function does not fully initialize the returned VirtQueueElement and should
be used only internally from the virtio module.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/virtio/virtio.c         | 2 +-
 include/hw/virtio/virtio.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 12cef6c..c0c3046 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -610,7 +610,7 @@ void virtqueue_map(VirtQueueElement *elem)
                         VIRTQUEUE_MAX_SIZE, 0);
 }
 
-void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num)
+static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num)
 {
     VirtQueueElement *elem;
     size_t in_addr_ofs = QEMU_ALIGN_UP(sz, __alignof__(elem->in_addr[0]));
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 5d12434..97df323 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -151,7 +151,6 @@ VirtQueue *virtio_add_queue_aio(VirtIODevice *vdev, int queue_size,
 
 void virtio_del_queue(VirtIODevice *vdev, int n);
 
-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);
-- 
2.5.5

Re: [Qemu-devel] [PATCH 0/2] virtio: cosmetic tweaks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On Tue, Oct 11, 2016 at 11:22:02AM +0200, Ladi Prosek wrote:
> Just a trivial couple of patches addressing minor style issues.
> 
> Enforcing the correct order of operations (pop, unpop, push, rewind) with an assert,
> which was another potential change discussed previously, would add a non-trivial
> amount of code and is not worth the effort in my opinion. A simple linked-list of
> VirtQueueElement would not be enough because these are not in the ownership of the
> common virtio code. The effect of an out-of-order, yet valid, virtqueue_push would
> need to be encoded in the linked list even though the element has been deallocated.
> It's almost like maintaining a shadow available ring - doable but not justified at
> the moment.
> 
> Ladi Prosek (2):
>   virtio: rename virtqueue_discard to virtqueue_unpop
>   virtio: make virtqueue_alloc_element static
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]