[Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Qemu-devel] [PATCH v4] 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..fd7f39f 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, 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 v4] socket: Allocating Large sized arrays to heap

[Stefan Hajnoczi]

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

On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat wrote:
> @@ -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));

This change is unrelated to allocating buf1 on the heap.  What is the
purpose of this line?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Pooja Dhannawat]

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

On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat wrote:
> > @@ -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));
>
> This change is unrelated to allocating buf1 on the heap.  What is the
> purpose of this line?
>

I moved buf from stack to Heap, used g_new(), but I got your point if we
need to initialize it with 0 then I have to keep that one.

Other wise doing so it gets whatever garbage it has already.

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

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Paolo Bonzini]

On 17/03/2016 16:31, Pooja Dhannawat wrote:
> 
> 
> On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <stefanha@gmail.com
> <mailto:stefanha@gmail.com>> wrote:
> 
>     On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat wrote:
>     > @@ -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));
> 
>     This change is unrelated to allocating buf1 on the heap.  What is the
>     purpose of this line?
> 
> 
> I moved buf from stack to Heap, used g_new(), but I got your point if we
> need to initialize it with 0 then I have to keep that one.
> 
> Other wise doing so it gets whatever garbage it has already.  

This is s->buf, not buf.  Also, the BiteSizedTasks page says "Make the
stack array smaller and allocate on the heap in the rare case that the
data does not fit in the small array".

Paolo

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Stefan Hajnoczi]

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

On Thu, Mar 17, 2016 at 11:50:15PM +0100, Paolo Bonzini wrote:
> 
> 
> On 17/03/2016 16:31, Pooja Dhannawat wrote:
> > 
> > 
> > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <stefanha@gmail.com
> > <mailto:stefanha@gmail.com>> wrote:
> > 
> >     On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat wrote:
> >     > @@ -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));
> > 
> >     This change is unrelated to allocating buf1 on the heap.  What is the
> >     purpose of this line?
> > 
> > 
> > I moved buf from stack to Heap, used g_new(), but I got your point if we
> > need to initialize it with 0 then I have to keep that one.
> > 
> > Other wise doing so it gets whatever garbage it has already.  
> 
> This is s->buf, not buf.

Exactly, they are different variables.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Pooja Dhannawat]

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

On Fri, Mar 18, 2016 at 3:05 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Mar 17, 2016 at 11:50:15PM +0100, Paolo Bonzini wrote:
> >
> >
> > On 17/03/2016 16:31, Pooja Dhannawat wrote:
> > >
> > >
> > > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <stefanha@gmail.com
> > > <mailto:stefanha@gmail.com>> wrote:
> > >
> > >     On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat wrote:
> > >     > @@ -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));
> > >
> > >     This change is unrelated to allocating buf1 on the heap.  What is
> the
> > >     purpose of this line?
> > >
> > >
> > > I moved buf from stack to Heap, used g_new(), but I got your point if
> we
> > > need to initialize it with 0 then I have to keep that one.
> > >
> > > Other wise doing so it gets whatever garbage it has already.
> >
> > This is s->buf, not buf.
>
> Exactly, they are different variables.
>
> Yes.
The line should not be removed.
Extremely sorry for the noise and my terrible confusion.
Will mail the updated patch.

> Stefan
>

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

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Pooja Dhannawat]

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

On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

>
>
> On 17/03/2016 16:31, Pooja Dhannawat wrote:
> >
> >
> > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <stefanha@gmail.com
> > <mailto:stefanha@gmail.com>> wrote:
> >
> >     On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat wrote:
> >     > @@ -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));
> >
> >     This change is unrelated to allocating buf1 on the heap.  What is the
> >     purpose of this line?
> >
> >
> > I moved buf from stack to Heap, used g_new(), but I got your point if we
> > need to initialize it with 0 then I have to keep that one.
> >
> > Other wise doing so it gets whatever garbage it has already.
>
> This is s->buf, not buf.  Also, the BiteSizedTasks page says "Make the
> stack array smaller and allocate on the heap in the rare case that the
> data does not fit in the small array".
>
> So here, should I check with stack consumption(size of array) and if it is
greater than accepted level, then only keep on heap?
If no, Can you please help me with this one?

