[dpdk-users] attach/detach on secondary process

[Ricardo Roldan <rroldan@bequant.com>]

Hi,

We have a multi-process application and we need to support 
attaching/detaching of ports. We are using the 17.11 version with the 
Intel x520 (ixgbe driver) and virtio.

At the time we initialize our processes there are not any devices binded 
with the DPDK drivers, so we initialize all processes (primary and 
secondaries) with 0 ports.

This seems to work fine only on the primary processes, but on the 
secondary processes we see some problems. In the following paragraphs I 
describe the procedure used to attach/detach interfaces with DPDK.

For the attach procedure (all processes initially have no devices 
attached):

- Bind the devices we want to attach to the DPDK driver (with the script 
dpdk-devbind, from external process)

- Primary process: Call rte_eth_dev_attach

- Primary process: Configure ports using ...

- Secondary processes: Call to rte_eth_dev_attach


Start to send/receive packets from all processes.


For the detach procedure:

- Secondary processes: For each port, call rte_eth_dev_stop(port), 
rte_eth_dev_close(port) and rte_eth_dev_detach(port, dev).

- Primary process: After the secondary processes have detach all their 
ports, for each port call rte_eth_dev_stop(port), 
rte_eth_dev_close(port) and rte_eth_dev_detach(port, dev).

- Bind the device to the original Linux driver (with the script 
dpdk-devbind, from external process)


With this approach we have noticed that when the secondary processes 
call rte_dev_detach there is an error, because it calls the remove 
operation, which ends up calling eth_ixgbe_dev_uninit that returns 
-EPERM (because it does not allow a non primary process to uninitialize 
the driver).


Therefore, the port attach never works again on the secondary processes 
as the function rte_eal_hotplug_add fails because it cannot find the 
device.


dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
         if (dev == NULL) {
                 RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
                         devname);
                 ret = -ENODEV;
                 goto err_devarg;
         }


This happens because in order to check unplugged devices, the function 
cmp_detached_dev_name  checks if there is a pointer to the driver and 
fails if it is already set, because the detach procedure never set the 
driver variable to NULL.

static int cmp_detached_dev_name(const struct rte_device *dev,
         const void *_name)
{
         const char *name = _name;

         /* skip attached devices */
         RTE_LOG(ERR, EAL, "cmp_detached_dev_name dev %p name %s driver %p"
         " search %s\n",
         dev, dev->name, dev->driver, name);
         if (dev->driver != NULL)
                 return 1;

         return strcmp(dev->name, name);
}

To fix this behavior we have done the following changes on the DPDK code.

First, in order to prevent cmp_detached_dev_name from failing, 
rte_eal_dev_detach sets driver to NULL.

diff --git a/lib/librte_eal/common/eal_common_dev.c 
b/lib/librte_eal/common/eal_common_dev.c

index dda8f5835..9a363dcf7 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -114,6 +114,7 @@ int rte_eal_dev_detach(struct rte_device *dev)
         if (ret)
                 RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
                         dev->name);
+       dev->driver = NULL;
         return ret;
  }/*
*/


Then, in the rte_eth_dev_pci_generic_remove function, the call to 
dev_uninit does not consider -EPERM an error, because when detaching a 
port, some drivers return 0 and other drivers return -EPERM to indicate 
that it is called from a secondary process.

diff --git a/lib/librte_ether/rte_ethdev_pci.h 
b/lib/librte_ether/rte_ethdev_pci.h
@@ -184,7 +184,7 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device 
*pci_dev,

         if (dev_uninit) {
                 ret = dev_uninit(eth_dev);
-               if (ret)
+               if (ret && ret != -EPERM)
                         return ret;
         }

Finally, in the rte_eth_dev_pci_release function, only the fields in the 
shared memory region are reset if called from a primary process.

