On Fri, 15 Jan 2016 13:41:50 +0100
Paolo Bonzini <pbonz...@redhat.com> 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 <pbonz...@redhat.com>
> ---
> 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;
