[dpdk-dev] [RFC] vhost user: add error handling for fd > 1023

[Christian Ehrhardt]

I like the approach as well to go for the fix for robustness first.

I was accidentally able to find another testcase to hit the same root cause.
Adding guests with 15 vhost_user based NICs each while having rxq for
openvswitch-dpdk set to 4 and multiqueue for the guest devices at 4 already
breaks when adding the thirds such guests.
That is way earlier than I would have expected the fd's to be exhausted but
still the same root cause, so just another test for the same.

In prep to the wider check to the patch a minor review question from me:
On the section of rte_vhost_driver_register that now detects if there were
issues we might want to close the socketfd as well when bailing out.
Otherwise we would just have another source of fd leaks or would that be
reused later on even now that we have freed vserver-path and vserver itself?

       ret = fdset_add(&g_vhost_server.fdset, vserver->listenfd,
               vserver_new_vq_conn, NULL, vserver);
       if (ret < 0) {
               pthread_mutex_unlock(&g_vhost_server.server_mutex);
               RTE_LOG(ERR, VHOST_CONFIG,
                               "failed to add listen fd %d to vhost server
fdset\n",
                               vserver->listenfd);
               free(vserver->path);
+              close(vserver->listenfd);
               free(vserver);
               return -1;
       }


Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Mon, Apr 11, 2016 at 8:06 AM, Patrik Andersson R <
patrik.r.andersson at ericsson.com> wrote:

> I fully agree with this course of action.
>
> Thank you,
>
> Patrik
>
>
>
> On 04/08/2016 08:47 AM, Xie, Huawei wrote:
>
>> On 4/7/2016 10:52 PM, Christian Ehrhardt wrote:
>>
>>> I totally agree to that there is no deterministic rule what to expect.
>>> The only rule is that #fd certainly always is > #vhost_user devices.
>>> In various setup variants I've crossed fd 1024 anywhere between 475
>>> and 970 vhost_user ports.
>>>
>>> Once the discussion continues and we have an updates version of the
>>> patch with some more agreement I hope I can help to test it.
>>>
>> Thanks. Let us first temporarily fix this problem for robustness, then
>> we consider whether upgrade to (e)poll.
>> Will check the patch in detail later. Basically it should work but need
>> check whether we need extra fixes elsewhere.
>>
>
>