> Paolo
>

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

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Paolo Bonzini]

On 18/03/2016 12:24, Pooja Dhannawat wrote:
> 
> 
> On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini <pbonzini@redhat.com
> <mailto:pbonzini@redhat.com>> wrote:
> 
> 
> 
>     On 17/03/2016 16:31, Pooja Dhannawat wrote:
>     >
>     >
>     > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <stefanha@gmail.com <mailto:stefanha@gmail.com>
>     > <mailto:stefanha@gmail.com <mailto:stefanha@gmail.com>>> wrote:
>     >
>     >     On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat wrote:
>     >     > @@ -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));
>     >
>     >     This change is unrelated to allocating buf1 on the heap.  What is the
>     >     purpose of this line?
>     >
>     >
>     > I moved buf from stack to Heap, used g_new(), but I got your point if we
>     > need to initialize it with 0 then I have to keep that one.
>     >
>     > Other wise doing so it gets whatever garbage it has already.
> 
>     This is s->buf, not buf.  Also, the BiteSizedTasks page says "Make the
>     stack array smaller and allocate on the heap in the rare case that the
>     data does not fit in the small array".
> 
> So here, should I check with stack consumption(size of array) and if it
> is greater than accepted level, then only keep on heap?

If it is greater than the accepted level, the on-stack buffer is not
used and you allocate one that has the right size on the heap.

Paolo

> If no, Can you please help me with this one?
> 
>     Paolo
> 
> 

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Jaya Tiwari]

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

On Fri, Mar 18, 2016 at 5:21 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

>
>
> On 18/03/2016 12:24, Pooja Dhannawat wrote:
> >
> >
> > On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini <pbonzini@redhat.com
> > <mailto:pbonzini@redhat.com>> wrote:
> >
> >
> >
> >     On 17/03/2016 16:31, Pooja Dhannawat wrote:
> >     >
> >     >
> >     > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <
> stefanha@gmail.com <mailto:stefanha@gmail.com>
> >     > <mailto:stefanha@gmail.com <mailto:stefanha@gmail.com>>> wrote:
> >     >
> >     >     On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat
> wrote:
> >     >     > @@ -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));
> >     >
> >     >     This change is unrelated to allocating buf1 on the heap.  What
> is the
> >     >     purpose of this line?
> >     >
> >     >
> >     > I moved buf from stack to Heap, used g_new(), but I got your point
> if we
> >     > need to initialize it with 0 then I have to keep that one.
> >     >
> >     > Other wise doing so it gets whatever garbage it has already.
> >
> >     This is s->buf, not buf.  Also, the BiteSizedTasks page says "Make
> the
> >     stack array smaller and allocate on the heap in the rare case that
> the
> >     data does not fit in the small array".
> >
> > So here, should I check with stack consumption(size of array) and if it
> > is greater than accepted level, then only keep on heap?
>
> If it is greater than the accepted level, the on-stack buffer is not
> used and you allocate one that has the right size on the heap.
>
Yes Okay. Thank you for the comments.
I had one more question.
size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
The one above returns bytes read into buf1 (if large then bytes equivalent
to len(buf1) is read) ?
If true, size is the correct measure of buf1? Hence, I should compare the
allowed stack size to "size" variable?

>
> Paolo
>
> > If no, Can you please help me with this one?
> >
> >     Paolo
> >
> >
>
>


-- 
Regards,
Jaya

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

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Jaya Tiwari]

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

On Fri, Mar 18, 2016 at 6:57 PM, Jaya Tiwari <tiwari.jaya18@gmail.com>
wrote:

