From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53943) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afTM7-0002cr-6u for qemu-devel@nongnu.org; Mon, 14 Mar 2016 10:20:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afTM3-00065F-60 for qemu-devel@nongnu.org; Mon, 14 Mar 2016 10:20:23 -0400 Received: from mail-wm0-x243.google.com ([2a00:1450:400c:c09::243]:32796) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afTM2-00065A-Rd for qemu-devel@nongnu.org; Mon, 14 Mar 2016 10:20:19 -0400 Received: by mail-wm0-x243.google.com with SMTP id n186so15410325wmn.0 for ; Mon, 14 Mar 2016 07:20:18 -0700 (PDT) Sender: Paolo Bonzini References: <1457712721-2933-1-git-send-email-tiwari.jaya18@gmail.com> From: Paolo Bonzini Message-ID: <56E6C89F.5030503@redhat.com> Date: Mon, 14 Mar 2016 15:20:15 +0100 MIME-Version: 1.0 In-Reply-To: <1457712721-2933-1-git-send-email-tiwari.jaya18@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] Allocating Large sized arrays to heap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jaya Tiwari , qemu-devel@nongnu.org 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 > --- > 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 I suggest that you send a fixed version and include mst@redhat.com in the recipients. Thanks, Paolo > } > > /* virtio device */ >