[1/3] eal: remove useless checks for already probed device
Checks
Commit Message
The function eal_dev_hotplug_request_to_secondary() never returns
-EEXIST result. The case of already probed device is filtered out.
The test in __handle_secondary_request() was always true.
The test in rte_dev_probe() was never true, and that's fine not
returning -EEXIST if device is already attached in secondary processes.
Fixes: 494db286f37d ("eal: fix multi-process hotplug if attached in secondary")
Cc: dariusz.stojaczyk@intel.com
Cc: qi.z.zhang@intel.com
Cc: stable@dpdk.org
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
lib/librte_eal/common/eal_common_dev.c | 9 +--------
lib/librte_eal/common/hotplug_mp.c | 3 +--
2 files changed, 2 insertions(+), 10 deletions(-)
Comments
Hi:
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Saturday, March 2, 2019 10:43 AM
> To: dev@dpdk.org
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH 1/3] eal: remove useless checks for already probed device
>
> The function eal_dev_hotplug_request_to_secondary() never returns -EEXIST
> result. The case of already probed device is filtered out.
>
> The test in __handle_secondary_request() was always true.
> The test in rte_dev_probe() was never true, and that's fine not returning -EEXIST
> if device is already attached in secondary processes.
I didn't get this.
eal_dev_hotplug_request_to_secondary() never return -EEXIST, but req->result could be -EEXIST.
This happens when secondary try to attach an already attached device
(__handle_primary_request --> local_dev_probe --> dev->bus->plug )
>
> Fixes: 494db286f37d ("eal: fix multi-process hotplug if attached in secondary")
> Cc: dariusz.stojaczyk@intel.com
> Cc: qi.z.zhang@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> lib/librte_eal/common/eal_common_dev.c | 9 +--------
> lib/librte_eal/common/hotplug_mp.c | 3 +--
> 2 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_dev.c
> b/lib/librte_eal/common/eal_common_dev.c
> index fd7f5ca7d5..048c0b025f 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -247,18 +247,11 @@ rte_dev_probe(const char *devargs)
> goto rollback;
> }
>
> - /**
> - * if any secondary failed to attach, we need to consider if rollback
> - * is necessary.
> - */
> + /* if any secondary failed to attach, need to rollback. */
> if (req.result != 0) {
> RTE_LOG(ERR, EAL,
> "Failed to attach device on secondary process\n");
> ret = req.result;
> -
> - /* for -EEXIST, we don't need to rollback. */
> - if (ret == -EEXIST)
> - return ret;
> goto rollback;
> }
>
> diff --git a/lib/librte_eal/common/hotplug_mp.c
> b/lib/librte_eal/common/hotplug_mp.c
> index 4052a5c7fb..94bd1d896e 100644
> --- a/lib/librte_eal/common/hotplug_mp.c
> +++ b/lib/librte_eal/common/hotplug_mp.c
> @@ -110,8 +110,7 @@ __handle_secondary_request(void *param)
> if (tmp_req.result != 0) {
> ret = tmp_req.result;
> RTE_LOG(ERR, EAL, "Failed to hotplug add device on
> secondary\n");
> - if (ret != -EEXIST)
> - goto rollback;
> + goto rollback;
> }
> } else if (req->t == EAL_DEV_REQ_TYPE_DETACH) {
> ret = rte_devargs_parse(&da, req->devargs);
> --
> 2.20.1
On Wed, 13 Mar 2019 13:46:01 +0000
"Zhang, Qi Z" <qi.z.zhang@intel.com> wrote:
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Saturday, March 2, 2019 10:43 AM
> > To: dev@dpdk.org
> > Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; stable@dpdk.org
> > Subject: [PATCH 1/3] eal: remove useless checks for already probed device
> >
> > The function eal_dev_hotplug_request_to_secondary() never returns -EEXIST
> > result. The case of already probed device is filtered out.
> >
> > The test in __handle_secondary_request() was always true.
> > The test in rte_dev_probe() was never true, and that's fine not returning -EEXIST
> > if device is already attached in secondary processes.
>
> I didn't get this.
> eal_dev_hotplug_request_to_secondary() never return -EEXIST, but req->result could be -EEXIST.
>
> This happens when secondary try to attach an already attached device
> (__handle_primary_request --> local_dev_probe --> dev->bus->plug )
This seems to be the outstanding question on this old patch.
Is this possible? If so then the original code is ok, and patch is not required.
@@ -247,18 +247,11 @@ rte_dev_probe(const char *devargs)
goto rollback;
}
- /**
- * if any secondary failed to attach, we need to consider if rollback
- * is necessary.
- */
+ /* if any secondary failed to attach, need to rollback. */
if (req.result != 0) {
RTE_LOG(ERR, EAL,
"Failed to attach device on secondary process\n");
ret = req.result;
-
- /* for -EEXIST, we don't need to rollback. */
- if (ret == -EEXIST)
- return ret;
goto rollback;
}
@@ -110,8 +110,7 @@ __handle_secondary_request(void *param)
if (tmp_req.result != 0) {
ret = tmp_req.result;
RTE_LOG(ERR, EAL, "Failed to hotplug add device on secondary\n");
- if (ret != -EEXIST)
- goto rollback;
+ goto rollback;
}
} else if (req->t == EAL_DEV_REQ_TYPE_DETACH) {
ret = rte_devargs_parse(&da, req->devargs);