[Qemu-devel] [Patch 1/1] net/socket: Allocating Large sized arrays to heap

[Qemu-devel] [Patch 1/1] net/socket: Allocating Large sized arrays to heap

[Pooja Dhannawat]

Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>
---
 net/socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index e32e3cb..483dcac 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, 1);
     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,7 +170,6 @@ 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));
 
         return;
@@ -222,6 +221,7 @@ static void net_socket_send(void *opaque)
             break;
         }
     }
+    g_free(buf1);
 }
 
 static void net_socket_send_dgram(void *opaque)
-- 
2.5.0

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

[Li Zhijian]

Don't get why we need this changes, could you explain more for it?
and it seem it's not exactly correct, see below.

On 03/12/2016 11:28 AM, Pooja Dhannawat wrote:
> Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>
> ---
>   net/socket.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index e32e3cb..483dcac 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, 1);
>       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);
one byte for each reading, it looks expensive


>       if (size < 0) {
>           err = socket_error();
>           if (err != EWOULDBLOCK)
> @@ -170,7 +170,6 @@ 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));
>
forget to release buf1.

Thanks
Li ZHijian

>           return;
> @@ -222,6 +221,7 @@ static void net_socket_send(void *opaque)
>               break;
>           }
>       }
> +    g_free(buf1);
>   }
>
>   static void net_socket_send_dgram(void *opaque)
>

[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 <dhannawatpooja1@gmail.com>
---
 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

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 <dhannawatpooja1@gmail.com>
> ---
>  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 :|

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

[Pooja Dhannawat]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On Mon, Mar 14, 2016 at 10:20 PM, Daniel P. Berrange <berrange@redhat.com>
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 <dhannawatpooja1@gmail.com>
> > ---
> >  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
> :|
>

[-- Attachment #2: Type: text/html, Size: 3274 bytes --]

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 <berrange@redhat.com>
> 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 <dhannawatpooja1@gmail.com>
> > > ---
> > >  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 :|

[Qemu-devel] [PATCH v3] 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 <dhannawatpooja1@gmail.com>
---
 net/socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index e32e3cb..d7fa8ce 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, NET_BUFSIZE, 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

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

[Markus Armbruster]

Pooja Dhannawat <dhannawatpooja1@gmail.com> writes:

> On Mon, Mar 14, 2016 at 10:20 PM, Daniel P. Berrange <berrange@redhat.com>
> 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 <dhannawatpooja1@gmail.com>
>> > ---
>> >  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.