* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-09 14:02 [PATCH] hw/net: move allocation to the heap due to very large stack frame Elena Afanasova
@ 2020-10-09 14:14 ` Richard Henderson
2020-10-09 14:25 ` Philippe Mathieu-Daudé
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Richard Henderson @ 2020-10-09 14:14 UTC (permalink / raw)
To: Elena Afanasova, david, jasowang, qemu-ppc, qemu-devel; +Cc: qemu-trivial
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~
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-09 14:02 [PATCH] hw/net: move allocation to the heap due to very large stack frame Elena Afanasova
2020-10-09 14:14 ` Richard Henderson
@ 2020-10-09 14:25 ` Philippe Mathieu-Daudé
2020-10-09 14:48 ` Greg Kurz
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-09 14:25 UTC (permalink / raw)
To: Elena Afanasova, david, jasowang, qemu-ppc, qemu-devel; +Cc: qemu-trivial
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]),
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-09 14:02 [PATCH] hw/net: move allocation to the heap due to very large stack frame Elena Afanasova
2020-10-09 14:14 ` Richard Henderson
2020-10-09 14:25 ` Philippe Mathieu-Daudé
@ 2020-10-09 14:48 ` Greg Kurz
2020-10-09 14:55 ` Li Qiang
2020-10-10 6:07 ` David Gibson
4 siblings, 0 replies; 17+ messages in thread
From: Greg Kurz @ 2020-10-09 14:48 UTC (permalink / raw)
To: Elena Afanasova; +Cc: qemu-trivial, jasowang, qemu-ppc, qemu-devel, david
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]),
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-09 14:02 [PATCH] hw/net: move allocation to the heap due to very large stack frame Elena Afanasova
` (2 preceding siblings ...)
2020-10-09 14:48 ` Greg Kurz
@ 2020-10-09 14:55 ` Li Qiang
2020-10-10 6:07 ` David Gibson
4 siblings, 0 replies; 17+ messages in thread
From: Li Qiang @ 2020-10-09 14:55 UTC (permalink / raw)
To: Elena Afanasova
Cc: qemu-trivial, Jason Wang, qemu-ppc, Qemu Developers, David Gibson
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
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-09 14:02 [PATCH] hw/net: move allocation to the heap due to very large stack frame Elena Afanasova
` (3 preceding siblings ...)
2020-10-09 14:55 ` Li Qiang
@ 2020-10-10 6:07 ` David Gibson
2020-10-10 15:53 ` Elena Afanasova
` (2 more replies)
4 siblings, 3 replies; 17+ messages in thread
From: David Gibson @ 2020-10-10 6:07 UTC (permalink / raw)
To: Elena Afanasova; +Cc: qemu-trivial, jasowang, qemu-ppc, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-10 6:07 ` David Gibson
@ 2020-10-10 15:53 ` Elena Afanasova
2020-10-12 5:30 ` David Gibson
2020-10-11 2:23 ` Li Qiang
2020-10-12 11:09 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 17+ messages in thread
From: Elena Afanasova @ 2020-10-10 15:53 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-trivial, jasowang, qemu-ppc, qemu-devel
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]),
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-10 15:53 ` Elena Afanasova
@ 2020-10-12 5:30 ` David Gibson
2020-10-12 10:44 ` Thomas Huth
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2020-10-12 5:30 UTC (permalink / raw)
To: Elena Afanasova; +Cc: qemu-trivial, jasowang, qemu-ppc, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-12 5:30 ` David Gibson
@ 2020-10-12 10:44 ` Thomas Huth
2020-10-12 13:45 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Thomas Huth @ 2020-10-12 10:44 UTC (permalink / raw)
To: David Gibson, Elena Afanasova
Cc: qemu-trivial, Paolo Bonzini, jasowang, qemu-ppc, qemu-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-12 10:44 ` Thomas Huth
@ 2020-10-12 13:45 ` Paolo Bonzini
2020-10-13 5:32 ` David Gibson
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2020-10-12 13:45 UTC (permalink / raw)
To: Thomas Huth, David Gibson, Elena Afanasova
Cc: qemu-trivial, jasowang, qemu-ppc, qemu-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-12 13:45 ` Paolo Bonzini
@ 2020-10-13 5:32 ` David Gibson
2020-10-14 14:15 ` Elena Afanasova
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2020-10-13 5:32 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Thomas Huth, qemu-trivial, jasowang, qemu-devel, qemu-ppc,
Elena Afanasova
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-13 5:32 ` David Gibson
@ 2020-10-14 14:15 ` Elena Afanasova
2020-10-16 0:32 ` David Gibson
0 siblings, 1 reply; 17+ messages in thread
From: Elena Afanasova @ 2020-10-14 14:15 UTC (permalink / raw)
To: David Gibson, Paolo Bonzini
Cc: qemu-trivial, Thomas Huth, qemu-ppc, jasowang, qemu-devel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-14 14:15 ` Elena Afanasova
@ 2020-10-16 0:32 ` David Gibson
0 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2020-10-16 0:32 UTC (permalink / raw)
To: Elena Afanasova
Cc: Thomas Huth, qemu-trivial, jasowang, qemu-devel, qemu-ppc, Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-10 6:07 ` David Gibson
2020-10-10 15:53 ` Elena Afanasova
@ 2020-10-11 2:23 ` Li Qiang
2020-10-12 5:28 ` David Gibson
2020-10-12 11:09 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 17+ messages in thread
From: Li Qiang @ 2020-10-11 2:23 UTC (permalink / raw)
To: David Gibson
Cc: Elena Afanasova, qemu-trivial, Jason Wang, qemu-ppc, Qemu Developers
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-11 2:23 ` Li Qiang
@ 2020-10-12 5:28 ` David Gibson
0 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2020-10-12 5:28 UTC (permalink / raw)
To: Li Qiang
Cc: Elena Afanasova, qemu-trivial, Jason Wang, qemu-ppc, Qemu Developers
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-10 6:07 ` David Gibson
2020-10-10 15:53 ` Elena Afanasova
2020-10-11 2:23 ` Li Qiang
@ 2020-10-12 11:09 ` Philippe Mathieu-Daudé
2020-10-12 11:48 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-12 11:09 UTC (permalink / raw)
To: David Gibson, Elena Afanasova
Cc: qemu-trivial, jasowang, qemu-ppc, qemu-devel
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]),
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame
2020-10-12 11:09 ` Philippe Mathieu-Daudé
@ 2020-10-12 11:48 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-12 11:48 UTC (permalink / raw)
To: David Gibson, Elena Afanasova
Cc: qemu-trivial, jasowang, qemu-ppc, qemu-devel
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)
^ permalink raw reply [flat|nested] 17+ messages in thread