diff mbox series

hw/net: move allocation to the heap due to very large stack frame

Message ID 8f07132478469b35fb50a4706691e2b56b10a67b.camel@gmail.com
State New
Headers show
Series hw/net: move allocation to the heap due to very large stack frame | expand

Commit Message

Elena Afanasova Oct. 9, 2020, 2:02 p.m. UTC 
From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
From: Elena Afanasova <eafanasova@gmail.com>
Date: Fri, 9 Oct 2020 06:41:36 -0700
Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
 frame

Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
---
 hw/net/spapr_llan.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Richard Henderson Oct. 9, 2020, 2:14 p.m. UTC | #1 
On 10/9/20 9:02 AM, Elena Afanasova wrote:
> From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
> From: Elena Afanasova <eafanasova@gmail.com>
> Date: Fri, 9 Oct 2020 06:41:36 -0700
> Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
>  frame
> 
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> ---
>  hw/net/spapr_llan.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Philippe Mathieu-Daudé Oct. 9, 2020, 2:25 p.m. UTC | #2 
On 10/9/20 4:02 PM, Elena Afanasova wrote:
>  From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
> From: Elena Afanasova <eafanasova@gmail.com>
> Date: Fri, 9 Oct 2020 06:41:36 -0700
> Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
>   frame
> 
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   hw/net/spapr_llan.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index 2093f1bad0..581320a0e7 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -688,7 +688,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>       SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>       SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>       unsigned total_len;
> -    uint8_t *lbuf, *p;
> +    uint8_t *p;
> +    g_autofree uint8_t *lbuf = NULL;
>       int i, nbufs;
>       int ret;
>   
> @@ -729,7 +730,7 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>           return H_RESOURCE;
>       }
>   
> -    lbuf = alloca(total_len);
> +    lbuf = g_malloc(total_len);
>       p = lbuf;
>       for (i = 0; i < nbufs; i++) {
>           ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
>
Greg Kurz Oct. 9, 2020, 2:48 p.m. UTC | #3 
On Fri, 09 Oct 2020 07:02:56 -0700
Elena Afanasova <eafanasova@gmail.com> wrote:

> From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
> From: Elena Afanasova <eafanasova@gmail.com>
> Date: Fri, 9 Oct 2020 06:41:36 -0700
> Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
>  frame
> 

Something seems to have gone wrong with the message body here, which
results in an awkward changelog... but maybe someone can fix that when
applying the patch.

> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/net/spapr_llan.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index 2093f1bad0..581320a0e7 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -688,7 +688,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>      SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>      unsigned total_len;
> -    uint8_t *lbuf, *p;
> +    uint8_t *p;
> +    g_autofree uint8_t *lbuf = NULL;
>      int i, nbufs;
>      int ret;
>  
> @@ -729,7 +730,7 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>          return H_RESOURCE;
>      }
>  
> -    lbuf = alloca(total_len);
> +    lbuf = g_malloc(total_len);
>      p = lbuf;
>      for (i = 0; i < nbufs; i++) {
>          ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
Li Qiang Oct. 9, 2020, 2:55 p.m. UTC | #4 
Elena Afanasova <eafanasova@gmail.com> 于2020年10月9日周五 下午10:03写道：
>
> From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
> From: Elena Afanasova <eafanasova@gmail.com>
> Date: Fri, 9 Oct 2020 06:41:36 -0700
> Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
>  frame
>
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/net/spapr_llan.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index 2093f1bad0..581320a0e7 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -688,7 +688,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>      SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>      unsigned total_len;
> -    uint8_t *lbuf, *p;
> +    uint8_t *p;
> +    g_autofree uint8_t *lbuf = NULL;
>      int i, nbufs;
>      int ret;
>
> @@ -729,7 +730,7 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>          return H_RESOURCE;
>      }
>
> -    lbuf = alloca(total_len);
> +    lbuf = g_malloc(total_len);
>      p = lbuf;
>      for (i = 0; i < nbufs; i++) {
>          ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
> --
> 2.25.1
>
>
>
David Gibson Oct. 10, 2020, 6:07 a.m. UTC | #5 
On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
> >From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
> From: Elena Afanasova <eafanasova@gmail.com>
> Date: Fri, 9 Oct 2020 06:41:36 -0700
> Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
>  frame

Patch looks fine, but some more details of the motivation would be
nice.  I wouldn't have thought that the size of a network packet
counted as a "very large" stack frame by userspace standards.

> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> ---
>  hw/net/spapr_llan.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index 2093f1bad0..581320a0e7 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -688,7 +688,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>      SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>      unsigned total_len;
> -    uint8_t *lbuf, *p;
> +    uint8_t *p;
> +    g_autofree uint8_t *lbuf = NULL;
>      int i, nbufs;
>      int ret;
>  
> @@ -729,7 +730,7 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>          return H_RESOURCE;
>      }
>  
> -    lbuf = alloca(total_len);
> +    lbuf = g_malloc(total_len);
>      p = lbuf;
>      for (i = 0; i < nbufs; i++) {
>          ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
Elena Afanasova Oct. 10, 2020, 3:53 p.m. UTC | #6 
On Sat, 2020-10-10 at 17:07 +1100, David Gibson wrote:
> On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
> > > From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00
> > > 2001
> > From: Elena Afanasova <eafanasova@gmail.com>
> > Date: Fri, 9 Oct 2020 06:41:36 -0700
> > Subject: [PATCH] hw/net: move allocation to the heap due to very
> > large stack
> >  frame
> 
> Patch looks fine, but some more details of the motivation would be
> nice.  I wouldn't have thought that the size of a network packet
> counted as a "very large" stack frame by userspace standards.
> 

gcc with wstack-usage warns that stack frame size may be ~65Kbytes

> > Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> > ---
> >  hw/net/spapr_llan.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > index 2093f1bad0..581320a0e7 100644
> > --- a/hw/net/spapr_llan.c
> > +++ b/hw/net/spapr_llan.c
> > @@ -688,7 +688,8 @@ static target_ulong
> > h_send_logical_lan(PowerPCCPU *cpu,
> >      SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus,
> > reg);
> >      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
> >      unsigned total_len;
> > -    uint8_t *lbuf, *p;
> > +    uint8_t *p;
> > +    g_autofree uint8_t *lbuf = NULL;
> >      int i, nbufs;
> >      int ret;
> >  
> > @@ -729,7 +730,7 @@ static target_ulong
> > h_send_logical_lan(PowerPCCPU *cpu,
> >          return H_RESOURCE;
> >      }
> >  
> > -    lbuf = alloca(total_len);
> > +    lbuf = g_malloc(total_len);
> >      p = lbuf;
> >      for (i = 0; i < nbufs; i++) {
> >          ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
Li Qiang Oct. 11, 2020, 2:23 a.m. UTC | #7 
David Gibson <david@gibson.dropbear.id.au> 于2020年10月10日周六 下午2:34写道：
>
> On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
> > >From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
> > From: Elena Afanasova <eafanasova@gmail.com>
> > Date: Fri, 9 Oct 2020 06:41:36 -0700
> > Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
> >  frame
>
> Patch looks fine, but some more details of the motivation would be
> nice.  I wouldn't have thought that the size of a network packet
> counted as a "very large" stack frame by userspace standards.
>

It is also a best practice to avoid large stack allocation according.
-->https://wiki.sei.cmu.edu/confluence/display/c/MEM05-C.+Avoid+large+stack+allocations

Though I don't see any issue here.

Thanks,
Li Qiang

> > Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> > ---
> >  hw/net/spapr_llan.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > index 2093f1bad0..581320a0e7 100644
> > --- a/hw/net/spapr_llan.c
> > +++ b/hw/net/spapr_llan.c
> > @@ -688,7 +688,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
> >      SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> >      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
> >      unsigned total_len;
> > -    uint8_t *lbuf, *p;
> > +    uint8_t *p;
> > +    g_autofree uint8_t *lbuf = NULL;
> >      int i, nbufs;
> >      int ret;
> >
> > @@ -729,7 +730,7 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
> >          return H_RESOURCE;
> >      }
> >
> > -    lbuf = alloca(total_len);
> > +    lbuf = g_malloc(total_len);
> >      p = lbuf;
> >      for (i = 0; i < nbufs; i++) {
> >          ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson
David Gibson Oct. 12, 2020, 5:28 a.m. UTC | #8 
On Sun, Oct 11, 2020 at 10:23:49AM +0800, Li Qiang wrote:
> David Gibson <david@gibson.dropbear.id.au> 于2020年10月10日周六 下午2:34写道：
> >
> > On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
> > > >From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
> > > From: Elena Afanasova <eafanasova@gmail.com>
> > > Date: Fri, 9 Oct 2020 06:41:36 -0700
> > > Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
> > >  frame
> >
> > Patch looks fine, but some more details of the motivation would be
> > nice.  I wouldn't have thought that the size of a network packet
> > counted as a "very large" stack frame by userspace standards.
> >
> 
> It is also a best practice to avoid large stack allocation according.
> -->https://wiki.sei.cmu.edu/confluence/display/c/MEM05-C.+Avoid+large+stack+allocations

Hm, yeah, it's not really clear what "large" means in that context.
It mostly seems to be concerned with allocations controlled by an
external attacker, in which case we could be talking up to INT_MAX.
Even with jumbo frames the most we're talking here is ~64kiB.

> 
> Though I don't see any issue here.
> 
> Thanks,
> Li Qiang
> 
> > > Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> > > ---
> > >  hw/net/spapr_llan.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > > index 2093f1bad0..581320a0e7 100644
> > > --- a/hw/net/spapr_llan.c
> > > +++ b/hw/net/spapr_llan.c
> > > @@ -688,7 +688,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
> > >      SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> > >      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
> > >      unsigned total_len;
> > > -    uint8_t *lbuf, *p;
> > > +    uint8_t *p;
> > > +    g_autofree uint8_t *lbuf = NULL;
> > >      int i, nbufs;
> > >      int ret;
> > >
> > > @@ -729,7 +730,7 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
> > >          return H_RESOURCE;
> > >      }
> > >
> > > -    lbuf = alloca(total_len);
> > > +    lbuf = g_malloc(total_len);
> > >      p = lbuf;
> > >      for (i = 0; i < nbufs; i++) {
> > >          ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
> >
>
David Gibson Oct. 12, 2020, 5:30 a.m. UTC | #9 
On Sat, Oct 10, 2020 at 08:53:00AM -0700, Elena Afanasova wrote:
> On Sat, 2020-10-10 at 17:07 +1100, David Gibson wrote:
> > On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
> > > > From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00
> > > > 2001
> > > From: Elena Afanasova <eafanasova@gmail.com>
> > > Date: Fri, 9 Oct 2020 06:41:36 -0700
> > > Subject: [PATCH] hw/net: move allocation to the heap due to very
> > > large stack
> > >  frame
> > 
> > Patch looks fine, but some more details of the motivation would be
> > nice.  I wouldn't have thought that the size of a network packet
> > counted as a "very large" stack frame by userspace standards.
> > 
> 
> gcc with wstack-usage warns that stack frame size may be ~65Kbytes

AFAICT, -Wstack-usage takes a parameter giving what size it will
complain about.  What was that value, and what was the rationale for
choosing it?

> 
> > > Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> > > ---
> > >  hw/net/spapr_llan.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > > index 2093f1bad0..581320a0e7 100644
> > > --- a/hw/net/spapr_llan.c
> > > +++ b/hw/net/spapr_llan.c
> > > @@ -688,7 +688,8 @@ static target_ulong
> > > h_send_logical_lan(PowerPCCPU *cpu,
> > >      SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus,
> > > reg);
> > >      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
> > >      unsigned total_len;
> > > -    uint8_t *lbuf, *p;
> > > +    uint8_t *p;
> > > +    g_autofree uint8_t *lbuf = NULL;
> > >      int i, nbufs;
> > >      int ret;
> > >  
> > > @@ -729,7 +730,7 @@ static target_ulong
> > > h_send_logical_lan(PowerPCCPU *cpu,
> > >          return H_RESOURCE;
> > >      }
> > >  
> > > -    lbuf = alloca(total_len);
> > > +    lbuf = g_malloc(total_len);
> > >      p = lbuf;
> > >      for (i = 0; i < nbufs; i++) {
> > >          ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
>
Thomas Huth Oct. 12, 2020, 10:44 a.m. UTC | #10 
On 12/10/2020 07.30, David Gibson wrote:
> On Sat, Oct 10, 2020 at 08:53:00AM -0700, Elena Afanasova wrote:
>> On Sat, 2020-10-10 at 17:07 +1100, David Gibson wrote:
>>> On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
>>>>> From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00
>>>>> 2001
>>>> From: Elena Afanasova <eafanasova@gmail.com>
>>>> Date: Fri, 9 Oct 2020 06:41:36 -0700
>>>> Subject: [PATCH] hw/net: move allocation to the heap due to very
>>>> large stack
>>>>  frame
>>>
>>> Patch looks fine, but some more details of the motivation would be
>>> nice.  I wouldn't have thought that the size of a network packet
>>> counted as a "very large" stack frame by userspace standards.
>>>
>>
>> gcc with wstack-usage warns that stack frame size may be ~65Kbytes
> 
> AFAICT, -Wstack-usage takes a parameter giving what size it will
> complain about.  What was that value, and what was the rationale for
> choosing it?

I think this is one of the tasks from:

 https://wiki.qemu.org/Contribute/BiteSizedTasks#Compiler-driven_cleanups

It has been added by Paolo in 2016:

 https://wiki.qemu.org/index.php?title=Contribute/BiteSizedTasks&diff=5368&oldid=5367

... so maybe Paolo can comment on the size that has been chosen here...?

 Thomas
Philippe Mathieu-Daudé Oct. 12, 2020, 11:09 a.m. UTC | #11 
On 10/10/20 8:07 AM, David Gibson wrote:
> On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
>> >From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
>> From: Elena Afanasova <eafanasova@gmail.com>
>> Date: Fri, 9 Oct 2020 06:41:36 -0700
>> Subject: [PATCH] hw/net: move allocation to the heap due to very large stack
>>   frame
> 
> Patch looks fine, but some more details of the motivation would be
> nice.  I wouldn't have thought that the size of a network packet
> counted as a "very large" stack frame by userspace standards.

Maybe academia doing research on "super jumbo frames"?

"Super jumbo frames ... increase the path MTU of high-performance
national research and education networks from 1500 bytes to 9000
bytes or so, a subsequent increase, possibly to 64,000 bytes"

(https://en.wikipedia.org/wiki/Jumbo_frame#Super_jumbo_frames)

> 
>> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
>> ---
>>   hw/net/spapr_llan.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
>> index 2093f1bad0..581320a0e7 100644
>> --- a/hw/net/spapr_llan.c
>> +++ b/hw/net/spapr_llan.c
>> @@ -688,7 +688,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>>       SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>>       SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>>       unsigned total_len;
>> -    uint8_t *lbuf, *p;
>> +    uint8_t *p;
>> +    g_autofree uint8_t *lbuf = NULL;
>>       int i, nbufs;
>>       int ret;
>>   
>> @@ -729,7 +730,7 @@ static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
>>           return H_RESOURCE;
>>       }
>>   
>> -    lbuf = alloca(total_len);
>> +    lbuf = g_malloc(total_len);
>>       p = lbuf;
>>       for (i = 0; i < nbufs; i++) {
>>           ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
>
Philippe Mathieu-Daudé Oct. 12, 2020, 11:48 a.m. UTC | #12 
On 10/12/20 1:09 PM, Philippe Mathieu-Daudé wrote:
> On 10/10/20 8:07 AM, David Gibson wrote:
>> On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
>>> >From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
>>> From: Elena Afanasova <eafanasova@gmail.com>
>>> Date: Fri, 9 Oct 2020 06:41:36 -0700
>>> Subject: [PATCH] hw/net: move allocation to the heap due to very 
>>> large stack
>>>   frame
>>
>> Patch looks fine, but some more details of the motivation would be
>> nice.  I wouldn't have thought that the size of a network packet
>> counted as a "very large" stack frame by userspace standards.
> 
> Maybe academia doing research on "super jumbo frames"?
> 
> "Super jumbo frames ... increase the path MTU of high-performance
> national research and education networks from 1500 bytes to 9000
> bytes or so, a subsequent increase, possibly to 64,000 bytes"
> 
> (https://en.wikipedia.org/wiki/Jumbo_frame#Super_jumbo_frames)

The one I was actually looking for is the IPv6 jumbogram:

"An optional feature of IPv6, the jumbo payload option, allows the 
exchange of packets with payloads of up to one byte less than 4 GiB,
by making use of a 32-bit length field."

(https://en.wikipedia.org/wiki/Jumbogram)
Paolo Bonzini Oct. 12, 2020, 1:45 p.m. UTC | #13 
On 12/10/20 12:44, Thomas Huth wrote:
> I think this is one of the tasks from:
> 
>  https://wiki.qemu.org/Contribute/BiteSizedTasks#Compiler-driven_cleanups
> 
> It has been added by Paolo in 2016:
> 
>  https://wiki.qemu.org/index.php?title=Contribute/BiteSizedTasks&diff=5368&oldid=5367
> 
> ... so maybe Paolo can comment on the size that has been chosen here...?

I used 16K, mostly because it is a nice round number.  8k is too small
due to PATH_MAX-sized variables.  16k seemed to be plenty and triggered
in few-enough places that the cleanup is viable.

Paolo
David Gibson Oct. 13, 2020, 5:32 a.m. UTC | #14 
On Mon, Oct 12, 2020 at 03:45:02PM +0200, Paolo Bonzini wrote:
> On 12/10/20 12:44, Thomas Huth wrote:
> > I think this is one of the tasks from:
> > 
> >  https://wiki.qemu.org/Contribute/BiteSizedTasks#Compiler-driven_cleanups
> > 
> > It has been added by Paolo in 2016:
> > 
> >  https://wiki.qemu.org/index.php?title=Contribute/BiteSizedTasks&diff=5368&oldid=5367
> > 
> > ... so maybe Paolo can comment on the size that has been chosen here...?
> 
> I used 16K, mostly because it is a nice round number.  8k is too small
> due to PATH_MAX-sized variables.  16k seemed to be plenty and triggered
> in few-enough places that the cleanup is viable.

Ok.  Why are large stack frames bad in qemu?
Elena Afanasova Oct. 14, 2020, 2:15 p.m. UTC | #15 
On Tue, 2020-10-13 at 16:32 +1100, David Gibson wrote:
> On Mon, Oct 12, 2020 at 03:45:02PM +0200, Paolo Bonzini wrote:
> > On 12/10/20 12:44, Thomas Huth wrote:
> > > I think this is one of the tasks from:
> > > 
> > >  
> > > https://wiki.qemu.org/Contribute/BiteSizedTasks#Compiler-driven_cleanups
> > > 
> > > It has been added by Paolo in 2016:
> > > 
> > >  
> > > https://wiki.qemu.org/index.php?title=Contribute/BiteSizedTasks&diff=5368&oldid=5367
> > > 
> > > ... so maybe Paolo can comment on the size that has been chosen
> > > here...?
> > 
> > I used 16K, mostly because it is a nice round number.  8k is too
> > small
> > due to PATH_MAX-sized variables.  16k seemed to be plenty and
> > triggered
> > in few-enough places that the cleanup is viable.
> 
> Ok.  Why are large stack frames bad in qemu?
> 

I think that the main issue here is alloca() because it can lead to UB.
David Gibson Oct. 16, 2020, 12:32 a.m. UTC | #16 
On Wed, Oct 14, 2020 at 07:15:47AM -0700, Elena Afanasova wrote:
> On Tue, 2020-10-13 at 16:32 +1100, David Gibson wrote:
> > On Mon, Oct 12, 2020 at 03:45:02PM +0200, Paolo Bonzini wrote:
> > > On 12/10/20 12:44, Thomas Huth wrote:
> > > > I think this is one of the tasks from:
> > > > 
> > > >  
> > > > https://wiki.qemu.org/Contribute/BiteSizedTasks#Compiler-driven_cleanups
> > > > 
> > > > It has been added by Paolo in 2016:
> > > > 
> > > >  
> > > > https://wiki.qemu.org/index.php?title=Contribute/BiteSizedTasks&diff=5368&oldid=5367
> > > > 
> > > > ... so maybe Paolo can comment on the size that has been chosen
> > > > here...?
> > > 
> > > I used 16K, mostly because it is a nice round number.  8k is too
> > > small
> > > due to PATH_MAX-sized variables.  16k seemed to be plenty and
> > > triggered
> > > in few-enough places that the cleanup is viable.
> > 
> > Ok.  Why are large stack frames bad in qemu?
> > 
> 
> I think that the main issue here is alloca() because it can lead to UB.

That's a fair point.  I've applied the patch to ppc-for-5.2, with a
tweak to the commit message.
diff mbox series

Patch

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index 2093f1bad0..581320a0e7 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -688,7 +688,8 @@  static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
     SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
     SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
     unsigned total_len;
-    uint8_t *lbuf, *p;
+    uint8_t *p;
+    g_autofree uint8_t *lbuf = NULL;
     int i, nbufs;
     int ret;
 
@@ -729,7 +730,7 @@  static target_ulong h_send_logical_lan(PowerPCCPU *cpu,
         return H_RESOURCE;
     }
 
-    lbuf = alloca(total_len);
+    lbuf = g_malloc(total_len);
     p = lbuf;
     for (i = 0; i < nbufs; i++) {
         ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),