Re: [dpdk-users] attach/detach on secondary process

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 13 Dec 2017 17:58:53 +0100
Ricardo Roldan <rroldan@bequant.com> wrote:

> 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
> >
> >  
> 

Many DPDK drivers require that setup and initialization be done by
the primary process. This is mostly to avoid dealing with concurrency since
there can be multiple secondary processes.