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.jay...@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 <pbonz...@redhat.com> I suggest that you send a fixed version and include m...@redhat.com in the recipients. Thanks, Paolo > } > > /* virtio device */ >
