From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41]) by dpdk.org (Postfix) with ESMTP id 1D0557D06 for ; Wed, 13 Dec 2017 18:00:10 +0100 (CET) Received: by mail-wm0-f41.google.com with SMTP id g75so6418320wme.0 for ; Wed, 13 Dec 2017 09:00:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bequant-com.20150623.gappssmtp.com; s=20150623; h=subject:references:to:from:message-id:date:user-agent:mime-version :in-reply-to:content-language; bh=VQ9h8qiavoK0sN5Hk9sebBWwdGu41hrVa9hcAtR4Sk0=; b=sknh6vl0H5V0fC5xEF6Nm8n3yiToXII1aZwyhdMx/6d5ASUwsJlYOIab4CJmyySIF0 j/PQBaWOq9Y1se8Y4lioS16dEflXnss0C37XVhQsabhDJt1KXdHErbIrhshoB726BNgc BQCGSA/xpceLiShDGiTq+5MDLnNQ88ZGgwz60LVXJMhdDz+eA2YqfKey3B3QMr0sUnTA MVmfr7BLKRupJ6oeMoSA4qPRWltNqXcK1edeIJHohOwrdxLFyTIWlA2aPzgY65GpXE8z a0iD+WbxsD57ymxG5+T4uwooLXSQTPvFEN8C+p1gHsHZq4fFLRDIOFemr72ai9rIvtNt EwHg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:references:to:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=VQ9h8qiavoK0sN5Hk9sebBWwdGu41hrVa9hcAtR4Sk0=; b=P+K/E9IcqK4yMfrFNiGiOoVkmgbxfHWv0NMHJ/8ktWp919L6K4H1jBrvtBKdopMnrR f9hAcS6DivdLFTkx177iTdyZ+gNfnSrD13hmwLhwy24kJi60zSIcphPxB+5vVGzMbADF 5OU8WnjgSMlUL3O+7s6Hy/X/LCNaRTb1fnpYqBjVF3d9CSHT0UcKKiDgPaVQOLDMXqcA VpJXaMxgVwNS3vm2f13ciKzihNeZ3+Fwu3zanIOMSQsXrWpMhAD4MpmrTtpGT9ffEi4n KdcBkE+QKvNW0UM0hZy2YwL28ckaw0EH4kSvibM0kmmW4ys8PSLhMxTFp86VUQeWIdG3 MKvA== X-Gm-Message-State: AKGB3mJTON9jqtJ0xzR63+Dc0ziDs3yQO2+7lFJrnhD8yQB/ECQBJJCT 1bhNnrOgAr0YyDriT5GSEIGSsAzIYmE= X-Google-Smtp-Source: ACJfBouRSvaSebM5iSdbMCW3Db0ROfjvEgj8/IeRzWX7+NsUGgLG3Kc5T0xkKFcOU8G1xbWnRV0Bpw== X-Received: by 10.80.158.194 with SMTP id a60mr8324211edf.228.1513184410210; Wed, 13 Dec 2017 09:00:10 -0800 (PST) Received: from [172.27.2.248] (static-166-190-26-46.ipcom.comunitel.net. [46.26.190.166]) by smtp.gmail.com with ESMTPSA id h4sm1754745edb.16.2017.12.13.09.00.09 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 13 Dec 2017 09:00:09 -0800 (PST) References: To: users@dpdk.org From: Ricardo Roldan X-Forwarded-Message-Id: Message-ID: <7a3a2174-831f-caa8-ed33-0f06133c96a2@bequant.com> Date: Wed, 13 Dec 2017 17:58:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: [dpdk-users] attach/detach on secondary process X-BeenThere: users@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK usage discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Dec 2017 17:00:11 -0000 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 > >