| Message ID | 20181108183635.16925-3-ktraynor@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | Update to DPDK 18.11-rc2 | expand |
On 08.11.2018 21:36, Kevin Traynor wrote: > rte_eth_dev_attach/detach have been removed from > DPDK 18.11. Replace them with rte_dev_probe/remove. > > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > --- > lib/netdev-dpdk.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 10c4879a1..190d50007 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1351,5 +1351,5 @@ netdev_dpdk_destruct(struct netdev *netdev) > { > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > - char devname[RTE_ETH_NAME_MAX_LEN]; > + struct rte_eth_dev_info dev_info; > > ovs_mutex_lock(&dpdk_mutex); > @@ -1360,8 +1360,9 @@ netdev_dpdk_destruct(struct netdev *netdev) > if (dev->attached) { > rte_eth_dev_close(dev->port_id); > - if (rte_eth_dev_detach(dev->port_id, devname) < 0) { > + rte_eth_dev_info_get(dev->port_id, &dev_info); > + if (dev_info.device && !rte_dev_remove(dev_info.device)) { > + VLOG_INFO("Device '%s' has been detached", dev->devargs); > + } else { > VLOG_ERR("Device '%s' can not be detached", dev->devargs); > - } else { > - VLOG_INFO("Device '%s' has been detached", devname); > } > } > @@ -1645,5 +1646,5 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev, > char *name; > dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID; > - > + struct rte_dev_iterator iterator; > if (strncmp(devargs, "class=eth,mac=", 14) == 0) { > new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]); > @@ -1653,8 +1654,12 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev, > || !rte_eth_dev_is_valid_port(new_port_id)) { > /* Device not found in DPDK, attempt to attach it */ > - if (!rte_eth_dev_attach(devargs, &new_port_id)) { > + if (!rte_dev_probe(devargs)) { > /* Attach successful */ > dev->attached = true; > VLOG_INFO("Device '%s' attached to DPDK", devargs); > + RTE_ETH_FOREACH_MATCHING_DEV(new_port_id, devargs, &iterator) { > + rte_eth_iterator_cleanup(&iterator); > + break; > + } This is a recommended way to find the device after probe, but it looks very unclear. Can we just call 'rte_eth_dev_get_port_by_name(name, &new_port_id)' here instead ? It should have same effect. If not, it'll be nice to have some comments. > } else { > /* Attach unsuccessful */ > @@ -3206,6 +3211,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, > char *response; > dpdk_port_t port_id; > - char devname[RTE_ETH_NAME_MAX_LEN]; > struct netdev_dpdk *dev; > + struct rte_eth_dev_info dev_info; > > ovs_mutex_lock(&dpdk_mutex); > @@ -3226,9 +3231,9 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, > 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; > - } > + rte_eth_dev_info_get(port_id, &dev_info); > + if (!dev_info.device || rte_dev_remove(dev_info.device)) { > + response = xasprintf("Device '%s' can not be detached", argv[1]); > + goto error; > + } Indents are a bit off. > > response = xasprintf("Device '%s' has been detached", argv[1]); >
On 11/09/2018 11:57 AM, Ilya Maximets wrote: > On 08.11.2018 21:36, Kevin Traynor wrote: >> rte_eth_dev_attach/detach have been removed from >> DPDK 18.11. Replace them with rte_dev_probe/remove. >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >> --- >> lib/netdev-dpdk.c | 29 +++++++++++++++++------------ >> 1 file changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 10c4879a1..190d50007 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1351,5 +1351,5 @@ netdev_dpdk_destruct(struct netdev *netdev) >> { >> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> - char devname[RTE_ETH_NAME_MAX_LEN]; >> + struct rte_eth_dev_info dev_info; >> >> ovs_mutex_lock(&dpdk_mutex); >> @@ -1360,8 +1360,9 @@ netdev_dpdk_destruct(struct netdev *netdev) >> if (dev->attached) { >> rte_eth_dev_close(dev->port_id); >> - if (rte_eth_dev_detach(dev->port_id, devname) < 0) { >> + rte_eth_dev_info_get(dev->port_id, &dev_info); >> + if (dev_info.device && !rte_dev_remove(dev_info.device)) { >> + VLOG_INFO("Device '%s' has been detached", dev->devargs); >> + } else { >> VLOG_ERR("Device '%s' can not be detached", dev->devargs); >> - } else { >> - VLOG_INFO("Device '%s' has been detached", devname); >> } >> } >> @@ -1645,5 +1646,5 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev, >> char *name; >> dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID; >> - >> + struct rte_dev_iterator iterator; >> if (strncmp(devargs, "class=eth,mac=", 14) == 0) { >> new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]); >> @@ -1653,8 +1654,12 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev, >> || !rte_eth_dev_is_valid_port(new_port_id)) { >> /* Device not found in DPDK, attempt to attach it */ >> - if (!rte_eth_dev_attach(devargs, &new_port_id)) { >> + if (!rte_dev_probe(devargs)) { >> /* Attach successful */ >> dev->attached = true; >> VLOG_INFO("Device '%s' attached to DPDK", devargs); >> + RTE_ETH_FOREACH_MATCHING_DEV(new_port_id, devargs, &iterator) { >> + rte_eth_iterator_cleanup(&iterator); >> + break; >> + } > > This is a recommended way to find the device after probe, but it looks very > unclear. Can we just call 'rte_eth_dev_get_port_by_name(name, &new_port_id)' > here instead ? It should have same effect. Yeah, looks like we can use it in this case. Changed to that in v2 and checked the return before setting attached. > If not, it'll be nice to have some comments. > >> } else { >> /* Attach unsuccessful */ >> @@ -3206,6 +3211,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, >> char *response; >> dpdk_port_t port_id; >> - char devname[RTE_ETH_NAME_MAX_LEN]; >> struct netdev_dpdk *dev; >> + struct rte_eth_dev_info dev_info; >> >> ovs_mutex_lock(&dpdk_mutex); >> @@ -3226,9 +3231,9 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, >> 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; >> - } >> + rte_eth_dev_info_get(port_id, &dev_info); >> + if (!dev_info.device || rte_dev_remove(dev_info.device)) { >> + response = xasprintf("Device '%s' can not be detached", argv[1]); >> + goto error; >> + } > > Indents are a bit off. > fixed, thanks. >> >> response = xasprintf("Device '%s' has been detached", argv[1]); >>
Hi Kevin, Thanks for this RFC. Please find comments inline. > -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Thursday, November 08, 2018 8:37 PM > To: dev@openvswitch.org; Ophir Munk <ophirmu@mellanox.com>; > ian.stokes@intel.com; i.maximets@samsung.com > Cc: Kevin Traynor <ktraynor@redhat.com> > Subject: [RFC dpdk-latest 2/2] netdev-dpdk: Replace > rte_eth_dev_attach/detach. > > rte_eth_dev_attach/detach have been removed from DPDK 18.11. Replace > them with rte_dev_probe/remove. > I have submitted a patch on the same issue, see [1]. Please suggest how to unify our patches. > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > --- > lib/netdev-dpdk.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > 10c4879a1..190d50007 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1351,5 +1351,5 @@ netdev_dpdk_destruct(struct netdev *netdev) { > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > - char devname[RTE_ETH_NAME_MAX_LEN]; > + struct rte_eth_dev_info dev_info; > > ovs_mutex_lock(&dpdk_mutex); > @@ -1360,8 +1360,9 @@ netdev_dpdk_destruct(struct netdev *netdev) > if (dev->attached) { > rte_eth_dev_close(dev->port_id); > - if (rte_eth_dev_detach(dev->port_id, devname) < 0) { > + rte_eth_dev_info_get(dev->port_id, &dev_info); I suggest having a direct access to the device (rather than accessing dev_info). rte_eth_devices[dev->port_id].device; > + if (dev_info.device && !rte_dev_remove(dev_info.device)) { With dpdk 18.11 an rte device can be used by multiple eth devices (one to many relationship). Remark: in OVS terms an eth device is a dpdk port_id. When calling rte_dev_remove() all the eth devices are closed and the PCI device is detached. We need to count the number of eth devices used by the rte device. Only when closing the last eth device shoud we call rte_dev_remove (it is like a reference count algorithm). For example: if we add to OVS 2 ports belonging the same rte device (e.g. the same PCI bus address) then after deleting one of the ports we should not implicitly delete the other port. This case is handled in [1] > + VLOG_INFO("Device '%s' has been detached", dev->devargs); > + } else { > VLOG_ERR("Device '%s' can not be detached", dev->devargs); > - } else { > - VLOG_INFO("Device '%s' has been detached", devname); > } > } > @@ -1645,5 +1646,5 @@ netdev_dpdk_process_devargs(struct > netdev_dpdk *dev, > char *name; > dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID; > - > + struct rte_dev_iterator iterator; > if (strncmp(devargs, "class=eth,mac=", 14) == 0) { > new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]); > @@ -1653,8 +1654,12 @@ netdev_dpdk_process_devargs(struct > netdev_dpdk *dev, > || !rte_eth_dev_is_valid_port(new_port_id)) { > /* Device not found in DPDK, attempt to attach it */ > - if (!rte_eth_dev_attach(devargs, &new_port_id)) { > + if (!rte_dev_probe(devargs)) { > /* Attach successful */ > dev->attached = true; > VLOG_INFO("Device '%s' attached to DPDK", devargs); > + RTE_ETH_FOREACH_MATCHING_DEV(new_port_id, devargs, > &iterator) { > + rte_eth_iterator_cleanup(&iterator); +1 Good point which I missed! Indeed we should free resources by calling the rte_eth_iterator_cleanup(). > + break; > + } > } else { It seems with this patch the code still has the call: rte_eth_dev_get_port_by_name(). This call is no more relevant when using representors. Previously a PCI address (e.g. 0000:08.00:0) was used in devargs. With dpdk 18.11 a device devargs became a regular expression string, (e.g. 0000:08.00:0,representor=[1]) which is different than the device name assigned by the PMD. In other words: previously devargs and device name where identical. Starting from dpdk 18.11 they are not. This case is handled in [1]. > /* Attach unsuccessful */ @@ -3206,6 +3211,6 @@ > netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, > char *response; > dpdk_port_t port_id; > - char devname[RTE_ETH_NAME_MAX_LEN]; > struct netdev_dpdk *dev; > + struct rte_eth_dev_info dev_info; > > ovs_mutex_lock(&dpdk_mutex); > @@ -3226,9 +3231,9 @@ netdev_dpdk_detach(struct unixctl_conn *conn, > int argc OVS_UNUSED, > 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; > - } > + rte_eth_dev_info_get(port_id, &dev_info); > + if (!dev_info.device || rte_dev_remove(dev_info.device)) { > + response = xasprintf("Device '%s' can not be detached", argv[1]); > + goto error; > + } > Before calling ret_dev_remove() we should check if there are no more ports which use the same device. If there are - we should fail the detach command with an appropriate error message to the user. Please see [1]. > response = xasprintf("Device '%s' has been detached", argv[1]); > -- > 2.19.0 [1] https://patchwork.ozlabs.org/patch/997879/
On 11/15/2018 04:38 PM, Ophir Munk wrote: > Hi Kevin, > Thanks for this RFC. Please find comments inline. > >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Thursday, November 08, 2018 8:37 PM >> To: dev@openvswitch.org; Ophir Munk <ophirmu@mellanox.com>; >> ian.stokes@intel.com; i.maximets@samsung.com >> Cc: Kevin Traynor <ktraynor@redhat.com> >> Subject: [RFC dpdk-latest 2/2] netdev-dpdk: Replace >> rte_eth_dev_attach/detach. >> >> rte_eth_dev_attach/detach have been removed from DPDK 18.11. Replace >> them with rte_dev_probe/remove. >> > > I have submitted a patch on the same issue, see [1]. > Please suggest how to unify our patches. > Hi Ophir, I looked through your patch and it is trying to do two things: 1. update OVS to use DPDK 18.11 2. enable additional functionality/representors etc in OVS based on using DPDK 18.11 I don't think that needs to be all in one patch and there isn't really any throw away work in doing 1. on it's own. My suggestion is that we proceed with 1. through the patches I sent, and then (or in parallel) you can send patches to cover 2. What do people think? thanks, Kevin. >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >> --- >> lib/netdev-dpdk.c | 29 +++++++++++++++++------------ >> 1 file changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >> 10c4879a1..190d50007 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1351,5 +1351,5 @@ netdev_dpdk_destruct(struct netdev *netdev) { >> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> - char devname[RTE_ETH_NAME_MAX_LEN]; >> + struct rte_eth_dev_info dev_info; >> >> ovs_mutex_lock(&dpdk_mutex); >> @@ -1360,8 +1360,9 @@ netdev_dpdk_destruct(struct netdev *netdev) >> if (dev->attached) { >> rte_eth_dev_close(dev->port_id); >> - if (rte_eth_dev_detach(dev->port_id, devname) < 0) { >> + rte_eth_dev_info_get(dev->port_id, &dev_info); > > I suggest having a direct access to the device (rather than accessing dev_info). > rte_eth_devices[dev->port_id].device; > >> + if (dev_info.device && !rte_dev_remove(dev_info.device)) { > > With dpdk 18.11 an rte device can be used by multiple eth devices (one to many relationship). > Remark: in OVS terms an eth device is a dpdk port_id. > When calling rte_dev_remove() all the eth devices are closed and the PCI device is detached. > We need to count the number of eth devices used by the rte device. Only when closing the last eth device > shoud we call rte_dev_remove (it is like a reference count algorithm). > For example: if we add to OVS 2 ports belonging the same rte device (e.g. the same PCI bus address) then after deleting one of the ports we should not implicitly delete the other port. > This case is handled in [1] > >> + VLOG_INFO("Device '%s' has been detached", dev->devargs); >> + } else { >> VLOG_ERR("Device '%s' can not be detached", dev->devargs); >> - } else { >> - VLOG_INFO("Device '%s' has been detached", devname); >> } >> } >> @@ -1645,5 +1646,5 @@ netdev_dpdk_process_devargs(struct >> netdev_dpdk *dev, >> char *name; >> dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID; >> - >> + struct rte_dev_iterator iterator; >> if (strncmp(devargs, "class=eth,mac=", 14) == 0) { >> new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]); >> @@ -1653,8 +1654,12 @@ netdev_dpdk_process_devargs(struct >> netdev_dpdk *dev, >> || !rte_eth_dev_is_valid_port(new_port_id)) { >> /* Device not found in DPDK, attempt to attach it */ >> - if (!rte_eth_dev_attach(devargs, &new_port_id)) { >> + if (!rte_dev_probe(devargs)) { >> /* Attach successful */ >> dev->attached = true; >> VLOG_INFO("Device '%s' attached to DPDK", devargs); >> + RTE_ETH_FOREACH_MATCHING_DEV(new_port_id, devargs, >> &iterator) { >> + rte_eth_iterator_cleanup(&iterator); > > +1 > Good point which I missed! > Indeed we should free resources by calling the rte_eth_iterator_cleanup(). > >> + break; >> + } >> } else { > > It seems with this patch the code still has the call: rte_eth_dev_get_port_by_name(). > This call is no more relevant when using representors. Previously a PCI address (e.g. 0000:08.00:0) was used in devargs. > With dpdk 18.11 a device devargs became a regular expression string, (e.g. 0000:08.00:0,representor=[1]) which is different than the device name assigned by the PMD. > In other words: previously devargs and device name where identical. Starting from dpdk 18.11 they are not. > This case is handled in [1]. > They are all valid comments, but as per above, I was only trying to cover 1. in these patches. >> /* Attach unsuccessful */ @@ -3206,6 +3211,6 @@ >> netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, >> char *response; >> dpdk_port_t port_id; >> - char devname[RTE_ETH_NAME_MAX_LEN]; >> struct netdev_dpdk *dev; >> + struct rte_eth_dev_info dev_info; >> >> ovs_mutex_lock(&dpdk_mutex); >> @@ -3226,9 +3231,9 @@ netdev_dpdk_detach(struct unixctl_conn *conn, >> int argc OVS_UNUSED, >> 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; >> - } >> + rte_eth_dev_info_get(port_id, &dev_info); >> + if (!dev_info.device || rte_dev_remove(dev_info.device)) { >> + response = xasprintf("Device '%s' can not be detached", argv[1]); >> + goto error; >> + } >> > > Before calling ret_dev_remove() we should check if there are no more ports which use the same device. > If there are - we should fail the detach command with an appropriate error message to the user. > Please see [1]. > >> response = xasprintf("Device '%s' has been detached", argv[1]); >> -- >> 2.19.0 > > [1] > https://patchwork.ozlabs.org/patch/997879/ >
> On 11/15/2018 04:38 PM, Ophir Munk wrote: > > Hi Kevin, > > Thanks for this RFC. Please find comments inline. > > > >> -----Original Message----- > >> From: Kevin Traynor [mailto:ktraynor@redhat.com] > >> Sent: Thursday, November 08, 2018 8:37 PM > >> To: dev@openvswitch.org; Ophir Munk <ophirmu@mellanox.com>; > >> ian.stokes@intel.com; i.maximets@samsung.com > >> Cc: Kevin Traynor <ktraynor@redhat.com> > >> Subject: [RFC dpdk-latest 2/2] netdev-dpdk: Replace > >> rte_eth_dev_attach/detach. > >> > >> rte_eth_dev_attach/detach have been removed from DPDK 18.11. Replace > >> them with rte_dev_probe/remove. > >> > > > > I have submitted a patch on the same issue, see [1]. > > Please suggest how to unify our patches. > > > > Hi Ophir, > > I looked through your patch and it is trying to do two things: > 1. update OVS to use DPDK 18.11 > 2. enable additional functionality/representors etc in OVS based on using > DPDK 18.11 > > I don't think that needs to be all in one patch and there isn't really any > throw away work in doing 1. on it's own. My suggestion is that we proceed > with 1. through the patches I sent, and then (or in parallel) you can send > patches to cover 2. > > What do people think? +1, the RFC submitted by Kevin to move to 18.11 isn't too complicated and allows for the same functionality we had when using DPDK 17.11. My preference is to upstream this work first. This provides a stable base to work from when adding the new functionality enabled by 18.11. Upstreaming this work first will enable the travis build for dpdk-latest and dpdk-hwol to pass again which is also welcome. Ian > > thanks, > Kevin. > > >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > >> --- > >> lib/netdev-dpdk.c | 29 +++++++++++++++++------------ > >> 1 file changed, 17 insertions(+), 12 deletions(-) > >> > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > >> 10c4879a1..190d50007 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -1351,5 +1351,5 @@ netdev_dpdk_destruct(struct netdev *netdev) { > >> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > >> - char devname[RTE_ETH_NAME_MAX_LEN]; > >> + struct rte_eth_dev_info dev_info; > >> > >> ovs_mutex_lock(&dpdk_mutex); > >> @@ -1360,8 +1360,9 @@ netdev_dpdk_destruct(struct netdev *netdev) > >> if (dev->attached) { > >> rte_eth_dev_close(dev->port_id); > >> - if (rte_eth_dev_detach(dev->port_id, devname) < 0) { > >> + rte_eth_dev_info_get(dev->port_id, &dev_info); > > > > I suggest having a direct access to the device (rather than accessing > dev_info). > > rte_eth_devices[dev->port_id].device; > > > >> + if (dev_info.device && !rte_dev_remove(dev_info.device)) { > > > > With dpdk 18.11 an rte device can be used by multiple eth devices (one > to many relationship). > > Remark: in OVS terms an eth device is a dpdk port_id. > > When calling rte_dev_remove() all the eth devices are closed and the PCI > device is detached. > > We need to count the number of eth devices used by the rte device. > > Only when closing the last eth device shoud we call rte_dev_remove (it > is like a reference count algorithm). > > For example: if we add to OVS 2 ports belonging the same rte device > (e.g. the same PCI bus address) then after deleting one of the ports we > should not implicitly delete the other port. > > This case is handled in [1] > > > >> + VLOG_INFO("Device '%s' has been detached", dev->devargs); > >> + } else { > >> VLOG_ERR("Device '%s' can not be detached", dev->devargs); > >> - } else { > >> - VLOG_INFO("Device '%s' has been detached", devname); > >> } > >> } > >> @@ -1645,5 +1646,5 @@ netdev_dpdk_process_devargs(struct > >> netdev_dpdk *dev, > >> char *name; > >> dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID; > >> - > >> + struct rte_dev_iterator iterator; > >> if (strncmp(devargs, "class=eth,mac=", 14) == 0) { > >> new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]); > >> @@ -1653,8 +1654,12 @@ netdev_dpdk_process_devargs(struct > >> netdev_dpdk *dev, > >> || !rte_eth_dev_is_valid_port(new_port_id)) { > >> /* Device not found in DPDK, attempt to attach it */ > >> - if (!rte_eth_dev_attach(devargs, &new_port_id)) { > >> + if (!rte_dev_probe(devargs)) { > >> /* Attach successful */ > >> dev->attached = true; > >> VLOG_INFO("Device '%s' attached to DPDK", devargs); > >> + RTE_ETH_FOREACH_MATCHING_DEV(new_port_id, devargs, > >> &iterator) { > >> + rte_eth_iterator_cleanup(&iterator); > > > > +1 > > Good point which I missed! > > Indeed we should free resources by calling the > rte_eth_iterator_cleanup(). > > > >> + break; > >> + } > >> } else { > > > > It seems with this patch the code still has the call: > rte_eth_dev_get_port_by_name(). > > This call is no more relevant when using representors. Previously a PCI > address (e.g. 0000:08.00:0) was used in devargs. > > With dpdk 18.11 a device devargs became a regular expression string, > (e.g. 0000:08.00:0,representor=[1]) which is different than the device > name assigned by the PMD. > > In other words: previously devargs and device name where identical. > Starting from dpdk 18.11 they are not. > > This case is handled in [1]. > > > > They are all valid comments, but as per above, I was only trying to cover > 1. in these patches. > > >> /* Attach unsuccessful */ @@ -3206,6 +3211,6 @@ > >> netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, > >> char *response; > >> dpdk_port_t port_id; > >> - char devname[RTE_ETH_NAME_MAX_LEN]; > >> struct netdev_dpdk *dev; > >> + struct rte_eth_dev_info dev_info; > >> > >> ovs_mutex_lock(&dpdk_mutex); > >> @@ -3226,9 +3231,9 @@ netdev_dpdk_detach(struct unixctl_conn *conn, > >> int argc OVS_UNUSED, > >> 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; > >> - } > >> + rte_eth_dev_info_get(port_id, &dev_info); > >> + if (!dev_info.device || rte_dev_remove(dev_info.device)) { > >> + response = xasprintf("Device '%s' can not be detached", > argv[1]); > >> + goto error; > >> + } > >> > > > > Before calling ret_dev_remove() we should check if there are no more > ports which use the same device. > > If there are - we should fail the detach command with an appropriate > error message to the user. > > Please see [1]. > > > >> response = xasprintf("Device '%s' has been detached", argv[1]); > >> -- > >> 2.19.0 > > > > [1] > > https://patchwork.ozlabs.org/patch/997879/ > >
> -----Original Message----- > From: Stokes, Ian [mailto:ian.stokes@intel.com] > Sent: Thursday, November 15, 2018 7:49 PM > > > > Hi Ophir, > > > > I looked through your patch and it is trying to do two things: > > 1. update OVS to use DPDK 18.11 > > 2. enable additional functionality/representors etc in OVS based on > > using DPDK 18.11 > > > > I don't think that needs to be all in one patch and there isn't really > > any throw away work in doing 1. on it's own. My suggestion is that we > > proceed with 1. through the patches I sent, and then (or in parallel) > > you can send patches to cover 2. > > > > What do people think? > > +1, the RFC submitted by Kevin to move to 18.11 isn't too complicated and > allows for the same functionality we had when using DPDK 17.11. My > preference is to upstream this work first. This provides a stable base to work > from when adding the new functionality enabled by 18.11. > +1, sounds good. > Upstreaming this work first will enable the travis build for dpdk-latest and > dpdk-hwol to pass again which is also welcome. > > Ian > > > > > thanks, > > Kevin. > >
On 15.11.2018 20:26, Kevin Traynor wrote: > On 11/15/2018 04:38 PM, Ophir Munk wrote: >> Hi Kevin, >> Thanks for this RFC. Please find comments inline. >> >>> -----Original Message----- >>> From: Kevin Traynor [mailto:ktraynor@redhat.com] >>> Sent: Thursday, November 08, 2018 8:37 PM >>> To: dev@openvswitch.org; Ophir Munk <ophirmu@mellanox.com>; >>> ian.stokes@intel.com; i.maximets@samsung.com >>> Cc: Kevin Traynor <ktraynor@redhat.com> >>> Subject: [RFC dpdk-latest 2/2] netdev-dpdk: Replace >>> rte_eth_dev_attach/detach. >>> >>> rte_eth_dev_attach/detach have been removed from DPDK 18.11. Replace >>> them with rte_dev_probe/remove. >>> >> >> I have submitted a patch on the same issue, see [1]. >> Please suggest how to unify our patches. >> > > Hi Ophir, > > I looked through your patch and it is trying to do two things: > 1. update OVS to use DPDK 18.11 > 2. enable additional functionality/representors etc in OVS based on > using DPDK 18.11 > > I don't think that needs to be all in one patch and there isn't really > any throw away work in doing 1. on it's own. My suggestion is that we > proceed with 1. through the patches I sent, and then (or in parallel) > you can send patches to cover 2. > > What do people think? +1. Sounds good. > thanks, > Kevin. > >>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >>> --- >>> lilib/librte_ethdev/rte_ethdev_core.hb/netdev-dpdk.c | 29 +++++++++++++++++------------ >>> 1 file changed, 17 insertions(+), 12 deletions(-) >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >>> 10c4879a1..190d50007 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -1351,5 +1351,5 @@ netdev_dpdk_destruct(struct netdev *netdev) { >>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >>> - char devname[RTE_ETH_NAME_MAX_LEN]; >>> + struct rte_eth_dev_info dev_info; >>> >>> ovs_mutex_lock(&dpdk_mutex); >>> @@ -1360,8 +1360,9 @@ netdev_dpdk_destruct(struct netdev *netdev) >>> if (dev->attached) { >>> rte_eth_dev_close(dev->port_id); >>> - if (rte_eth_dev_detach(dev->port_id, devname) < 0) { >>> + rte_eth_dev_info_get(dev->port_id, &dev_info); >> >> I suggest having a direct access to the device (rather than accessing dev_info). >> rte_eth_devices[dev->port_id].device; I don't think that we can use 'rte_eth_devices' directly because it marked as @internal in dpdk code. Also it's located in file lib/librte_ethdev/rte_ethdev_core.h, which has following disclaimer: /** * @file * * RTE Ethernet Device internal header. * * This header contains internal data types. But they are still part of the * public API because they are used by inline functions in the published API. * * Applications should not use these directly. * */ I'd like to raise this issue in DPDK community, because test-pmd and some example apps are using this array directly. Best regards, Ilya Maximets.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 10c4879a1..190d50007 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1351,5 +1351,5 @@ netdev_dpdk_destruct(struct netdev *netdev) { struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); - char devname[RTE_ETH_NAME_MAX_LEN]; + struct rte_eth_dev_info dev_info; ovs_mutex_lock(&dpdk_mutex); @@ -1360,8 +1360,9 @@ netdev_dpdk_destruct(struct netdev *netdev) if (dev->attached) { rte_eth_dev_close(dev->port_id); - if (rte_eth_dev_detach(dev->port_id, devname) < 0) { + rte_eth_dev_info_get(dev->port_id, &dev_info); + if (dev_info.device && !rte_dev_remove(dev_info.device)) { + VLOG_INFO("Device '%s' has been detached", dev->devargs); + } else { VLOG_ERR("Device '%s' can not be detached", dev->devargs); - } else { - VLOG_INFO("Device '%s' has been detached", devname); } } @@ -1645,5 +1646,5 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev, char *name; dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID; - + struct rte_dev_iterator iterator; if (strncmp(devargs, "class=eth,mac=", 14) == 0) { new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]); @@ -1653,8 +1654,12 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev, || !rte_eth_dev_is_valid_port(new_port_id)) { /* Device not found in DPDK, attempt to attach it */ - if (!rte_eth_dev_attach(devargs, &new_port_id)) { + if (!rte_dev_probe(devargs)) { /* Attach successful */ dev->attached = true; VLOG_INFO("Device '%s' attached to DPDK", devargs); + RTE_ETH_FOREACH_MATCHING_DEV(new_port_id, devargs, &iterator) { + rte_eth_iterator_cleanup(&iterator); + break; + } } else { /* Attach unsuccessful */ @@ -3206,6 +3211,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, char *response; dpdk_port_t port_id; - char devname[RTE_ETH_NAME_MAX_LEN]; struct netdev_dpdk *dev; + struct rte_eth_dev_info dev_info; ovs_mutex_lock(&dpdk_mutex); @@ -3226,9 +3231,9 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, 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; - } + rte_eth_dev_info_get(port_id, &dev_info); + if (!dev_info.device || rte_dev_remove(dev_info.device)) { + response = xasprintf("Device '%s' can not be detached", argv[1]); + goto error; + } response = xasprintf("Device '%s' has been detached", argv[1]);
rte_eth_dev_attach/detach have been removed from DPDK 18.11. Replace them with rte_dev_probe/remove. Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- lib/netdev-dpdk.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)