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

[Ilya Maximets]

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.

> +            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?

> +    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'.

> +        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.

>  
> -    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.