diff mbox

net/tap: Allocating Large sized arrays to heap

Message ID 1461633961-31713-1-git-send-email-zhoujie2011@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zhou Jie April 26, 2016, 1:26 a.m. UTC
net_init_tap has a huge stack usage of 8192 bytes approx.
Moving large arrays to heap to reduce stack usage.

Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
---
 net/tap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Jason Wang April 26, 2016, 7:14 a.m. UTC | #1
On 04/26/2016 09:26 AM, Zhou Jie wrote:
> net_init_tap has a huge stack usage of 8192 bytes approx.
> Moving large arrays to heap to reduce stack usage.
>
> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
> ---
>  net/tap.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 740e8a2..49817c7 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -769,8 +769,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>              return -1;
>          }
>      } else if (tap->has_fds) {
> -        char *fds[MAX_TAP_QUEUES];
> -        char *vhost_fds[MAX_TAP_QUEUES];
> +        char **fds = g_new(char *, MAX_TAP_QUEUES);
> +        char **vhost_fds = g_new(char *, MAX_TAP_QUEUES);
>          int nfds, nvhosts;
>  
>          if (tap->has_ifname || tap->has_script || tap->has_downscript ||
> @@ -818,6 +818,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>                  return -1;
>              }
>          }
> +        g_free(fds);
> +        g_free(vhost_fds);
>      } else if (tap->has_helper) {
>          if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>              tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) {

Apply to net-next.

Thanks
Christian Borntraeger April 26, 2016, 7:45 a.m. UTC | #2
On 04/26/2016 03:26 AM, Zhou Jie wrote:
> net_init_tap has a huge stack usage of 8192 bytes approx.
> Moving large arrays to heap to reduce stack usage.

I am wondering. Why is 8k a problem for a user space program? 

Please note that malloc/new like allocations are much more expensive
than stack allocation in terms of performance. This does not matter
here, but in your other patch that deals with the xmit function, I would
not be surprised if that actually harms performance.

Christian



> 
> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
> ---
>  net/tap.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index 740e8a2..49817c7 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -769,8 +769,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>              return -1;
>          }
>      } else if (tap->has_fds) {
> -        char *fds[MAX_TAP_QUEUES];
> -        char *vhost_fds[MAX_TAP_QUEUES];
> +        char **fds = g_new(char *, MAX_TAP_QUEUES);
> +        char **vhost_fds = g_new(char *, MAX_TAP_QUEUES);
>          int nfds, nvhosts;
> 
>          if (tap->has_ifname || tap->has_script || tap->has_downscript ||
> @@ -818,6 +818,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>                  return -1;
>              }
>          }
> +        g_free(fds);
> +        g_free(vhost_fds);
>      } else if (tap->has_helper) {
>          if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>              tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) {
>
Zhou Jie April 26, 2016, 8:26 a.m. UTC | #3
On 2016/4/26 15:45, Christian Borntraeger wrote:
> On 04/26/2016 03:26 AM, Zhou Jie wrote:
>> net_init_tap has a huge stack usage of 8192 bytes approx.
>> Moving large arrays to heap to reduce stack usage.
>
> I am wondering. Why is 8k a problem for a user space program?
For 64bit machine it will be 16k.

> Please note that malloc/new like allocations are much more expensive
> than stack allocation in terms of performance. This does not matter
> here, but in your other patch that deals with the xmit function, I would
> not be surprised if that actually harms performance.
OK. I will note it.

Sincerely,
Zhou Jie

>
> Christian
>
>
>
>>
>> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
>> ---
>>   net/tap.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 740e8a2..49817c7 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -769,8 +769,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>               return -1;
>>           }
>>       } else if (tap->has_fds) {
>> -        char *fds[MAX_TAP_QUEUES];
>> -        char *vhost_fds[MAX_TAP_QUEUES];
>> +        char **fds = g_new(char *, MAX_TAP_QUEUES);
>> +        char **vhost_fds = g_new(char *, MAX_TAP_QUEUES);
>>           int nfds, nvhosts;
>>
>>           if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>> @@ -818,6 +818,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>                   return -1;
>>               }
>>           }
>> +        g_free(fds);
>> +        g_free(vhost_fds);
>>       } else if (tap->has_helper) {
>>           if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>>               tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) {
>>
>
>
>
>
Christian Borntraeger April 26, 2016, 8:39 a.m. UTC | #4
On 04/26/2016 10:26 AM, Zhou Jie wrote:
> On 2016/4/26 15:45, Christian Borntraeger wrote:
>> On 04/26/2016 03:26 AM, Zhou Jie wrote:
>>> net_init_tap has a huge stack usage of 8192 bytes approx.
>>> Moving large arrays to heap to reduce stack usage.
>>
>> I am wondering. Why is 8k a problem for a user space program?
> For 64bit machine it will be 16k.

Even that does not matter. The userspace stack is on most Linuxes 
somewhere between 8MB and unlimited. Reducing stack usage makes 
sense for the kernel as the kernel stack is limited to 8k or 16k.
For userspace it does also make sense for things like thread stacks 
or coroutines, where we have a much smaller stack (well, still 1MB 
for coroutines as of today) and if we are not on a hot path.
Regarding hot pathes: There is a reason why people use jemalloc or 
tcmalloc. The allocators try to use per-thread arena, but still each
allocation might cause significant cross cpu/thread traffic. And 
having a allocator that scales well across many CPUs is not trivial.

> 
>> Please note that malloc/new like allocations are much more expensive
>> than stack allocation in terms of performance. This does not matter
>> here, but in your other patch that deals with the xmit function, I would
>> not be surprised if that actually harms performance.
> OK. I will note it.
> 
> Sincerely,
> Zhou Jie
> 
>>
>> Christian
>>
>>
>>
>>>
>>> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
>>> ---
>>>   net/tap.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/tap.c b/net/tap.c
>>> index 740e8a2..49817c7 100644
>>> --- a/net/tap.c
>>> +++ b/net/tap.c
>>> @@ -769,8 +769,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>>               return -1;
>>>           }
>>>       } else if (tap->has_fds) {
>>> -        char *fds[MAX_TAP_QUEUES];
>>> -        char *vhost_fds[MAX_TAP_QUEUES];
>>> +        char **fds = g_new(char *, MAX_TAP_QUEUES);
>>> +        char **vhost_fds = g_new(char *, MAX_TAP_QUEUES);
>>>           int nfds, nvhosts;
>>>
>>>           if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>>> @@ -818,6 +818,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>>                   return -1;
>>>               }
>>>           }
>>> +        g_free(fds);
>>> +        g_free(vhost_fds);
>>>       } else if (tap->has_helper) {
>>>           if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>>>               tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) {
>>>
>>
>>
>>
>>
>
diff mbox

Patch

diff --git a/net/tap.c b/net/tap.c
index 740e8a2..49817c7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -769,8 +769,8 @@  int net_init_tap(const NetClientOptions *opts, const char *name,
             return -1;
         }
     } else if (tap->has_fds) {
-        char *fds[MAX_TAP_QUEUES];
-        char *vhost_fds[MAX_TAP_QUEUES];
+        char **fds = g_new(char *, MAX_TAP_QUEUES);
+        char **vhost_fds = g_new(char *, MAX_TAP_QUEUES);
         int nfds, nvhosts;
 
         if (tap->has_ifname || tap->has_script || tap->has_downscript ||
@@ -818,6 +818,8 @@  int net_init_tap(const NetClientOptions *opts, const char *name,
                 return -1;
             }
         }
+        g_free(fds);
+        g_free(vhost_fds);
     } else if (tap->has_helper) {
         if (tap->has_ifname || tap->has_script || tap->has_downscript ||
             tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) {