Re: [Qemu-devel] [PATCH v2 1/1] socket: Allocating Large sized arrays to heap

[Markus Armbruster]

Pooja Dhannawat  writes:

> On Mon, Mar 14, 2016 at 10:20 PM, Daniel P. Berrange 
> wrote:
>
>> On Mon, Mar 14, 2016 at 10:07:53PM +0530, Pooja Dhannawat wrote:
>> > net_socket_send has a huge stack usage of 69712 bytes approx.
>> > Moving large arrays to heap to reduce stack usage.
>> >
>> > Signed-off-by: Pooja Dhannawat 
>> > ---
>> >  net/socket.c | 8 +---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/net/socket.c b/net/socket.c
>> > index e32e3cb..3fcd7a6 100644
>> > --- a/net/socket.c
>> > +++ b/net/socket.c
>> > @@ -147,10 +147,10 @@ static void net_socket_send(void *opaque)
>> >  NetSocketState *s = opaque;
>> >  int size, err;
>> >  unsigned l;
>> > -uint8_t buf1[NET_BUFSIZE];
>> > +uint8_t *buf1 = g_new(uint8_t, NET_BUFSIZE);
>>
>> You're allocating NET_BUFSIZE worth of uint8_t's
>>
>> I didn't get you clear.
>
>
>> >  const uint8_t *buf;
>> >
>> > -size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
>> > +size = qemu_recv(s->fd, (uint8_t *)buf1, sizeof(uint8_t), 0);
>>
>> But only reading 1 byte which is clearly wrong. You likely wanted
>> NET_BUFSIZE here, not sizeof(uint8_t)
>>
>> Correct me If I am wrong. This should also work :
> size = qemu_recv(s->fd, (uint8_t *)buf1, NET_BUFSIZE * sizeof(uint8_t), 0);

An expression of type "T[]" deteriorates to "T *" without a cast.
Therefore, your cast of buf1 to uint8_t * is redundant and clutters the
code, please drop it.

In general, try hard to avoid casts not just to reduce clutter, but also
because casts can hide mistakes from the type checker.

sizeof(uint8_t) is always one.  Multiplying by it clutters the code,
please drop it.

Re: [Qemu-devel] [PATCH v2 1/1] socket: Allocating Large sized arrays to heap

[Daniel P. Berrange]

On Mon, Mar 14, 2016 at 10:44:22PM +0530, Pooja Dhannawat wrote:
> On Mon, Mar 14, 2016 at 10:20 PM, Daniel P. Berrange 
> wrote:
> 
> > On Mon, Mar 14, 2016 at 10:07:53PM +0530, Pooja Dhannawat wrote:
> > > net_socket_send has a huge stack usage of 69712 bytes approx.
> > > Moving large arrays to heap to reduce stack usage.
> > >
> > > Signed-off-by: Pooja Dhannawat 
> > > ---
> > >  net/socket.c | 8 +---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/socket.c b/net/socket.c
> > > index e32e3cb..3fcd7a6 100644
> > > --- a/net/socket.c
> > > +++ b/net/socket.c
> > > @@ -147,10 +147,10 @@ static void net_socket_send(void *opaque)
> > >  NetSocketState *s = opaque;
> > >  int size, err;
> > >  unsigned l;
> > > -uint8_t buf1[NET_BUFSIZE];
> > > +uint8_t *buf1 = g_new(uint8_t, NET_BUFSIZE);
> >
> > You're allocating NET_BUFSIZE worth of uint8_t's
> >
> > I didn't get you clear.
> 
> 
> > >  const uint8_t *buf;
> > >
> > > -size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
> > > +size = qemu_recv(s->fd, (uint8_t *)buf1, sizeof(uint8_t), 0);
> >
> > But only reading 1 byte which is clearly wrong. You likely wanted
> > NET_BUFSIZE here, not sizeof(uint8_t)
> >
> > Correct me If I am wrong. This should also work :
> size = qemu_recv(s->fd, (uint8_t *)buf1, NET_BUFSIZE * sizeof(uint8_t), 0);

Sure, if you want to add a pointless 'multiply by 1' to the code,
which you didn't bother doing for the g_new call.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 1/1] socket: Allocating Large sized arrays to heap

[Pooja Dhannawat]