>
>
> On Fri, Mar 18, 2016 at 5:21 PM, Paolo Bonzini <pbonzini@redhat.com>
> wrote:
>
>>
>>
>> On 18/03/2016 12:24, Pooja Dhannawat wrote:
>> >
>> >
>> > On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini <pbonzini@redhat.com
>> > <mailto:pbonzini@redhat.com>> wrote:
>> >
>> >
>> >
>> >     On 17/03/2016 16:31, Pooja Dhannawat wrote:
>> >     >
>> >     >
>> >     > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <
>> stefanha@gmail.com <mailto:stefanha@gmail.com>
>> >     > <mailto:stefanha@gmail.com <mailto:stefanha@gmail.com>>> wrote:
>> >     >
>> >     >     On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat
>> wrote:
>> >     >     > @@ -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));
>> >     >
>> >     >     This change is unrelated to allocating buf1 on the heap.
>> What is the
>> >     >     purpose of this line?
>> >     >
>> >     >
>> >     > I moved buf from stack to Heap, used g_new(), but I got your
>> point if we
>> >     > need to initialize it with 0 then I have to keep that one.
>> >     >
>> >     > Other wise doing so it gets whatever garbage it has already.
>> >
>> >     This is s->buf, not buf.  Also, the BiteSizedTasks page says "Make
>> the
>> >     stack array smaller and allocate on the heap in the rare case that
>> the
>> >     data does not fit in the small array".
>> >
>> > So here, should I check with stack consumption(size of array) and if it
>> > is greater than accepted level, then only keep on heap?
>>
>> If it is greater than the accepted level, the on-stack buffer is not
>> used and you allocate one that has the right size on the heap.
>>
> Yes Okay. Thank you for the comments.
> I had one more question.
> size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
> The one above returns bytes read into buf1 (if large then bytes equivalent
> to len(buf1) is read) ?
> If true, size is the correct measure of buf1? Hence, I should compare the
> allowed stack size to "size" variable?
>
   So isnt here size should be compared to "size" varibale paolo?

>
>> Paolo
>>
>> > If no, Can you please help me with this one?
>> >
>> >     Paolo
>> >
>> >
>>
>>
>
>
> --
> Regards,
> Jaya
>



-- 
Regards,
Jaya

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

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Pooja Dhannawat]

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

On Fri, Mar 18, 2016 at 6:59 PM, Jaya Tiwari <tiwari.jaya18@gmail.com>
wrote:

>
> On Fri, Mar 18, 2016 at 6:57 PM, Jaya Tiwari <tiwari.jaya18@gmail.com>
> wrote:
>
>>
>>
>> On Fri, Mar 18, 2016 at 5:21 PM, Paolo Bonzini <pbonzini@redhat.com>
>> wrote:
>>
>>>
>>>
>>> On 18/03/2016 12:24, Pooja Dhannawat wrote:
>>> >
>>> >
>>> > On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini <pbonzini@redhat.com
>>> > <mailto:pbonzini@redhat.com>> wrote:
>>> >
>>> >
>>> >
>>> >     On 17/03/2016 16:31, Pooja Dhannawat wrote:
>>> >     >
>>> >     >
>>> >     > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <
>>> stefanha@gmail.com <mailto:stefanha@gmail.com>
>>> >     > <mailto:stefanha@gmail.com <mailto:stefanha@gmail.com>>> wrote:
>>> >     >
>>> >     >     On Tue, Mar 15, 2016 at 09:29:58PM +0530, Pooja Dhannawat
>>> wrote:
>>> >     >     > @@ -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));
>>> >     >
>>> >     >     This change is unrelated to allocating buf1 on the heap.
>>> What is the
>>> >     >     purpose of this line?
>>> >     >
>>> >     >
>>> >     > I moved buf from stack to Heap, used g_new(), but I got your
>>> point if we
>>> >     > need to initialize it with 0 then I have to keep that one.
>>> >     >
>>> >     > Other wise doing so it gets whatever garbage it has already.
>>> >
>>> >     This is s->buf, not buf.  Also, the BiteSizedTasks page says "Make
>>> the
>>> >     stack array smaller and allocate on the heap in the rare case that
>>> the
>>> >     data does not fit in the small array".
>>> >
>>> > So here, should I check with stack consumption(size of array) and if it
>>> > is greater than accepted level, then only keep on heap?
>>>
>>> If it is greater than the accepted level, the on-stack buffer is not
>>> used and you allocate one that has the right size on the heap.
>>>
>> Yes Okay. Thank you for the comments.
>> I had one more question.
>> size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
>> The one above returns bytes read into buf1 (if large then bytes
>> equivalent to len(buf1) is read) ?
>> If true, size is the correct measure of buf1? Hence, I should compare the
>> allowed stack size to "size" variable?
>>
>    So isnt here size should be compared to "size" varibale paolo?
>
So instead of comparing with NET_BUFSIZE, should I compare with "size"
variable?
Can you help me with this?

