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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Thu, Jun 05, 2025 at 01:28:49PM +0200, Philippe Mathieu-Daudé wrote:
> 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;

This is an alternative. Using g_alloca() is another alternative.

I chose __attribute__((uninitialized)) because it clearly documents the
reason why these variables need special treatment. In your patch the
"Only used by virtqueue_pop()" comment isn't enough to explain why these
variables should be located here. Someone might accidentally move them
back into virtqueue_pop() functions in the future if they are unaware of
the reason.

I'm happy to change approaches based on the pros/cons. Why do you prefer
moving the local variables into VirtQueue?

>  };
> 
>  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(-)
> 

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