| Message ID | 1461722868-11624-1-git-send-email-zhoujie2011@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
>
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>
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.
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
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;
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.
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 --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;
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(-)