On Mon, Mar 14, 2016 at 10:20 PM, Daniel P. Berrange 
wrote:

> On Mon, Mar 14, 2016 at 10:07:53PM +0530, Pooja Dhannawat wrote:
> > net_socket_send has a huge stack usage of 69712 bytes approx.
> > Moving large arrays to heap to reduce stack usage.
> >
> > Signed-off-by: Pooja Dhannawat 
> > ---
> >  net/socket.c | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/socket.c b/net/socket.c
> > index e32e3cb..3fcd7a6 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -147,10 +147,10 @@ static void net_socket_send(void *opaque)
> >  NetSocketState *s = opaque;
> >  int size, err;
> >  unsigned l;
> > -uint8_t buf1[NET_BUFSIZE];
> > +uint8_t *buf1 = g_new(uint8_t, NET_BUFSIZE);
>
> You're allocating NET_BUFSIZE worth of uint8_t's
>
> I didn't get you clear.


> >  const uint8_t *buf;
> >
> > -size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
> > +size = qemu_recv(s->fd, (uint8_t *)buf1, sizeof(uint8_t), 0);
>
> But only reading 1 byte which is clearly wrong. You likely wanted
> NET_BUFSIZE here, not sizeof(uint8_t)
>
> Correct me If I am wrong. This should also work :
size = qemu_recv(s->fd, (uint8_t *)buf1, NET_BUFSIZE * sizeof(uint8_t), 0);


> Regards,
> Daniel
> --
> |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/
> :|
> |: http://libvirt.org  -o- http://virt-manager.org
> :|
> |: http://autobuild.org   -o- http://search.cpan.org/~danberr/
> :|
> |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc
> :|
>

Re: [Qemu-devel] [PATCH v2 1/1] socket: Allocating Large sized arrays to heap

[Daniel P. Berrange]

On Mon, Mar 14, 2016 at 10:07:53PM +0530, Pooja Dhannawat wrote:
> net_socket_send has a huge stack usage of 69712 bytes approx.
> Moving large arrays to heap to reduce stack usage.
> 
> Signed-off-by: Pooja Dhannawat 
> ---
>  net/socket.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index e32e3cb..3fcd7a6 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -147,10 +147,10 @@ static void net_socket_send(void *opaque)
>  NetSocketState *s = opaque;
>  int size, err;
>  unsigned l;
> -uint8_t buf1[NET_BUFSIZE];
> +uint8_t *buf1 = g_new(uint8_t, NET_BUFSIZE);

You're allocating NET_BUFSIZE worth of uint8_t's

>  const uint8_t *buf;
>  
> -size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
> +size = qemu_recv(s->fd, (uint8_t *)buf1, sizeof(uint8_t), 0);

But only reading 1 byte which is clearly wrong. You likely wanted
NET_BUFSIZE here, not sizeof(uint8_t)

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

[Qemu-devel] [PATCH v2 1/1] socket: Allocating Large sized arrays to heap

[Pooja Dhannawat]

net_socket_send has a huge stack usage of 69712 bytes approx.
Moving large arrays to heap to reduce stack usage.

Signed-off-by: Pooja Dhannawat 
---
 net/socket.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index e32e3cb..3fcd7a6 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -147,10 +147,10 @@ static void net_socket_send(void *opaque)
 NetSocketState *s = opaque;
 int size, err;
 unsigned l;
-uint8_t buf1[NET_BUFSIZE];
+uint8_t *buf1 = g_new(uint8_t, NET_BUFSIZE);
 const uint8_t *buf;
 
-size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
+size = qemu_recv(s->fd, (uint8_t *)buf1, sizeof(uint8_t), 0);
 if (size < 0) {
 err = socket_error();
 if (err != EWOULDBLOCK)
@@ -170,8 +170,8 @@ static void net_socket_send(void *opaque)
 s->index = 0;
 s->packet_len = 0;
 s->nc.link_down = true;
-memset(s->buf, 0, sizeof(s->buf));
 memset(s->nc.info_str, 0, sizeof(s->nc.info_str));
+g_free(buf1);
 
 return;
 }
@@ -222,6 +222,8 @@ static void net_socket_send(void *opaque)
 break;
 }
 }
+
+g_free(buf1);
 }
 
 static void net_socket_send_dgram(void *opaque)
-- 
2.5.0