| Message ID | 1495201052-2941-3-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Accepted |
| Headers | show |
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@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Ilya Maximets > Sent: Friday, May 19, 2017 2:38 PM > To: dev@openvswitch.org; Daniele Di Proietto <diproiettod@ovn.org>; > Darrell Ball <dball@vmware.com> > Cc: Ilya Maximets <i.maximets@samsung.com>; Heetae Ahn > <heetae82.ahn@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@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 26.05.2017 16:38, O Mahony, Billy wrote: > Hi Ilya, > > This patch does not apply to head of master, currently "c899576 build-windows: cccl fail compilation on Wimplicit-function-declaration". Hmm. I'm able to apply it using 'git am'. Have you applied first patch of this series? > 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. Originally, these patches are made on top of 126fb3e8abc2 ("ovn-ctl: Start ovn-northd even if ovsdb-servers are not running") but they are still applicable on top of current master. Bets regards, Ilya Maximets. > > Thanks, > Billy > >> -----Original Message----- >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- >> bounces@openvswitch.org] On Behalf Of Ilya Maximets >> Sent: Friday, May 19, 2017 2:38 PM >> To: dev@openvswitch.org; Daniele Di Proietto <diproiettod@ovn.org>; >> Darrell Ball <dball@vmware.com> >> Cc: Ilya Maximets <i.maximets@samsung.com>; Heetae Ahn >> <heetae82.ahn@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@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@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@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
Oops the patch series is probably the reason. > -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@samsung.com] > Sent: Friday, May 26, 2017 2:51 PM > To: O Mahony, Billy <billy.o.mahony@intel.com>; dev@openvswitch.org; > Daniele Di Proietto <diproiettod@ovn.org>; Darrell Ball > <dball@vmware.com> > Cc: Heetae Ahn <heetae82.ahn@samsung.com> > Subject: Re: [ovs-dev] [PATCH v4 2/3] netdev-dpdk: Fix device leak on port > deletion. > > On 26.05.2017 16:38, O Mahony, Billy wrote: > > Hi Ilya, > > > > This patch does not apply to head of master, currently "c899576 build- > windows: cccl fail compilation on Wimplicit-function-declaration". > > Hmm. I'm able to apply it using 'git am'. Have you applied first patch of this > series? > > > 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. > > Originally, these patches are made on top of > 126fb3e8abc2 ("ovn-ctl: Start ovn-northd even if ovsdb-servers are not > running") but they are still applicable on top of current master. > > Bets regards, Ilya Maximets. > > > > > Thanks, > > Billy > > > >> -----Original Message----- > >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > >> bounces@openvswitch.org] On Behalf Of Ilya Maximets > >> Sent: Friday, May 19, 2017 2:38 PM > >> To: dev@openvswitch.org; Daniele Di Proietto <diproiettod@ovn.org>; > >> Darrell Ball <dball@vmware.com> > >> Cc: Ilya Maximets <i.maximets@samsung.com>; Heetae Ahn > >> <heetae82.ahn@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@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@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@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > >
Hi All, This patch looks good to me. I've also tested this with deleting and re-adding dpdk ports of various types (physical , vhostuser, net_pcap) and no device id exhaustion occurs (I checked to beyond the limit of RTE_MAX_ETHPORTS (32) ). Acked-by: Billy O'Mahony <billy.o.mahony@intel.com> > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Ilya Maximets > Sent: Friday, May 19, 2017 2:38 PM > To: dev@openvswitch.org; Daniele Di Proietto <diproiettod@ovn.org>; > Darrell Ball <dball@vmware.com> > Cc: Ilya Maximets <i.maximets@samsung.com>; Heetae Ahn > <heetae82.ahn@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@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, May 19, 2017 at 04:37:31PM +0300, Ilya Maximets wrote: > 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. Thanks, I applied this patch to master.
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); }
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@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@samsung.com> --- Documentation/howto/dpdk.rst | 5 ++- lib/netdev-dpdk.c | 72 ++++++++++++-------------------------------- 2 files changed, 22 insertions(+), 55 deletions(-)