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

[Stefan Hajnoczi]

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  > > 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


signature.asc
Description: PGP signature

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

[Pooja Dhannawat]

On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini  wrote:

>
>
> On 17/03/2016 16:31, Pooja Dhannawat wrote:
> >
> >
> > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi  > > 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
>

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

[Pooja Dhannawat]

On Fri, Mar 18, 2016 at 3:05 PM, Stefan Hajnoczi  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  > > > 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
>

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

[Pooja Dhannawat]

On Fri, Mar 18, 2016 at 6:59 PM, Jaya Tiwari 
wrote:

>
> On Fri, Mar 18, 2016 at 6:57 PM, Jaya Tiwari 
> wrote:
>
>>
>>
>> On Fri, Mar 18, 2016 at 5:21 PM, Paolo Bonzini 
>> wrote:
>>
>>>
>>>
>>> On 18/03/2016 12:24, Pooja Dhannawat wrote:
>>> >
>>> >
>>> > On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini >> > > wrote:
>>> >
>>> >
>>> >
>>> > On 17/03/2016 16:31, Pooja Dhannawat wrote:
>>> > >
>>> > >
>>> > > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <
>>> stefa...@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
>

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  > 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

[Paolo Bonzini]

On 18/03/2016 12:24, Pooja Dhannawat wrote:
> 
> 
> On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini  > wrote:
> 
> 
> 
> On 17/03/2016 16:31, Pooja Dhannawat wrote:
> >
> >
> > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi  
> > >> 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

[Pooja Dhannawat]

On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi  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.

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

[Stefan Hajnoczi]

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?


signature.asc
Description: PGP signature

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

[Jaya Tiwari]

On Fri, Mar 18, 2016 at 5:21 PM, Paolo Bonzini  wrote:

>
>
> On 18/03/2016 12:24, Pooja Dhannawat wrote:
> >
> >
> > On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini  > > wrote:
> >
> >
> >
> > On 17/03/2016 16:31, Pooja Dhannawat wrote:
> > >
> > >
> > > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <
> stefa...@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

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

[Pooja Dhannawat]

On Friday, March 18, 2016, Paolo Bonzini  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
>

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

[Jaya Tiwari]

On Fri, Mar 18, 2016 at 6:57 PM, Jaya Tiwari 
wrote:

>
>
> On Fri, Mar 18, 2016 at 5:21 PM, Paolo Bonzini 
> wrote:
>
>>
>>
>> On 18/03/2016 12:24, Pooja Dhannawat wrote:
>> >
>> >
>> > On Fri, Mar 18, 2016 at 4:20 AM, Paolo Bonzini > > > wrote:
>> >
>> >
>> >
>> > On 17/03/2016 16:31, Pooja Dhannawat wrote:
>> > >
>> > >
>> > > On Thu, Mar 17, 2016 at 8:20 PM, Stefan Hajnoczi <
>> stefa...@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

[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 
---
 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