| Message ID | 1458057598-11041-1-git-send-email-dhannawatpooja1@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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.
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
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
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 >
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 >
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 > >
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 > > > > > >
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 >
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 >
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
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 >
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)
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(-)