From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39096) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acqKu-0007O4-5z for qemu-devel@nongnu.org; Mon, 07 Mar 2016 03:16:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1acqKr-0001U3-0M for qemu-devel@nongnu.org; Mon, 07 Mar 2016 03:16:16 -0500 Received: from mail-wm0-x233.google.com ([2a00:1450:400c:c09::233]:33189) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acqKq-0001Tb-Od for qemu-devel@nongnu.org; Mon, 07 Mar 2016 03:16:12 -0500 Received: by mail-wm0-x233.google.com with SMTP id l68so97302989wml.0 for ; Mon, 07 Mar 2016 00:16:12 -0800 (PST) Sender: Paolo Bonzini References: <1457210381-23325-1-git-send-email-aesmade@gmail.com> From: Paolo Bonzini Message-ID: <56DD38C9.2010404@redhat.com> Date: Mon, 7 Mar 2016 09:16:09 +0100 MIME-Version: 1.0 In-Reply-To: <1457210381-23325-1-git-send-email-aesmade@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikos Filippakis , qemu-devel@nongnu.org Cc: jasowang@redhat.com On 05/03/2016 21:39, Nikos Filippakis wrote: > Hello everyone! I am interested in getting to know the codebase a little better > so that I can eventually apply for a GSOC position. > This is my first attempt at posting a patch to a mailing list, any feedback > is greatly appreciated. > > Signed-off-by: Nikos Filippakis > --- > net/net.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/net/net.c b/net/net.c > index aebf753..79e9d7c 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size) > static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov, > int iovcnt, unsigned flags) > { > - uint8_t buf[NET_BUFSIZE]; > uint8_t *buffer; > size_t offset; > + ssize_t ret; > > if (iovcnt == 1) { > buffer = iov[0].iov_base; > offset = iov[0].iov_len; > } else { > - buffer = buf; > - offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf)); > + buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); > + offset = iov_to_buf(iov, iovcnt, 0, buffer, > + NET_BUFSIZE * sizeof(uint8_t)); > } Hi Nikos, I suspect an allocation (especially a large one) on this path can have a performance impact. On one hand, nc_sendv_compat is not used in high-performance network cards (e1000 and virtio-net, for example) so this is not a major issue. On the other hand, this is the first such conversion so you could set a good example. Another possibility thus is to make the buffer smaller (e.g. 2K) and check the size with iov_size. If the size is at most 2K, you use the on-stack buffer. If the size is bigger than 2K and smaller than NET_BUFSIZE, you go through the allocation but you only need to allocate the correct number of bytes. Finally, if the size is bigger than NET_BUFSIZE you set errno = EINVAL and return -1. Thanks, Paolo > if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > - return nc->info->receive_raw(nc, buffer, offset); > + ret = nc->info->receive_raw(nc, buffer, offset); > } else { > - return nc->info->receive(nc, buffer, offset); > + ret = nc->info->receive(nc, buffer, offset); > } > + > + if (iovcnt != 1) { > + g_free(buffer); > + } > + > + return ret; > } > > ssize_t qemu_deliver_packet_iov(NetClientState *sender, >