[dpdk-dev] [PATCH] dev: fix attach rollback of a device that was already attached

[dpdk-dev] [PATCH] dev: fix attach rollback of a device that was already attached

[Darek Stojaczyk]

When primary process receives an IPC attach request
of a device that's already locally-attached, it
doesn't setup its variables properly and is prone to
segfaulting on a subsequent rollback.

`ret = local_dev_probe(req->devargs, &dev)`

The above function will set `dev` pointer to the
proper device *unless* it returns with error. One of
those errors is -EEXIST, which the hotplug function
explicitly ignores. For -EEXIST, it proceeds with
attaching the device and expects the dev pointer to
be valid.

Despite this patch being a fix, it also introduces
a design decision - when any secondary process fails
to attach a device, the primary process that already
had the device attached won't attempt to detach that
device locally as a part of the rollback routine.
Primary process would have already printed a message
"Failed to [...] on secondary" and now it will also
print a warning "Devices may not be in sync [...]".

Fixes: ac9e4a17370f ("eal: support attach/detach shared device from secondary")
Cc: qi.z.zhang@intel.com

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 lib/librte_eal/common/hotplug_mp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/hotplug_mp.c b/lib/librte_eal/common/hotplug_mp.c
index 7c9fcc46c..7ee074a31 100644
--- a/lib/librte_eal/common/hotplug_mp.c
+++ b/lib/librte_eal/common/hotplug_mp.c
@@ -88,7 +88,7 @@ __handle_secondary_request(void *param)
 		(const struct eal_dev_mp_req *)msg->param;
 	struct eal_dev_mp_req tmp_req;
 	struct rte_devargs *da;
-	struct rte_device *dev;
+	struct rte_device *dev = NULL;
 	struct rte_bus *bus;
 	int ret = 0;
 
