[ovs-dev] [PATCH RFC 2/2] netdev-dpdk: Add vHost User PMD

[Ilya Maximets]

On 28.10.2016 13:44, Loftus, Ciara wrote:
>>
>>> I'll post few comments to v4 here.
>>>
>>>>  static int
>>>> +dpdk_attach_vhost_pmd(struct netdev_dpdk *dev, int mode)
>>>> +{
>>>> +    char *devargs;
>>>> +    int err = 0;
>>>> +    uint8_t port_no = 0;
>>>> +    uint32_t driver_id = -1;
>>>> +
>>>> +    if (id_pool_alloc_id(dpdk_get_vhost_id_pool(), &driver_id)) {
>>>> +        devargs = xasprintf("net_vhost%u,iface=%s,queues=%i,client=%i",
>>>> +                 driver_id, dev->vhost_id,
>>>> +                 MIN(OVS_VHOST_MAX_QUEUE_NUM,
>>> RTE_MAX_QUEUES_PER_PORT),
>>>> +                 mode);
>>>> +        err = rte_eth_dev_attach(devargs, &port_no);
>>>> +        if (!err) {
>>>> +            dev->port_id = port_no;
>>>> +            dev->vhost_pmd_id = driver_id;
>>>> +        } else {
>>>
>>> id should be freed on error.
>>
>> Fixed in v5
>>
>>>
>>>> +            VLOG_ERR("Failed to attach vhost-user device %s to DPDK",
>>>> +                     dev->vhost_id);
>>>> +        }
>>>> +    } else {
>>>> +        VLOG_ERR("Unable to create vhost-user device %s - too many
>> vhost-
>>> user"
>>>> +                 "devices registered with PMD", dev->vhost_id);
>>>> +        err = ENODEV;
>>>> +    }
>>>> +
>>>> +    return err;
>>>> +}
>>>
>>> --------------
>>>
>>>>  static void
>>>>  netdev_dpdk_vhost_destruct(struct netdev *netdev)
>>>>  {
>>>>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>>> -    char *vhost_id;
>>>>
>>>>      ovs_mutex_lock(&dpdk_mutex);
>>>>      ovs_mutex_lock(&dev->mutex);
>>>>
>>>> -    /* Guest becomes an orphan if still attached. */
>>>> -    if (netdev_dpdk_get_vid(dev) >= 0
>>>> -        && !(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
>>>> -        VLOG_ERR("Removing port '%s' while vhost device still attached.",
>>>> -                 netdev->name);
>>>> -        VLOG_ERR("To restore connectivity after re-adding of port, VM on "
>>>> -                 "socket '%s' must be restarted.", dev->vhost_id);
>>>
>>> These log messages are useful. I think it's better to keep them somehow.
>>> Maybe we can check for link status here?
>>
>> Sure
>>
>>>
>>>> +    if (rte_eth_dev_detach(dev->port_id, dev->vhost_id)) {
>>>
>>> 'rte_eth_dev_detach()' will call 'dpdk_vhost_driver_unregister()' and
>>> this will lead to link status change if vhost still attached.
>>> And as soon as 'dpdk_mutex' and 'dev->mutex' are taken, there will be
>>> deadlock
>>> inside the callback.
>>>
>>> See 3f891bbea61d ("netdev-dpdk: Fix deadlock in destroy_device().") for
>>> details.
>>>
>>> The problem here is that we can't call 'rte_eth_dev_detach()' without 'dev-
>>>> mutex'.
>>
>> Ok got it - I'm posting a v5 without this fix. Expect it in the v6. Not sure how to
>> approach it just yet.
> 
> We might be ok here actually.
> 
> The LSC callback to OVS where we try to acquire the mutex a second time will not occur after rte_eth_dev_detach(). We will fail in DPDK before then:
> 
> DPDK: rte_eth_vhost.c:
> 
> static void
> destroy_device(int vid)
> {
>                 .....
> 
> 	rte_vhost_get_ifname(vid, ifname, sizeof(ifname));
> 	list = find_internal_resource(ifname); <--- port was removed from list during detach
> 	if (list == NULL) {
> 		RTE_LOG(ERR, PMD, "Invalid interface name: %s\n", ifname);
> 		return; <--- we fail here
> 	}
> 
>                 .....
> 
>                 callback_to_ovs_lsc(); <--- we never reach this
> }
> 

Thanks for checking this. But I found another thing:
You must unregister all the callbacks before detach to
avoid memory leak, because there is an allocation for
each registered callback inside DPDK.
After that detach should be completely safe.

Best regards, Ilya Maximets.

>>>
>>>> +        VLOG_ERR("Error removing vhost device %s", dev->vhost_id);
>>>> +    } else {
>>>> +        if (dev->type == DPDK_DEV_VHOST) {
>>>
>>> "} else if {" ?
>>>
>>>> +            fatal_signal_remove_file_to_unlink(dev->vhost_id);
>>>> +        }
>>>>      }
>>>> +    id_pool_free_id(dpdk_get_vhost_id_pool(), dev->vhost_pmd_id);
>>>
>>> I guess, that It's better to call 'free()' only if id was allocated.
>>
>> I will introduce a check in v5.
>>
>>>
>>>>
>>>> -    free(ovsrcu_get_protected(struct ingress_policer *,
>>>> -                              &dev->ingress_policer));
>>>> -
>>>> -    rte_free(dev->tx_q);
>>>> -    ovs_list_remove(&dev->list_node);
>>>> -    dpdk_mp_put(dev->dpdk_mp);
>>>> -
>>>> -    vhost_id = xstrdup(dev->vhost_id);
>>>> +    dpdk_destruct_helper(dev);
>>>>
>>>>      ovs_mutex_unlock(&dev->mutex);
>>>>      ovs_mutex_unlock(&dpdk_mutex);
>>>> -
>>>> -    if (dpdk_vhost_driver_unregister(dev, vhost_id)) {
>>>> -        VLOG_ERR("%s: Unable to unregister vhost driver for socket
>> '%s'.\n",
>>>> -                 netdev->name, vhost_id);
>>>> -    } else if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
>>>> -        /* OVS server mode - remove this socket from list for deletion */
>>>> -        fatal_signal_remove_file_to_unlink(vhost_id);
>>>> -    }
>>>> -    free(vhost_id);
>>>>  }
>>>
>>> Best regards, Ilya Maximets.
> 
> 
>