>
> > 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,
Ciara
>
> >
> > > + 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
