All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 1/1] Allocating Large sized arrays to heap
@ 2016-03-11 16:12 Jaya Tiwari
  2016-03-14 14:20 ` Paolo Bonzini
  0 siblings, 1 reply; 7+ messages in thread
From: Jaya Tiwari @ 2016-03-11 16:12 UTC (permalink / raw)
  To: qemu-devel

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] Allocating Large sized arrays to heap
  2016-03-11 16:12 [Qemu-devel] [PATCH 1/1] Allocating Large sized arrays to heap Jaya Tiwari
@ 2016-03-14 14:20 ` Paolo Bonzini
  2016-03-14 15:40   ` [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap Jaya Tiwari
  0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2016-03-14 14:20 UTC (permalink / raw)
  To: Jaya Tiwari, qemu-devel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap
  2016-03-14 14:20 ` Paolo Bonzini
@ 2016-03-14 15:40   ` Jaya Tiwari
  2016-03-15  7:02     ` Michael S. Tsirkin
  0 siblings, 1 reply; 7+ messages in thread
From: Jaya Tiwari @ 2016-03-14 15:40 UTC (permalink / raw)
  To: qemu-devel, mst

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap
  2016-03-14 15:40   ` [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap Jaya Tiwari
@ 2016-03-15  7:02     ` Michael S. Tsirkin
  2016-03-15  7:36       ` Jaya Tiwari
  0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2016-03-15  7:02 UTC (permalink / raw)
  To: Jaya Tiwari; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap
  2016-03-15  7:02     ` Michael S. Tsirkin
@ 2016-03-15  7:36       ` Jaya Tiwari
  2016-03-15  9:16         ` Paolo Bonzini
  0 siblings, 1 reply; 7+ messages in thread
From: Jaya Tiwari @ 2016-03-15  7:36 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap
  2016-03-15  7:36       ` Jaya Tiwari
@ 2016-03-15  9:16         ` Paolo Bonzini
  2016-03-15  9:40           ` Michael S. Tsirkin
  0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2016-03-15  9:16 UTC (permalink / raw)
  To: Jaya Tiwari, Michael S. Tsirkin; +Cc: qemu-devel



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap
  2016-03-15  9:16         ` Paolo Bonzini
@ 2016-03-15  9:40           ` Michael S. Tsirkin
  0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2016-03-15  9:40 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Jaya Tiwari

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-03-15  9:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-11 16:12 [Qemu-devel] [PATCH 1/1] Allocating Large sized arrays to heap Jaya Tiwari
2016-03-14 14:20 ` Paolo Bonzini
2016-03-14 15:40   ` [Qemu-devel] [PATCH V2 1/1] virtio:Allocate temporary VirtQueueElementOld on heap Jaya Tiwari
2016-03-15  7:02     ` Michael S. Tsirkin
2016-03-15  7:36       ` Jaya Tiwari
2016-03-15  9:16         ` Paolo Bonzini
2016-03-15  9:40           ` Michael S. Tsirkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.