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

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

[Elena Afanasova]

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

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Richard Henderson]

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~

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Philippe Mathieu-Daudé]

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]),
> 

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Greg Kurz]

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]),

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Li Qiang]

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[David Gibson]

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

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]),

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

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Elena Afanasova]

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]),

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Li Qiang]

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[David Gibson]

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

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

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[David Gibson]

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

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]),
> 

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

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Thomas Huth]

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Philippe Mathieu-Daudé]

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]),
> 

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Philippe Mathieu-Daudé]

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)

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Paolo Bonzini]

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[David Gibson]

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

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?

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

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

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[Elena Afanasova]

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.

Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[David Gibson]

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

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.

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

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