/**/diff --git a/lib/librte_ether/rte_ethdev_pci.h 
b/lib/librte_ether/rte_ethdev_pci.h
index 722075e09..a79188fbf 100644
--- a/lib/librte_ether/rte_ethdev_pci.h
+++ b/lib/librte_ether/rte_ethdev_pci.h
@@ -125,16 +125,16 @@ rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev)
         /* free ether device */
         rte_eth_dev_release_port(eth_dev);

-       if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
rte_free(eth_dev->data->dev_private);
+ eth_dev->data->dev_private = NULL;

-       eth_dev->data->dev_private = NULL;
-
-       /*
-        * Secondary process will check the name to attach.
-        * Clear this field to avoid attaching a released ports.
-        */
-       eth_dev->data->name[0] = '\0';
+               /*
+                * Secondary process will check the name to attach.
+                * Clear this field to avoid attaching a released ports.
+                */
+               eth_dev->data->name[0] = '\0';
+        }

         eth_dev->device = NULL;
         eth_dev->intr_handle = NULL;


After applying these changes, it seems like attaching and detaching 
ports multiple times works without problems, at least with the ixgbe and 
virtio drivers.

In order to generate a patch, It would be very helpful if anyone can 
confirm that this approach is correct and that we do not break any other 
parts.

Best regards,
Ricardo


