diff mbox

[v2] hw/net/opencores_eth: Allocating Large sized arrays to heap

Message ID 1461722868-11624-1-git-send-email-zhoujie2011@cn.fujitsu.com
State New
Headers show

Commit Message

Zhou Jie April 27, 2016, 2:07 a.m. UTC 
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(-)

Comments

Zhou Jie April 27, 2016, 2:18 a.m. UTC | #1 
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;
>
Max Filippov April 27, 2016, 2:37 a.m. UTC | #2 
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>
Max Filippov April 27, 2016, 2:46 a.m. UTC | #3 
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.
Zhou Jie April 27, 2016, 2:51 a.m. UTC | #4 
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
Wei Jiangang April 27, 2016, 3:27 a.m. UTC | #5 
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;
Max Filippov April 27, 2016, 3:44 a.m. UTC | #6 
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.
Wei Jiangang April 27, 2016, 3:48 a.m. UTC | #7 
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
diff mbox

Patch

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;