Re: [PATCH] virtio: avoid cost of -ftrivial-auto-var-init in hot path

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 5/6/25 10:34, Daniel P. Berrangé wrote:
> On Wed, Jun 04, 2025 at 03:18:43PM -0400, Stefan Hajnoczi wrote:
>> Since commit 7ff9ff039380 ("meson: mitigate against use of uninitialize
>> stack for exploits") the -ftrivial-auto-var-init=zero compiler option is
>> used to zero local variables. While this reduces security risks
>> associated with uninitialized stack data, it introduced a measurable
>> bottleneck in the virtqueue_split_pop() and virtqueue_packed_pop()
>> functions.
>>
>> These virtqueue functions are in the hot path. They are called for each
>> element (request) that is popped from a VIRTIO device's virtqueue. Using
>> __attribute__((uninitialized)) on large stack variables in these
>> functions improves fio randread bs=4k iodepth=64 performance from 304k
>> to 332k IOPS (+9%).
> 
> IIUC, the 'hwaddr addr' variable is 8k in size, and the 'struct iovec iov'
> array is 16k in size, so we have 24k on the stack that we're clearing and
> then later writing the real value. Makes sense that this would have a
> perf impact in a hotpath.
> 
>> This issue was found using perf-top(1). virtqueue_split_pop() was one of
>> the top CPU consumers and the "annotate" feature showed that the memory
>> zeroing instructions at the beginning of the functions were hot.
> 
> When you say you found it with 'perf-top' was that just discovered by
> accident, or was this usage of perf-top in response to users reporting
> a performance degradation vs earlier QEMU ?

Would it make sense to move these to VirtQueue (since the structure
definition is local anyway)?

-- >8 --
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 85110bce374..b96c6ec603c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -153,6 +153,12 @@ struct VirtQueue
      EventNotifier host_notifier;
      bool host_notifier_enabled;
      QLIST_ENTRY(VirtQueue) node;
+
+    /* Only used by virtqueue_pop() */
+    struct {
+        hwaddr addr[VIRTQUEUE_MAX_SIZE];
+        struct iovec iov[VIRTQUEUE_MAX_SIZE];
+    } pop;
  };

  const char *virtio_device_names[] = {
@@ -1680,8 +1686,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, 
size_t sz)
      VirtIODevice *vdev = vq->vdev;
      VirtQueueElement *elem = NULL;
      unsigned out_num, in_num, elem_entries;
-    hwaddr addr[VIRTQUEUE_MAX_SIZE];
-    struct iovec iov[VIRTQUEUE_MAX_SIZE];
+    hwaddr *addr = vq->pop.addr;
+    struct iovec *iov = vq->pop.iov;
      VRingDesc desc;
      int rc;

@@ -1826,8 +1832,8 @@ static void *virtqueue_packed_pop(VirtQueue *vq, 
size_t sz)
      VirtIODevice *vdev = vq->vdev;
      VirtQueueElement *elem = NULL;
      unsigned out_num, in_num, elem_entries;
-    hwaddr addr[VIRTQUEUE_MAX_SIZE];
-    struct iovec iov[VIRTQUEUE_MAX_SIZE];
+    hwaddr *addr = vq->pop.addr;
+    struct iovec *iov = vq->pop.iov;
      VRingPackedDesc desc;
      uint16_t id;
      int rc;
---

> 
>>
>> Fixes: 7ff9ff039380 ("meson: mitigate against use of uninitialize stack for exploits")
>> Cc: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   include/qemu/compiler.h | 12 ++++++++++++
>>   hw/virtio/virtio.c      |  8 ++++----
>>   2 files changed, 16 insertions(+), 4 deletions(-)