qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 02/10] virtio: move allocation to virtqueue_pop/


From: Cornelia Huck
Subject: Re: [Qemu-devel] [PATCH 02/10] virtio: move allocation to virtqueue_pop/vring_pop
Date: Tue, 19 Jan 2016 13:22:00 +0100

On Fri, 15 Jan 2016 13:41:50 +0100
Paolo Bonzini <address@hidden> wrote:

> The return code of virtqueue_pop/vring_pop is unused except to check for
> errors or 0.  We can thus easily move allocation inside the functions
> and just return a pointer to the VirtQueueElement.

Like this change.

> 
> The advantage is that we will be able to allocate only the space that
> is needed for the actual size of the s/g list instead of the full
> VIRTQUEUE_MAX_SIZE items.  Currently VirtQueueElement takes about 48K
> of memory, and this kind of allocation puts a lot of stress on malloc.
> By cutting the size by two or three orders of magnitude, malloc can
> use much more efficient algorithms.
> 
> The patch is pretty large, but changes to each device are testable
> more or less independently.  Splitting it would mostly add churn.

Would it help to add a no-frills virtqueue_pop() version that simply
allocates the base VirtQueueElement and use a _size() variant for those
callers that want an extended structure?

> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/9pfs/9p.c                        |  2 +-
>  hw/9pfs/virtio-9p-device.c          | 16 ++++----
>  hw/9pfs/virtio-9p.h                 |  2 +-
>  hw/block/dataplane/virtio-blk.c     | 11 +++--
>  hw/block/virtio-blk.c               | 15 +++----
>  hw/char/virtio-serial-bus.c         | 80 
> +++++++++++++++++++++++--------------
>  hw/display/virtio-gpu.c             | 25 +++++++-----
>  hw/input/virtio-input.c             | 24 +++++++----
>  hw/net/virtio-net.c                 | 69 ++++++++++++++++++++------------
>  hw/scsi/virtio-scsi-dataplane.c     | 15 +++----
>  hw/scsi/virtio-scsi.c               | 18 ++++-----
>  hw/virtio/dataplane/vring.c         | 17 ++++----
>  hw/virtio/virtio-balloon.c          | 22 ++++++----
>  hw/virtio/virtio-rng.c              | 10 +++--
>  hw/virtio/virtio.c                  | 11 +++--
>  include/hw/virtio/dataplane/vring.h |  2 +-
>  include/hw/virtio/virtio-balloon.h  |  2 +-
>  include/hw/virtio/virtio-blk.h      |  3 +-
>  include/hw/virtio/virtio-net.h      |  2 +-
>  include/hw/virtio/virtio-scsi.h     |  2 +-
>  include/hw/virtio/virtio-serial.h   |  2 +-
>  include/hw/virtio/virtio.h          |  2 +-
>  22 files changed, 209 insertions(+), 143 deletions(-)

(...)

> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 23f667e..1b900fc 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -388,23 +388,25 @@ static void vring_unmap_element(VirtQueueElement *elem)
>   *
>   * Stolen from linux/drivers/vhost/vhost.c.
>   */
> -int vring_pop(VirtIODevice *vdev, Vring *vring,
> -              VirtQueueElement *elem)
> +void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz)
>  {
>      struct vring_desc desc;
>      unsigned int i, head, found = 0, num = vring->vr.num;
>      uint16_t avail_idx, last_avail_idx;
> +    VirtQueueElement *elem = NULL;
>      int ret;

assert(sz >= sizeof(VirtQueueElement));

?

> 
> -    /* Initialize elem so it can be safely unmapped */
> -    elem->in_num = elem->out_num = 0;
> -
>      /* If there was a fatal error then refuse operation */
>      if (vring->broken) {
>          ret = -EFAULT;
>          goto out;
>      }
> 
> +    elem = g_malloc(sz);
> +
> +    /* Initialize elem so it can be safely unmapped */
> +    elem->in_num = elem->out_num = 0;
> +
>      /* Check it isn't doing very strange things with descriptor numbers. */
>      last_avail_idx = vring->last_avail_idx;
>      avail_idx = vring_get_avail_idx(vdev, vring);

(...)

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index bd6b4df..6c4c8bc 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -501,16 +501,19 @@ void virtqueue_map(VirtQueueElement *elem)
>                          0);
>  }
> 
> -int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> +void *virtqueue_pop(VirtQueue *vq, size_t sz)
>  {
>      unsigned int i, head, max;
>      hwaddr desc_pa = vq->vring.desc;
>      VirtIODevice *vdev = vq->vdev;
> +    VirtQueueElement *elem;

assert(sz >= sizeof(VirtQueueElement));

here as well?

> 
> -    if (!virtqueue_num_heads(vq, vq->last_avail_idx))
> -        return 0;
> +    if (!virtqueue_num_heads(vq, vq->last_avail_idx)) {
> +        return NULL;
> +    }
> 
>      /* When we start there are none of either input nor output. */
> +    elem = g_malloc(sz);
>      elem->out_num = elem->in_num = 0;
> 
>      max = vq->vring.num;




reply via email to

[Prev in Thread] Current Thread [Next in Thread]