Hi Kevin,
Thanks for this RFC. Please find comments inline.
> -----Original Message-----
> From: Kevin Traynor [mailto:ktray...@redhat.com]
> Sent: Thursday, November 08, 2018 8:37 PM
> To: d...@openvswitch.org; Ophir Munk <ophi...@mellanox.com>;
> ian.sto...@intel.com; i.maxim...@samsung.com
> Cc: Kevin Traynor <ktray...@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 <ktray...@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/
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
