[ovs-dev] [PATCH v4 2/3] netdev-dpdk: Fix device leak on port deletion.
O Mahony, Billy
billy.o.mahony at intel.com
Fri May 26 13:38:07 UTC 2017
Hi Ilya,
This patch does not apply to head of master, currently "c899576 build-windows: cccl fail compilation on Wimplicit-function-declaration".
I'll don't have any comments on the code right now but if you can tell me the commit it's based on I'll check it out.
Thanks,
Billy
> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
> bounces at openvswitch.org] On Behalf Of Ilya Maximets
> Sent: Friday, May 19, 2017 2:38 PM
> To: dev at openvswitch.org; Daniele Di Proietto <diproiettod at ovn.org>;
> Darrell Ball <dball at vmware.com>
> Cc: Ilya Maximets <i.maximets at samsung.com>; Heetae Ahn
> <heetae82.ahn at samsung.com>
> Subject: [ovs-dev] [PATCH v4 2/3] netdev-dpdk: Fix device leak on port
> deletion.
>
> Currently, once created device in dpdk will exist forever even after del-port
> operation untill we manually call 'ovs-appctl netdev-dpdk/detach <name>',
> where <name> is not the port's name but the name of dpdk eth device or pci
> address.
>
> Few issues with current implementation:
>
> 1. Different API for usual (system) and DPDK devices.
> (We have to call 'ovs-appctl netdev-dpdk/detach' each
> time after 'del-port' to actually free the device)
> This is a big issue mostly for virtual DPDK devices.
>
> 2. Follows from 1:
> For DPDK devices 'del-port' leads just to
> 'rte_eth_dev_stop' and subsequent 'add-port' will
> just start the already existing device. Such behaviour
> will not reset the device to initial state as it could
> be expected. For example: virtual pcap pmd will continue
> reading input file instead of reading it from the beginning.
>
> 3. Follows from 2:
> After execution of the following commands 'port1' will be
> configured with the 'old-options' while 'ovs-vsctl show'
> will show us 'new-options' in dpdk-devargs field:
>
> ovs-vsctl add-port port1 -- set interface port1 type=dpdk \
> options:dpdk-devargs=<eth_pmd_name1>,<old-options>
> ovs-vsctl del-port port1
> ovs-vsctl add-port port1 -- set interface port1 type=dpdk \
> options:dpdk-devargs=<eth_pmd_name1>,<new-options>
>
> 4. Follows from 1:
> Not detached device consumes 'port_id'. Since we have very
> limited number of 'port_id's (32 in common case) this may
> lead to quick exhausting of id pool and inability to add any
> other port.
>
> To avoid above issues we need to detach all the attached devices on port
> destruction.
> appctl 'netdev-dpdk/detach' removed because not needed anymore.
>
> We need to use internal 'attached' variable to track ports on which
> rte_eth_dev_attach() was called and returned successfully to avoid closing
> and detaching devices that do not support hotplug or by any other reason
> attached using the 'dpdk-extra' cmdline options.
>
> CC: Ciara Loftus <ciara.loftus at intel.com>
> Fixes: 55e075e65ef9 ("netdev-dpdk: Arbitrary 'dpdk' port naming")
> Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs
> (vdevs)")
> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
> ---
> Documentation/howto/dpdk.rst | 5 ++-
> lib/netdev-dpdk.c | 72 ++++++++++++--------------------------------
> 2 files changed, 22 insertions(+), 55 deletions(-)
>
> diff --git a/Documentation/howto/dpdk.rst
> b/Documentation/howto/dpdk.rst index 3bd9e07..7c06239 100644
> --- a/Documentation/howto/dpdk.rst
> +++ b/Documentation/howto/dpdk.rst
> @@ -342,10 +342,9 @@ Then it can be attached to OVS::
> $ ovs-vsctl add-port br0 dpdkx -- set Interface dpdkx type=dpdk \
> options:dpdk-devargs=0000:01:00.0
>
> -It is also possible to detach a port from ovs, the user has to remove the -port
> using the del-port command, then it can be detached using::
> +Detaching will be performed while processing del-port command::
>
> - $ ovs-appctl netdev-dpdk/detach 0000:01:00.0
> + $ ovs-vsctl del-port dpdkx
>
> 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 diff
> --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1586e41..a41679f 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -360,6 +360,9 @@ struct netdev_dpdk {
> /* Device arguments for dpdk ports */
> char *devargs;
>
> + /* If true, device was attached by rte_eth_dev_attach(). */
> + bool attached;
> +
> /* In dpdk_list. */
> struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>
> @@ -853,6 +856,7 @@ common_construct(struct netdev *netdev, unsigned
> int port_no,
> dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
> ovsrcu_index_init(&dev->vid, -1);
> dev->vhost_reconfigured = false;
> + dev->attached = false;
>
> ovsrcu_init(&dev->qos_conf, NULL);
>
> @@ -998,10 +1002,21 @@ static void
> netdev_dpdk_destruct(struct netdev *netdev) {
> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> + char devname[RTE_ETH_NAME_MAX_LEN];
>
> ovs_mutex_lock(&dpdk_mutex);
>
> rte_eth_dev_stop(dev->port_id);
> +
> + if (dev->attached) {
> + rte_eth_dev_close(dev->port_id);
> + 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);
> + }
> + }
> +
> free(dev->devargs);
> common_destruct(dev);
>
> @@ -1113,7 +1128,8 @@ netdev_dpdk_lookup_by_port_id(int port_id) }
>
> static int
> -netdev_dpdk_process_devargs(const char *devargs, char **errp)
> +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> + const char *devargs, char **errp)
> {
> /* Get the name up to the first comma. */
> char *name = xmemdup0(devargs, strcspn(devargs, ",")); @@ -1125,6
> +1141,7 @@ netdev_dpdk_process_devargs(const char *devargs, char
> **errp)
> /* Device not found in DPDK, attempt to attach it */
> if (!rte_eth_dev_attach(devargs, &new_port_id)) {
> /* Attach successful */
> + dev->attached = true;
> VLOG_INFO("Device '%s' attached to DPDK", devargs);
> } else {
> /* Attach unsuccessful */
> @@ -1211,7 +1228,8 @@ netdev_dpdk_set_config(struct netdev *netdev,
> const struct smap *args,
> * is valid */
> if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
> && rte_eth_dev_is_valid_port(dev->port_id))) {
> - int new_port_id = netdev_dpdk_process_devargs(new_devargs,
> errp);
> + int new_port_id = netdev_dpdk_process_devargs(dev,
> new_devargs,
> + errp);
> if (!rte_eth_dev_is_valid_port(new_port_id)) {
> err = EINVAL;
> } else if (new_port_id == dev->port_id) { @@ -2438,53 +2456,6 @@
> 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.
> */
> @@ -2760,9 +2731,6 @@ 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);
> }
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list