Re: [Qemu-devel] [PATCH 1/1] Allocating Large sized arrays to heap

[Paolo Bonzini <pbonzini@redhat.com>]

The subject isn't very clear, because it doesn't say which subsystem is
being modified.  You should use something like

virtio: allocate temporary VirtQueueElementOld on heap

The rest of the commit message is okay, but the lines are a bit long.
Usually we use 70-75 characters only.

On 11/03/2016 17:12, Jaya Tiwari wrote:
> As per the list of functions in http://wiki.qemu.org/BiteSizedTasks#Large_frames,
> qemu_get_virtqueue_element  and qemu_put_virtqueue_element have large arrays on stack
> Hence, moving them to heap. This reduced their stack size from something 49248 to fit into less than 200
> 
> Signed-off-by: Jaya Tiwari <tiwari.jaya18@gmail.com>
> ---
>  hw/virtio/virtio.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 08275a9..7a7afae 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -636,67 +636,66 @@ typedef struct VirtQueueElementOld {
>  void *qemu_get_virtqueue_element(QEMUFile *f, size_t sz)
>  {
>      VirtQueueElement *elem;
> -    VirtQueueElementOld data;
> +    VirtQueueElementOld *data = g_new(VirtQueueElementOld, 1);

Unconditional allocation is okay, because this is not a fast path.

>      int i;
>  
> -    qemu_get_buffer(f, (uint8_t *)&data, sizeof(VirtQueueElementOld));
> +    qemu_get_buffer(f, (uint8_t *)data, sizeof(VirtQueueElementOld));
>  
> -    elem = virtqueue_alloc_element(sz, data.out_num, data.in_num);
> -    elem->index = data.index;
> +    elem = virtqueue_alloc_element(sz, data->out_num, data->in_num);
> +    elem->index = data->index;
>  
>      for (i = 0; i < elem->in_num; i++) {
> -        elem->in_addr[i] = data.in_addr[i];
> +        elem->in_addr[i] = data->in_addr[i];
>      }
>  
>      for (i = 0; i < elem->out_num; i++) {
> -        elem->out_addr[i] = data.out_addr[i];
> +        elem->out_addr[i] = data->out_addr[i];
>      }
>  
>      for (i = 0; i < elem->in_num; i++) {
>          /* Base is overwritten by virtqueue_map.  */
>          elem->in_sg[i].iov_base = 0;
> -        elem->in_sg[i].iov_len = data.in_sg[i].iov_len;
> +        elem->in_sg[i].iov_len = data->in_sg[i].iov_len;
>      }
>  
>      for (i = 0; i < elem->out_num; i++) {
>          /* Base is overwritten by virtqueue_map.  */
>          elem->out_sg[i].iov_base = 0;
> -        elem->out_sg[i].iov_len = data.out_sg[i].iov_len;
> +        elem->out_sg[i].iov_len = data->out_sg[i].iov_len;
>      }
> -
> +    g_free(data);
>      virtqueue_map(elem);
>      return elem;
>  }
>  
>  void qemu_put_virtqueue_element(QEMUFile *f, VirtQueueElement *elem)
>  {
> -    VirtQueueElementOld data;
> +    VirtQueueElementOld *data = g_new0(VirtQueueElementOld, 1);
>      int i;
> -
> -    memset(&data, 0, sizeof(data));
> -    data.index = elem->index;
> -    data.in_num = elem->in_num;
> -    data.out_num = elem->out_num;
> +    data->index = elem->index;
> +    data->in_num = elem->in_num;
> +    data->out_num = elem->out_num;
>  
>      for (i = 0; i < elem->in_num; i++) {
> -        data.in_addr[i] = elem->in_addr[i];
> +        data->in_addr[i] = elem->in_addr[i];
>      }
>  
>      for (i = 0; i < elem->out_num; i++) {
> -        data.out_addr[i] = elem->out_addr[i];
> +        data->out_addr[i] = elem->out_addr[i];
>      }
>  
>      for (i = 0; i < elem->in_num; i++) {
>          /* Base is overwritten by virtqueue_map when loading.  Do not
>           * save it, as it would leak the QEMU address space layout.  */
> -        data.in_sg[i].iov_len = elem->in_sg[i].iov_len;
> +        data->in_sg[i].iov_len = elem->in_sg[i].iov_len;
>      }
>  
>      for (i = 0; i < elem->out_num; i++) {
>          /* Do not save iov_base as above.  */
> -        data.out_sg[i].iov_len = elem->out_sg[i].iov_len;
> +        data->out_sg[i].iov_len = elem->out_sg[i].iov_len;
>      }
> -    qemu_put_buffer(f, (uint8_t *)&data, sizeof(VirtQueueElementOld));
> +    qemu_put_buffer(f, (uint8_t *)data, sizeof(VirtQueueElementOld));
> +    free(data);

This should have been g_free.  Apart from this and the subject,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

I suggest that you send a fixed version and include mst@redhat.com in
the recipients.

Thanks,

Paolo

>  }
>  
>  /* virtio device */
>