@@ -168,7 +168,15 @@ __handle_secondary_request(void *param)
 	if (req->t == EAL_DEV_REQ_TYPE_ATTACH) {
 		tmp_req.t = EAL_DEV_REQ_TYPE_ATTACH_ROLLBACK;
 		eal_dev_hotplug_request_to_secondary(&tmp_req);
-		local_dev_remove(dev);
+		if (dev == NULL) {
+			/* device was already attached at the time we got the
+			 * request, don't detach it now.
+			 */
+			RTE_LOG(WARNING, EAL,
+				"Devices in secondary may not sync with primary\n");
+		} else {
+			local_dev_remove(dev);
+		}
 	} else {
 		tmp_req.t = EAL_DEV_REQ_TYPE_DETACH_ROLLBACK;
 		eal_dev_hotplug_request_to_secondary(&tmp_req);
-- 
2.17.1

Re: [dpdk-dev] [PATCH] dev: fix attach rollback of a device that was already attached

[Zhang, Qi Z]

> -----Original Message-----
> From: Stojaczyk, Dariusz
> Sent: Friday, November 23, 2018 6:45 AM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [PATCH] dev: fix attach rollback of a device that was already attached
> 
> When primary process receives an IPC attach request of a device that's already
> locally-attached, it doesn't setup its variables properly and is prone to segfaulting
> on a subsequent rollback.
> 
> `ret = local_dev_probe(req->devargs, &dev)`
> 
> The above function will set `dev` pointer to the proper device *unless* it returns
> with error. One of those errors is -EEXIST, which the hotplug function explicitly
> ignores. For -EEXIST, it proceeds with attaching the device and expects the dev
> pointer to be valid.

Good capture.
> 
> Despite this patch being a fix, it also introduces a design decision - when any
> secondary process fails to attach a device, the primary process that already had
> the device attached won't attempt to detach that device locally as a part of the
> rollback routine.
> Primary process would have already printed a message "Failed to [...] on
> secondary" and now it will also print a warning "Devices may not be in sync [...]".

A little bit concern for this.
we may try to avoid the abnormal situation that device is not synced.
The scenario you describe actually is start from an abnormal situation due to some previous error.
so is it better to always take chance to end up with a normal situation.

It looks better for me if we can fixed it in local_dev_probe to return a valid device with -EEXIST.

> 
> Fixes: ac9e4a17370f ("eal: support attach/detach shared device from
> secondary")
> Cc: qi.z.zhang@intel.com
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>  lib/librte_eal/common/hotplug_mp.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/hotplug_mp.c
> b/lib/librte_eal/common/hotplug_mp.c
> index 7c9fcc46c..7ee074a31 100644
> --- a/lib/librte_eal/common/hotplug_mp.c
> +++ b/lib/librte_eal/common/hotplug_mp.c
> @@ -88,7 +88,7 @@ __handle_secondary_request(void *param)
>  		(const struct eal_dev_mp_req *)msg->param;
>  	struct eal_dev_mp_req tmp_req;
>  	struct rte_devargs *da;
> -	struct rte_device *dev;
> +	struct rte_device *dev = NULL;
>  	struct rte_bus *bus;
>  	int ret = 0;
> 
> @@ -168,7 +168,15 @@ __handle_secondary_request(void *param)
>  	if (req->t == EAL_DEV_REQ_TYPE_ATTACH) {
>  		tmp_req.t = EAL_DEV_REQ_TYPE_ATTACH_ROLLBACK;
>  		eal_dev_hotplug_request_to_secondary(&tmp_req);
> -		local_dev_remove(dev);
> +		if (dev == NULL) {
> +			/* device was already attached at the time we got the
> +			 * request, don't detach it now.
> +			 */
> +			RTE_LOG(WARNING, EAL,
> +				"Devices in secondary may not sync with primary\n");
> +		} else {
> +			local_dev_remove(dev);
> +		}
>  	} else {
>  		tmp_req.t = EAL_DEV_REQ_TYPE_DETACH_ROLLBACK;
>  		eal_dev_hotplug_request_to_secondary(&tmp_req);
> --
> 2.17.1

Re: [dpdk-dev] [PATCH] dev: fix attach rollback of a device that was already attached

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Friday, November 23, 2018 8:11 PM
> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; dev@dpdk.org
> Cc: thomas@monjalon.net
> Subject: RE: [PATCH] dev: fix attach rollback of a device that was already
> attached
> 
> 
> 
> > -----Original Message-----
> > From: Stojaczyk, Dariusz
> > Sent: Friday, November 23, 2018 6:45 AM
> > To: dev@dpdk.org
> > Cc: thomas@monjalon.net; Stojaczyk, Dariusz
> <dariusz.stojaczyk@intel.com>;
> > Zhang, Qi Z <qi.z.zhang@intel.com>
> > Subject: [PATCH] dev: fix attach rollback of a device that was already
> attached
> >
> > When primary process receives an IPC attach request of a device that's
> already
> > locally-attached, it doesn't setup its variables properly and is prone to
> segfaulting
> > on a subsequent rollback.
> >
> > `ret = local_dev_probe(req->devargs, &dev)`
> >
> > The above function will set `dev` pointer to the proper device *unless* it
> returns
> > with error. One of those errors is -EEXIST, which the hotplug function
> explicitly
> > ignores. For -EEXIST, it proceeds with attaching the device and expects the
> dev
> > pointer to be valid.
> 
> Good capture.
> >
> > Despite this patch being a fix, it also introduces a design decision - when
> any
> > secondary process fails to attach a device, the primary process that already
> had
> > the device attached won't attempt to detach that device locally as a part of
> the
> > rollback routine.
> > Primary process would have already printed a message "Failed to [...] on
> > secondary" and now it will also print a warning "Devices may not be in sync
> [...]".
> 
> A little bit concern for this.
> we may try to avoid the abnormal situation that device is not synced.
> The scenario you describe actually is start from an abnormal situation due to
> some previous error.
> so is it better to always take chance to end up with a normal situation.
> 
> It looks better for me if we can fixed it in local_dev_probe to return a valid
> device with -EEXIST.

Actually that was my original idea, but I gave it up in the end.
Ok, I'll do that in V2.

Thanks,
D.

> 
> >
> > Fixes: ac9e4a17370f ("eal: support attach/detach shared device from
> > secondary")
> > Cc: qi.z.zhang@intel.com
> >
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > ---
> >  lib/librte_eal/common/hotplug_mp.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/hotplug_mp.c
> > b/lib/librte_eal/common/hotplug_mp.c
> > index 7c9fcc46c..7ee074a31 100644
> > --- a/lib/librte_eal/common/hotplug_mp.c
> > +++ b/lib/librte_eal/common/hotplug_mp.c
> > @@ -88,7 +88,7 @@ __handle_secondary_request(void *param)
> >  		(const struct eal_dev_mp_req *)msg->param;
> >  	struct eal_dev_mp_req tmp_req;
> >  	struct rte_devargs *da;
> > -	struct rte_device *dev;
> > +	struct rte_device *dev = NULL;
> >  	struct rte_bus *bus;
> >  	int ret = 0;
> >
> > @@ -168,7 +168,15 @@ __handle_secondary_request(void *param)
> >  	if (req->t == EAL_DEV_REQ_TYPE_ATTACH) {
> >  		tmp_req.t = EAL_DEV_REQ_TYPE_ATTACH_ROLLBACK;
> >  		eal_dev_hotplug_request_to_secondary(&tmp_req);
> > -		local_dev_remove(dev);
> > +		if (dev == NULL) {
> > +			/* device was already attached at the time we got
> the
> > +			 * request, don't detach it now.
> > +			 */
> > +			RTE_LOG(WARNING, EAL,
> > +				"Devices in secondary may not sync with
> primary\n");
> > +		} else {
> > +			local_dev_remove(dev);
> > +		}
> >  	} else {
> >  		tmp_req.t = EAL_DEV_REQ_TYPE_DETACH_ROLLBACK;
> >  		eal_dev_hotplug_request_to_secondary(&tmp_req);
> > --
> > 2.17.1

[dpdk-dev] [PATCH v2] dev: fix attach rollback of a device that was already attached

[Darek Stojaczyk]

When primary process receives an IPC attach request
of a device that's already locally-attached, it
doesn't setup its variables properly and is prone to
segfaulting on a subsequent rollback.

`ret = local_dev_probe(req->devargs, &dev)`

The above function will set `dev` pointer to the
proper device *unless* it returns with error. One of
those errors is -EEXIST, which the hotplug function
explicitly ignores. For -EEXIST, it proceeds with
attaching the device and expects the dev pointer to
be valid.

This patch makes `local_dev_probe` set the dev pointer
even if it returns -EEXIST.

Fixes: ac9e4a17370f ("eal: support attach/detach shared device from secondary")
Cc: qi.z.zhang@intel.com

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
Changes since v1:
 * attempt to detach the device in primary process (Qi)

 lib/librte_eal/common/eal_common_dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 1fdc9ab17..a08dc085f 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -168,16 +168,14 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 	}
 
 	ret = dev->bus->plug(dev);
-	if (ret) {
-		if (rte_dev_is_probed(dev)) /* if already succeeded earlier */
-			return ret; /* no rollback */
+	if (ret && !rte_dev_is_probed(dev)) { /* if hasn't ever succeeded */
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
 		goto err_devarg;
 	}
 
 	*new_dev = dev;
-	return 0;
+	return ret;
 
 err_devarg:
 	if (rte_devargs_remove(da) != 0) {
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] dev: fix attach rollback of a device that was already attached

[Thomas Monjalon]

23/11/2018 22:26, Darek Stojaczyk:
> When primary process receives an IPC attach request
> of a device that's already locally-attached, it
> doesn't setup its variables properly and is prone to
> segfaulting on a subsequent rollback.
> 
> `ret = local_dev_probe(req->devargs, &dev)`
> 
> The above function will set `dev` pointer to the
> proper device *unless* it returns with error. One of
> those errors is -EEXIST, which the hotplug function
> explicitly ignores. For -EEXIST, it proceeds with
> attaching the device and expects the dev pointer to
> be valid.
> 
> This patch makes `local_dev_probe` set the dev pointer
> even if it returns -EEXIST.
> 
> Fixes: ac9e4a17370f ("eal: support attach/detach shared device from secondary")
> Cc: qi.z.zhang@intel.com
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Applied, thanks