>
>>> Paolo
>>>
>>> > If no, Can you please help me with this one?
>>> >
>>> >     Paolo
>>> >
>>> >
>>>
>>>
>>
>>
>> --
>> Regards,
>> Jaya
>>
>
>
>
> --
> Regards,
> Jaya
>

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

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Paolo Bonzini]

On 18/03/2016 14:49, Pooja Dhannawat wrote:
> 
> 
>             If it is greater than the accepted level, the on-stack
>             buffer is not
>             used and you allocate one that has the right size on the heap.
> 
>         Yes Okay. Thank you for the comments.
>         I had one more question.
>         size = qemu_recv(s->fd, buf1, sizeof(buf1), 0); 
>         The one above returns bytes read into buf1 (if large then bytes
>         equivalent to len(buf1) is read) ?
>         If true, size is the correct measure of buf1? Hence, I should
>         compare the allowed stack size to "size" variable?
> 
>        So isnt here size should be compared to "size" varibale paolo? 
> 
> So instead of comparing with NET_BUFSIZE, should I compare with "size"
> variable? Can you help me with this? 

I was a bit confused myself; this function actually is a bit different
from the others because it does not really need a large buffer.  The
function already takes care of moving data in pieces from buf1 to
s->buf.  If you make the buffer smaller, the only change is that one
call to net_socket_send will process fewer bytes.

So you can just send a trivial patch that changes the size of the array
to something like 2048.

Thanks, and sorry for putting you on a false track!

Paolo

Re: [Qemu-devel] [PATCH v4] socket: Allocating Large sized arrays to heap

[Pooja Dhannawat]

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

On Friday, March 18, 2016, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 18/03/2016 14:49, Pooja Dhannawat wrote:
>>
>>
>>             If it is greater than the accepted level, the on-stack
>>             buffer is not
>>             used and you allocate one that has the right size on the
heap.
>>
>>         Yes Okay. Thank you for the comments.
>>         I had one more question.
>>         size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
>>         The one above returns bytes read into buf1 (if large then bytes
>>         equivalent to len(buf1) is read) ?
>>         If true, size is the correct measure of buf1? Hence, I should
>>         compare the allowed stack size to "size" variable?
>>
>>        So isnt here size should be compared to "size" varibale paolo?
>>
>> So instead of comparing with NET_BUFSIZE, should I compare with "size"
>> variable? Can you help me with this?
>
> I was a bit confused myself; this function actually is a bit different
> from the others because it does not really need a large buffer.  The
> function already takes care of moving data in pieces from buf1 to
> s->buf.  If you make the buffer smaller, the only change is that one
> call to net_socket_send will process fewer bytes.
>
> So you can just send a trivial patch that changes the size of the array
> to something like 2048.
>
> Thanks, and sorry for putting you on a false track!
>
No, it's completely fine,  I really appreciate your help.
> Paolo
>

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