Re: [Qemu-trivial] [Qemu-devel] [PATCH] net/tap: Allocating Large sized arrays to heap

[Christian Borntraeger]

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 <address@hidden>
>>> ---
>>>   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) {
>>>
>>
>>
>>
>>
>