Re: [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap

[Jaya Tiwari]

On Tue, Mar 15, 2016 at 12:32 PM, Michael S. Tsirkin <address@hidden> wrote:

On Mon, Mar 14, 2016 at 09:10:15PM +0530, 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 <address@hidden>

This is not what that page suggests. It says:
        Make the stack array
        smaller and allocate on the heap in the rare case that the data does not
        fit in the small array:

This patch just uses heap unconditionally which is sure to hurt performance.

Yes Okay.

Thank you for pointing it out.

So I should be including a condition to check with a small stack size, and if the array crosses it, only then

it should be placed in heap, otherwise it should not be using heap.

Am I correct in my understanding here? 

> ---
>  hw/virtio/virtio.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 08275a9..027e7e2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -636,67 +636,68 @@ typedef struct VirtQueueElementOld {
>  void *qemu_get_virtqueue_element(QEMUFile *f, size_t sz)
>  {
>      VirtQueueElement *elem;
> -    VirtQueueElementOld data;
> +    VirtQueueElementOld *data = "" 1);
>      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 = "" 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));
> +    g_free(data);
>  }
>
>  /* virtio device */
> --
> 1.8.1.2

--

Regards,

Jaya