> 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. > > > + 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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev