diff mbox

[1/1] Allocating Large sized arrays to heap

Message ID 1457712721-2933-1-git-send-email-tiwari.jaya18@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jaya Tiwari March 11, 2016, 4:12 p.m. UTC 
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(-)

Comments

Paolo Bonzini March 14, 2016, 2:20 p.m. UTC | #1 
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 */
>
diff mbox

Patch

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);
     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);
 }
 
 /* virtio device */