On Tue, Mar 15, 2016 at 12:32 PM, Michael S. Tsirkin <[email protected]> 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 <[email protected]> > > 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
