LGTM I agree with this safe approach. I did some basic testing with a Niantic NIC but I couldn't replicate the issue. Below some details on my testing.
To cause the issue I killed and restarted vswitchd only, so that it could find and run with an existing database with the devices already bound to dpdk. Then I deleted one dpdk port and re-added it later on. In both cases - with and without this patch - I couldn't see any difference, in particular when debugging the following functions: common_construct() netdev_dpdk_destruct() netdev_dpdk_process_devargs() With the patch, when I delete a port I don't see messages like Device '0000:xx:00.0' has been detached should I expect to see that? I saw that if I call ovs-appctl netdev-dpdk/detach 0000:xx:00.0 before deleting the port it works fine by displaying Device YYY is being used by interface... Remove it... Regards, -Antonio > -----Original Message----- > From: [email protected] [mailto:ovs-dev- > [email protected]] On Behalf Of Darrell Ball > Sent: Wednesday, July 26, 2017 6:10 AM > To: [email protected] > Cc: Ilya Maximets <[email protected]> > Subject: [ovs-dev] [patch_v1] dpdk: Fix device cleanup. > > Commit 5dcde09c80a8 was introduced to make detaching more > automatic without using an additional command. > > Sometimes, since commit 5dcde09c80a8, dpdk devices are > not detached when del-port is issued; command example: > > sudo ovs-vsctl del-port br0 dpdk1 > > This can happen when vswitchd is (re)started with an existing > database and devices are already bound to dpdk. > > A discussion is here: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-June/333462.html > along with a possible solution. > > Since we are nearing the end of a release, a safe approach is needed, > at this time. > One approach is to revert 5dcde09c80a8. This patch does not do that > but reinstates the command ovs-appctl netdev-dpdk/detach to handle > cases when del-port will not work. > > Fixes: 5dcde09c80a8 ("netdev-dpdk: Fix device leak on port deletion.") > CC: Ilya Maximets <[email protected]> > Signed-off-by: Darrell Ball <[email protected]> > --- > Documentation/howto/dpdk.rst | 12 ++++++++++ > lib/netdev-dpdk.c | 52 > +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst > index af01d3e..3c198a2 100644 > --- a/Documentation/howto/dpdk.rst > +++ b/Documentation/howto/dpdk.rst > @@ -331,6 +331,18 @@ Detaching will be performed while processing del-port > command:: > > $ ovs-vsctl del-port dpdkx > > +Sometimes, the del-port command may not detach the device. > +Detaching can be confirmed by the appearance of an INFO log. > +For example:: > + > + Device '0000:04:00.0' has been detached > + > +If the log is not seen, then the port can be detached using:: > + > +$ ovs-appctl netdev-dpdk/detach 0000:01:00.0 > + > +Again, detaching can be confirmed by the above INFO log. > + > This feature is not supported with VFIO and does not work with some NICs. > For more information please refer to the `DPDK Port Hotplug Framework > > <http://dpdk.org/doc/guides/prog_guide/port_hotplug_framework.html#hotplug > >`__. > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index ea17b97..812d262 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1013,7 +1013,7 @@ netdev_dpdk_destruct(struct netdev *netdev) > if (rte_eth_dev_detach(dev->port_id, devname) < 0) { > VLOG_ERR("Device '%s' can not be detached", dev->devargs); > } else { > - VLOG_INFO("Device '%s' detached", devname); > + VLOG_INFO("Device '%s' has been detached", devname); > } > } > > @@ -2449,6 +2449,53 @@ netdev_dpdk_set_admin_state(struct unixctl_conn > *conn, int argc, > unixctl_command_reply(conn, "OK"); > } > > +static void > +netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[], void *aux OVS_UNUSED) > +{ > + int ret; > + char *response; > + uint8_t port_id; > + char devname[RTE_ETH_NAME_MAX_LEN]; > + struct netdev_dpdk *dev; > + > + ovs_mutex_lock(&dpdk_mutex); > + > + if (!rte_eth_dev_count() || rte_eth_dev_get_port_by_name(argv[1], > + &port_id)) { > + response = xasprintf("Device '%s' not found in DPDK", argv[1]); > + goto error; > + } > + > + dev = netdev_dpdk_lookup_by_port_id(port_id); > + if (dev) { > + response = xasprintf("Device '%s' is being used by interface > '%s'. " > + "Remove it before detaching", > + argv[1], netdev_get_name(&dev->up)); > + goto error; > + } > + > + rte_eth_dev_close(port_id); > + > + ret = rte_eth_dev_detach(port_id, devname); > + if (ret < 0) { > + response = xasprintf("Device '%s' can not be detached", argv[1]); > + goto error; > + } > + > + response = xasprintf("Device '%s' has been detached", argv[1]); > + > + ovs_mutex_unlock(&dpdk_mutex); > + unixctl_command_reply(conn, response); > + free(response); > + return; > + > +error: > + ovs_mutex_unlock(&dpdk_mutex); > + unixctl_command_reply_error(conn, response); > + free(response); > +} > + > /* > * Set virtqueue flags so that we do not receive interrupts. > */ > @@ -2724,6 +2771,9 @@ netdev_dpdk_class_init(void) > unixctl_command_register("netdev-dpdk/set-admin-state", > "[netdev] up|down", 1, 2, > netdev_dpdk_set_admin_state, NULL); > + unixctl_command_register("netdev-dpdk/detach", > + "pci address of device", 1, 1, > + netdev_dpdk_detach, NULL); > > ovsthread_once_done(&once); > } > -- > 1.9.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
