[Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Zhou Jie]

open_eth_start_xmit has a huge stack usage of 65536 bytes approx.
Moving large arrays to heap to reduce stack usage.

Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
---
 hw/net/opencores_eth.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/net/opencores_eth.c b/hw/net/opencores_eth.c
index c6094fb..fa0a4e7 100644
--- a/hw/net/opencores_eth.c
+++ b/hw/net/opencores_eth.c
@@ -483,7 +483,8 @@ static NetClientInfo net_open_eth_info = {
 
 static void open_eth_start_xmit(OpenEthState *s, desc *tx)
 {
-    uint8_t buf[65536];
+    uint8_t *buf = NULL;
+    uint8_t buffer[0x600];
     unsigned len = GET_FIELD(tx->len_flags, TXD_LEN);
     unsigned tx_len = len;
 
@@ -498,6 +499,11 @@ static void open_eth_start_xmit(OpenEthState *s, desc *tx)
 
     trace_open_eth_start_xmit(tx->buf_ptr, len, tx_len);
 
+    if (tx_len > 0x600) {
+        buf = g_new(uint8_t, tx_len);
+    } else {
+        buf = buffer;
+    }
     if (len > tx_len) {
         len = tx_len;
     }
@@ -506,6 +512,9 @@ static void open_eth_start_xmit(OpenEthState *s, desc *tx)
         memset(buf + len, 0, tx_len - len);
     }
     qemu_send_packet(qemu_get_queue(s->nic), buf, tx_len);
+    if (tx_len > 0x600) {
+        g_free(buf);
+    }
 
     if (tx->len_flags & TXD_WR) {
         s->tx_desc = 0;
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Zhou Jie]

Hi Max,

    When I committed another patch which named as
    "hw/net/virtio-net: Allocating Large sized arrays to heap" .

    Christian Borntraeger said that 16k is usually perfectly fine
    for a userspace stack and doing allocations in a hot path
    might actually hurt performance.

    Although the size is 65536 bytes here,
    I think open_eth_start_xmit is in a hot path.
    So, it is OK, if you think that this patch should not be applied.

Sincerely,
Zhou Jie

On 2016/4/27 10:07, Zhou Jie wrote:
> open_eth_start_xmit has a huge stack usage of 65536 bytes approx.
> Moving large arrays to heap to reduce stack usage.
>
> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
> ---
>   hw/net/opencores_eth.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/opencores_eth.c b/hw/net/opencores_eth.c
> index c6094fb..fa0a4e7 100644
> --- a/hw/net/opencores_eth.c
> +++ b/hw/net/opencores_eth.c
> @@ -483,7 +483,8 @@ static NetClientInfo net_open_eth_info = {
>
>   static void open_eth_start_xmit(OpenEthState *s, desc *tx)
>   {
> -    uint8_t buf[65536];
> +    uint8_t *buf = NULL;
> +    uint8_t buffer[0x600];
>       unsigned len = GET_FIELD(tx->len_flags, TXD_LEN);
>       unsigned tx_len = len;
>
> @@ -498,6 +499,11 @@ static void open_eth_start_xmit(OpenEthState *s, desc *tx)
>
>       trace_open_eth_start_xmit(tx->buf_ptr, len, tx_len);
>
> +    if (tx_len > 0x600) {
> +        buf = g_new(uint8_t, tx_len);
> +    } else {
> +        buf = buffer;
> +    }
>       if (len > tx_len) {
>           len = tx_len;
>       }
> @@ -506,6 +512,9 @@ static void open_eth_start_xmit(OpenEthState *s, desc *tx)
>           memset(buf + len, 0, tx_len - len);
>       }
>       qemu_send_packet(qemu_get_queue(s->nic), buf, tx_len);
> +    if (tx_len > 0x600) {
> +        g_free(buf);
> +    }
>
>       if (tx->len_flags & TXD_WR) {
>           s->tx_desc = 0;
>

-- 
------------------------------------------------
周潔
Dept 1
No. 6 Wenzhu Road,
Nanjing, 210012, China
TEL：+86+25-86630566-8557
FUJITSU INTERNAL：7998-8557
E-Mail：zhoujie2011@cn.fujitsu.com
------------------------------------------------

Re: [Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Max Filippov]

Hi Zhou,

On Wed, Apr 27, 2016 at 10:18:56AM +0800, Zhou Jie wrote:
>    When I committed another patch which named as
>    "hw/net/virtio-net: Allocating Large sized arrays to heap" .
> 
>    Christian Borntraeger said that 16k is usually perfectly fine
>    for a userspace stack and doing allocations in a hot path
>    might actually hurt performance.
> 
>    Although the size is 65536 bytes here,
>    I think open_eth_start_xmit is in a hot path.
>    So, it is OK, if you think that this patch should not be applied.

With Linux as guest OS we shouldn't see any allocations
as it doesn't send huge packets, so I think this patch is fine.
I can take it through the xtensa tree if you don't have other
plan.

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Zhou Jie]

On 2016/4/27 10:46, Max Filippov wrote:
> Hi Zhou,
>
> On Wed, Apr 27, 2016 at 10:18:56AM +0800, Zhou Jie wrote:
>>     When I committed another patch which named as
>>     "hw/net/virtio-net: Allocating Large sized arrays to heap" .
>>
>>     Christian Borntraeger said that 16k is usually perfectly fine
>>     for a userspace stack and doing allocations in a hot path
>>     might actually hurt performance.
>>
>>     Although the size is 65536 bytes here,
>>     I think open_eth_start_xmit is in a hot path.
>>     So, it is OK, if you think that this patch should not be applied.
>
> With Linux as guest OS we shouldn't see any allocations
> as it doesn't send huge packets, so I think this patch is fine.
> I can take it through the xtensa tree if you don't have other
> plan.
>
OK, Thanks

Sincerely,
Zhou Jie

Re: [Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Max Filippov]

On Wed, Apr 27, 2016 at 10:07:48AM +0800, Zhou Jie wrote:
> open_eth_start_xmit has a huge stack usage of 65536 bytes approx.
> Moving large arrays to heap to reduce stack usage.
> 
> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
> ---
>  hw/net/opencores_eth.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Wei, Jiangang]

On Wed, 2016-04-27 at 10:07 +0800, Zhou Jie wrote:
> open_eth_start_xmit has a huge stack usage of 65536 bytes approx.
> Moving large arrays to heap to reduce stack usage.
> 
> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
> ---
>  hw/net/opencores_eth.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/opencores_eth.c b/hw/net/opencores_eth.c
> index c6094fb..fa0a4e7 100644
> --- a/hw/net/opencores_eth.c
> +++ b/hw/net/opencores_eth.c
> @@ -483,7 +483,8 @@ static NetClientInfo net_open_eth_info = {
>  
>  static void open_eth_start_xmit(OpenEthState *s, desc *tx)
>  {
> -    uint8_t buf[65536];
> +    uint8_t *buf = NULL;
> +    uint8_t buffer[0x600];
Hi,

I'm curious about 0x600.
How do you determine this size?
IMO, Max's suggestion looks more reasonable.
(1536 bytes, maximal frame length when HUGEN bit is not set in MODER)

Regards,
wei
>      unsigned len = GET_FIELD(tx->len_flags, TXD_LEN);
>      unsigned tx_len = len;
>  
> @@ -498,6 +499,11 @@ static void open_eth_start_xmit(OpenEthState *s, desc *tx)
>  
>      trace_open_eth_start_xmit(tx->buf_ptr, len, tx_len);
>  
> +    if (tx_len > 0x600) {
> +        buf = g_new(uint8_t, tx_len);
> +    } else {
> +        buf = buffer;
> +    }
>      if (len > tx_len) {
>          len = tx_len;
>      }
> @@ -506,6 +512,9 @@ static void open_eth_start_xmit(OpenEthState *s, desc *tx)
>          memset(buf + len, 0, tx_len - len);
>      }
>      qemu_send_packet(qemu_get_queue(s->nic), buf, tx_len);
> +    if (tx_len > 0x600) {
> +        g_free(buf);
> +    }
>  
>      if (tx->len_flags & TXD_WR) {
>          s->tx_desc = 0;

Re: [Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Max Filippov]

Hi Wei,

On Wed, Apr 27, 2016 at 03:27:47AM +0000, Wei, Jiangang wrote:
> On Wed, 2016-04-27 at 10:07 +0800, Zhou Jie wrote:
> >  static void open_eth_start_xmit(OpenEthState *s, desc *tx)
> >  {
> > -    uint8_t buf[65536];
> > +    uint8_t *buf = NULL;
> > +    uint8_t buffer[0x600];
> Hi,
> 
> I'm curious about 0x600.
> How do you determine this size?
> IMO, Max's suggestion looks more reasonable.
> (1536 bytes, maximal frame length when HUGEN bit is not set in MODER)

This is the same value. Opencores 10/100 ethernet spec uses both
decimal and hexadecimal notation.

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

[Wei, Jiangang]

On Wed, 2016-04-27 at 06:44 +0300, Max Filippov wrote:
> Hi Wei,
> 
> On Wed, Apr 27, 2016 at 03:27:47AM +0000, Wei, Jiangang wrote:
> > On Wed, 2016-04-27 at 10:07 +0800, Zhou Jie wrote:
> > >  static void open_eth_start_xmit(OpenEthState *s, desc *tx)
> > >  {
> > > -    uint8_t buf[65536];
> > > +    uint8_t *buf = NULL;
> > > +    uint8_t buffer[0x600];
> > Hi,
> > 
> > I'm curious about 0x600.
> > How do you determine this size?
> > IMO, Max's suggestion looks more reasonable.
> > (1536 bytes, maximal frame length when HUGEN bit is not set in MODER)
> 
> This is the same value. Opencores 10/100 ethernet spec uses both
> decimal and hexadecimal notation.
I got it
Thanks for your reply.

Wei