Re: [Qemu-devel] [Patch 1/1] net/net: Allocating Large sized arrays to heap
On Mon, Mar 14, 2016 at 7:41 PM, Eric Blakewrote: > On 03/12/2016 01:39 AM, Pooja Dhannawat wrote: > > Signed-off-by: Pooja Dhannawat > > --- > > net/net.c | 8 ++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/net/net.c b/net/net.c > > index b0c832e..5399758 100644 > > --- a/net/net.c > > +++ b/net/net.c > > @@ -709,16 +709,18 @@ 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 *buf; > > uint8_t *buffer; > > size_t offset; > > > > +buf = g_new(uint8_t, 1); > > If you're only going to malloc() one byte, it's more efficient to just > stack-allocate it: > > uint8_t buf[1]; > > Did you mean to do: > > buf = g_new(uint8_t, NET_BUFSIZE) > > instead? > > Yes, my bad, I meant NET_BUFSIZE. > > + > > 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)); > > +offset = iov_to_buf(iov, iovcnt, 0, (uint8_t *)buf, > sizeof(uint8_t)); > > This is wrong. The old code used NET_BUFSIZE bytes for iov_to_buf(), > the new code uses only one byte. By the way, sizeof(char) and > sizeof(uint8_t) are pointless in code, as they are strictly equivalent to > 1. > > I agree that the idea behind the patch is useful (NET_BUFSIZE is 68k, > which is too large for our goal of at most 4k stack allocation if we > never want to overflow a stack guard page), but you'll need a correct > working version of the patch. > > Yes, I will correct it and send next version of patch. > -- > Eric Blake eblake redhat com+1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
Re: [Qemu-devel] [Patch 1/1] net/net: Allocating Large sized arrays to heap
On 03/12/2016 01:39 AM, Pooja Dhannawat wrote: > Signed-off-by: Pooja Dhannawat> --- > net/net.c | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/net.c b/net/net.c > index b0c832e..5399758 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -709,16 +709,18 @@ 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 *buf; > uint8_t *buffer; > size_t offset; > > +buf = g_new(uint8_t, 1); If you're only going to malloc() one byte, it's more efficient to just stack-allocate it: uint8_t buf[1]; Did you mean to do: buf = g_new(uint8_t, NET_BUFSIZE) instead? > + > 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)); > +offset = iov_to_buf(iov, iovcnt, 0, (uint8_t *)buf, sizeof(uint8_t)); This is wrong. The old code used NET_BUFSIZE bytes for iov_to_buf(), the new code uses only one byte. By the way, sizeof(char) and sizeof(uint8_t) are pointless in code, as they are strictly equivalent to 1. I agree that the idea behind the patch is useful (NET_BUFSIZE is 68k, which is too large for our goal of at most 4k stack allocation if we never want to overflow a stack guard page), but you'll need a correct working version of the patch. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Patch 1/1] net/net: Allocating Large sized arrays to heap
could you explain why we need this change ? On 03/12/2016 04:39 PM, Pooja Dhannawat wrote: Signed-off-by: Pooja Dhannawat--- net/net.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/net/net.c b/net/net.c index b0c832e..5399758 100644 --- a/net/net.c +++ b/net/net.c @@ -709,16 +709,18 @@ 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 *buf; uint8_t *buffer; size_t offset; +buf = g_new(uint8_t, 1); + 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)); +offset = iov_to_buf(iov, iovcnt, 0, (uint8_t *)buf, sizeof(uint8_t)); } if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { @@ -726,6 +728,8 @@ static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov, } else { forget to release 'buf' return nc->info->receive(nc, buffer, offset); } + +g_free(buf); } ssize_t qemu_deliver_packet_iov(NetClientState *sender, -- Best regards. Li Zhijian
[Qemu-devel] [Patch 1/1] net/net: Allocating Large sized arrays to heap
Signed-off-by: Pooja Dhannawat--- net/net.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/net/net.c b/net/net.c index b0c832e..5399758 100644 --- a/net/net.c +++ b/net/net.c @@ -709,16 +709,18 @@ 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 *buf; uint8_t *buffer; size_t offset; +buf = g_new(uint8_t, 1); + 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)); +offset = iov_to_buf(iov, iovcnt, 0, (uint8_t *)buf, sizeof(uint8_t)); } if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { @@ -726,6 +728,8 @@ static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov, } else { return nc->info->receive(nc, buffer, offset); } + +g_free(buf); } ssize_t qemu_deliver_packet_iov(NetClientState *sender, -- 2.5.0