[PATCH v2 0/2] virtio: clean up of virtqueue_split_read_next_desc()

[PATCH v2 0/2] virtio: clean up of virtqueue_split_read_next_desc()

[Ilya Maximets]

Version 2:
  - Converted into a patch set adding a new patch that removes the
    'next' argument.  [Stefan]
  - Completely removing the barrier instead of changing into compiler
    barrier.  [Stefan]


Ilya Maximets (2):
  virtio: remove unnecessary thread fence while reading next descriptor
  virtio: remove unused next argument from
    virtqueue_split_read_next_desc()

 hw/virtio/virtio.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

-- 
2.41.0

[PATCH v2 1/2] virtio: remove unnecessary thread fence while reading next descriptor

[Ilya Maximets]

It was supposed to be a compiler barrier and it was a compiler barrier
initially called 'wmb' when virtio core support was introduced.
Later all the instances of 'wmb' were switched to smp_wmb to fix memory
ordering issues on non-x86 platforms.  However, this one doesn't need
to be an actual barrier, as its only purpose was to ensure that the
value is not read twice.

And since commit aa570d6fb6bd ("virtio: combine the read of a descriptor")
there is no need for a barrier at all, since we're no longer reading
guest memory here, but accessing a local structure.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 hw/virtio/virtio.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4577f3f5b3..37584aaa74 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1050,8 +1050,6 @@ static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
 
     /* Check they're not leading us off end of descriptors. */
     *next = desc->next;
-    /* Make sure compiler knows to grab that: we don't want it changing! */
-    smp_wmb();
 
     if (*next >= max) {
         virtio_error(vdev, "Desc next is %u", *next);
-- 
2.41.0

[PATCH v2 2/2] virtio: remove unused next argument from virtqueue_split_read_next_desc()

[Ilya Maximets]

The 'next' was converted from a local variable to an output parameter
in commit:
  412e0e81b174 ("virtio: handle virtqueue_read_next_desc() errors")

But all the actual uses of the 'i/next' as an output were removed a few
months prior in commit:
  aa570d6fb6bd ("virtio: combine the read of a descriptor")

Remove the unused argument to simplify the code.

Also, adding a comment to the function to describe what it is actually
doing, as it is not obvious that the 'desc' is both an input and an
output argument.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 hw/virtio/virtio.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 37584aaa74..bc0388d45b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1039,9 +1039,10 @@ enum {
     VIRTQUEUE_READ_DESC_MORE = 1,   /* more buffers in chain */
 };
 
+/* Reads the 'desc->next' descriptor into '*desc'. */
 static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
                                           MemoryRegionCache *desc_cache,
-                                          unsigned int max, unsigned int *next)
+                                          unsigned int max)
 {
     /* If this descriptor says it doesn't chain, we're done. */
     if (!(desc->flags & VRING_DESC_F_NEXT)) {
@@ -1049,14 +1050,12 @@ static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
     }
 
     /* Check they're not leading us off end of descriptors. */
-    *next = desc->next;
-
-    if (*next >= max) {
-        virtio_error(vdev, "Desc next is %u", *next);
+    if (desc->next >= max) {
+        virtio_error(vdev, "Desc next is %u", desc->next);
         return VIRTQUEUE_READ_DESC_ERROR;
     }
 
-    vring_split_desc_read(vdev, desc, desc_cache, *next);
+    vring_split_desc_read(vdev, desc, desc_cache, desc->next);
     return VIRTQUEUE_READ_DESC_MORE;
 }
 
@@ -1134,7 +1133,7 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
                 goto done;
             }
 
-            rc = virtqueue_split_read_next_desc(vdev, &desc, desc_cache, max, &i);
+            rc = virtqueue_split_read_next_desc(vdev, &desc, desc_cache, max);
         } while (rc == VIRTQUEUE_READ_DESC_MORE);
 
         if (rc == VIRTQUEUE_READ_DESC_ERROR) {
@@ -1585,7 +1584,7 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
             goto err_undo_map;
         }
 
-        rc = virtqueue_split_read_next_desc(vdev, &desc, desc_cache, max, &i);
+        rc = virtqueue_split_read_next_desc(vdev, &desc, desc_cache, max);
     } while (rc == VIRTQUEUE_READ_DESC_MORE);
 
     if (rc == VIRTQUEUE_READ_DESC_ERROR) {
@@ -4039,8 +4038,7 @@ VirtioQueueElement *qmp_x_query_virtio_queue_element(const char *path,
             list = node;
 
             ndescs++;
-            rc = virtqueue_split_read_next_desc(vdev, &desc, desc_cache,
-                                                max, &i);
+            rc = virtqueue_split_read_next_desc(vdev, &desc, desc_cache, max);
         } while (rc == VIRTQUEUE_READ_DESC_MORE);
         element->descs = list;
 done:
-- 
2.41.0

Re: [PATCH v2 0/2] virtio: clean up of virtqueue_split_read_next_desc()

[Stefan Hajnoczi]

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

On Wed, 27 Sept 2023 at 10:00, Ilya Maximets <i.maximets@ovn.org> wrote:
>
>
> Version 2:
>   - Converted into a patch set adding a new patch that removes the
>     'next' argument.  [Stefan]
>   - Completely removing the barrier instead of changing into compiler
>     barrier.  [Stefan]
>
>
> Ilya Maximets (2):
>   virtio: remove unnecessary thread fence while reading next descriptor
>   virtio: remove unused next argument from
>     virtqueue_split_read_next_desc()
>
>  hw/virtio/virtio.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> --
> 2.41.0
>
>