[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] Allocating Large sized arrays to heap
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] Allocating Large sized arrays to heap |
Date: |
Mon, 14 Mar 2016 15:20:15 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
I suggest that you send a fixed version and include address@hidden in
the recipients.
Thanks,
Paolo
> }
>
> /* virtio device */
>