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

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

[Jaya Tiwari]

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);
     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 */
-- 
1.9.1

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

[Paolo Bonzini]

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 */
> 

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

[Jaya Tiwari]

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 | 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 = 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));
+    g_free(data);
 }
 
 /* virtio device */
-- 
1.8.1.2

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

[Michael S. Tsirkin]

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 <tiwari.jaya18@gmail.com>

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.


> ---
>  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 = 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));
> +    g_free(data);
>  }
>  
>  /* virtio device */
> -- 
> 1.8.1.2

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

[Jaya Tiwari]

[-- Attachment #1: Type: text/plain, Size: 4605 bytes --]

On Tue, Mar 15, 2016 at 12:32 PM, Michael S. Tsirkin <mst@redhat.com> 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 <tiwari.jaya18@gmail.com>
>
> 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 = 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));
> > +    g_free(data);
> >  }
> >
> >  /* virtio device */
> > --
> > 1.8.1.2
>



-- 
Regards,
Jaya

[-- Attachment #2: Type: text/html, Size: 6635 bytes --]

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

[Paolo Bonzini]

On 15/03/2016 08:36, Jaya Tiwari wrote:
> 
>     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.

This is not a hot path.  It only happens when saving/loading data after
migration.  Surely the few microseconds wasted in allocating data on the
heap are beaten by zeroing the memory, by all the for loops in the
functions, and of course by the 3-500 *milli*seconds of downtime caused
by migration.

> 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? 

Jaya, this patch is okay.  What Michael said is true in other cases, but
not this one.

Paolo

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

[Michael S. Tsirkin]

On Tue, Mar 15, 2016 at 10:16:00AM +0100, Paolo Bonzini wrote:
> 
> 
> On 15/03/2016 08:36, Jaya Tiwari wrote:
> > 
> >     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.
> 
> This is not a hot path.  It only happens when saving/loading data after
> migration.  Surely the few microseconds wasted in allocating data on the
> heap are beaten by zeroing the memory, by all the for loops in the
> functions, and of course by the 3-500 *milli*seconds of downtime caused
> by migration.
> 
> > 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? 
> 
> Jaya, this patch is okay.  What Michael said is true in other cases, but
> not this one.
> 
> Paolo

Hmm, I got confused. You are right.
I'll redo the review, sorry about the noise.

-- 
MST