On 12/13/2017 03:31 PM, Ricardo Roldan wrote:
>
>
> Hi,
>
> We have a multi-process application and we need to support 
> attaching/detaching of ports.
>
> At the time we initialize our process they aren't any devices binded 
> with the dpdk drivers, so we initialize in all processes with 0 ports.
>
> We manage to work fine only on the primary processes, on the secondary 
> process we had some problems. Here is the way call the dpdk
> interface to attach/detach from our process
>
>       Attach - At this moment all process (primary and secondaries) 
> has not devices attached
>
>       - Bind the devices we want to attach to the DPDK driver (with 
> the script dpdk-devbind, from external process)
>
>       - Call rte_eth_dev_attach from primary process
>
>       - Configure ports from primary process
>
>       - From secondary process call to rte_eth_dev_attach
>
>
>      Began to send/receive packets from all process
>
>
>
>      Detach
>
>        - From secondary process call to:
>
>               rte_eth_dev_stop(port);
>
>               rte_eth_dev_close(port);
>               rte_eth_dev_detach(port, dev);
>
>
>        - From primary process call to:
>
>               rte_eth_dev_stop(port);
>
>               rte_eth_dev_close(port);
>               rte_eth_dev_detach(port, dev);
>
>
>        - Bind the device to the original Linux driver (with the script 
> dpdk-devbind, from external process)
>
>
>    With this second approach we notice that the detach from the 
> secondary process returns with an error. This is because the function 
> eth_ixgbe_dev_uninit has a check to not uninitialize the driver from a 
> not primary process.
>
>    So that the detach can't finish all its task.
>
>    After this, the attach never works again on the secondary process 
> as the function rte_eal_hotplug_add began to fail due that it cant not 
> find the device.
>
> */        dev = bus->find_device(NULL, cmp_detached_dev_name, 
> devname); /**/
> /**/        if (dev == NULL) { /**/
> /**/                RTE_LOG(ERR, EAL, "Cannot find unplugged device 
> (%s)\n", /**/
> /**/                        devname); /**/
> /**/                ret = -ENODEV; /**/
> /**/                goto err_devarg; /**/
> /**/        } /**/
> /**//*
>    This is due that to check unplugged devices, what the comparation 
> function cmp_detached_dev_name do, is to check if there is a pointer 
> to the driver.
>
> */static int cmp_detached_dev_name(const struct rte_device *dev, /**/
> /**/                   const void *_name) /**/
> /**/{ /**/
> /**/        const char *name = _name; /**/
> /**//**/
> /**/        /* skip attached devices */ /**/
> /**/        if (dev->driver != NULL) /**/
> /**/                return 1; /**/
> /**//**/
> /**/        return strcmp(dev->name, name); /**/
> /**/} /*/
> /
>
> And as detach has not finish correctly on the secondary process, the 
> device continues with the pointer to the driver setted. So the attach 
> fails as the device is not found.
>
>
> To overcome this behavior we had done this changes on the DPDK code:
>
> We have modify the dpdk to clean the pointer to the driver on the 
> detach.We had modify also the function rte_eth_dev_pci_generic_remove 
> so even if the uninit
> of the driver return with -EPERM the function continue executing the 
> rest of the code. We had done this as we had seen that the check on 
> the uninit testing to
> see if the process is not a primary  is donein all drivers, but some 
> drivers return with no error ( 0) and others with (-EPERM). So on 
> rte_eth_dev_pci_generic if the call
> to uninit returns with -EPERM we continue executing calling 
> rte_eth_dev_pci_release. To that last function we had also done some 
> changes, as only primary process
> should be able to uninitialized some common values, that a detach on a 
> secondary process should never do.
>
> These are the changes:
>
> /*diff --git a/lib/librte_eal/common/eal_common_dev.c 
> b/lib/librte_eal/common/eal_common_dev.c*//*
> *//*index dda8f5835..9a363dcf7 100644*//*
> *//*--- a/lib/librte_eal/common/eal_common_dev.c*//*
> *//*+++ b/lib/librte_eal/common/eal_common_dev.c*//*
> *//*@@ -114,6 +114,7 @@ int rte_eal_dev_detach(struct rte_device *dev)*//*
> *//*        if (ret)*//*
> *//*                RTE_LOG(ERR, EAL, "Driver cannot detach the device 
> (%s)\n",*//*
> *//*                        dev->name);*//*
> *//*+       dev->driver = NULL;*//*
> *//*        return ret;*//*
> *//* }*//*
> *//**//*
> *//*diff --git a/lib/librte_ether/rte_ethdev_pci.h 
> b/lib/librte_ether/rte_ethdev_pci.h*//*
> *//*index 722075e09..a79188fbf 100644*//*
> *//*--- a/lib/librte_ether/rte_ethdev_pci.h*//*
> *//*+++ b/lib/librte_ether/rte_ethdev_pci.h*//*
> *//*@@ -125,16 +125,16 @@ rte_eth_dev_pci_release(struct rte_eth_dev 
> *eth_dev)*//*
> *//*        /* free ether device */*//*
> *//*        rte_eth_dev_release_port(eth_dev);*//*
> *//**//*
> *//*-       if (rte_eal_process_type() == RTE_PROC_PRIMARY)*//*
> *//*+       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {*//*
> *//*rte_free(eth_dev->data->dev_private);*//*
> *//*+ eth_dev->data->dev_private = NULL;*//*
> *//**//*
> *//*-       eth_dev->data->dev_private = NULL;*//*
> *//*-*//*
> *//*-       /**//*
> *//*-        * Secondary process will check the name to attach.*//*
> *//*-        * Clear this field to avoid attaching a released ports.*//*
> *//*-        */*//*
> *//*-       eth_dev->data->name[0] = '\0';*//*
> *//*+               /**//*
> *//*+                * Secondary process will check the name to 
> attach.*//*
> *//*+                * Clear this field to avoid attaching a released 
> ports.*//*
> *//*+                */*//*
> *//*+ eth_dev->data->name[0] = '\0';*//*
> *//*+        }*//*
> *//**//*
> *//*        eth_dev->device = NULL;*//*
> *//*        eth_dev->intr_handle = NULL;*//*
> *//*@@ -184,7 +184,7 @@ rte_eth_dev_pci_generic_remove(struct 
> rte_pci_device *pci_dev,*//*
> *//**//*
> *//*        if (dev_uninit) {*//*
> *//*                ret = dev_uninit(eth_dev);*//*
> *//*-               if (ret)*//*
> *//*+               if (ret && ret != -EPERM)*//*
> *//*                        return ret;*//*
> *//*        }*//*
> *//**//*
> */
>
>
> And now seems to work. Is this a correct way to proceed?
>
> Could some one that have work with this functionalities can advise us?
>
> Best regards,
>
> Ricardo
>
>