[dpdk-dev] [PATCH 1/3] bus/pci: update device devargs on each rescan

[dpdk-dev] [PATCH 1/3] bus/pci: update device devargs on each rescan

[Darek Stojaczyk]

Bus rescan is done e.g. during the device hotplug,
where devargs are re-allocated. By not updating the
rte_device->devargs pointer we potentially make it
a dangling one, as previous devargs could have been
(or will be soon) freed.

Fixes: 55e411b301c3 ("bus/pci: fix resource mapping override")
Cc: qi.z.zhang@intel.com

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 drivers/bus/pci/linux/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index f87533c5c..f3172960e 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -349,10 +349,10 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 			if (ret < 0) {
 				rte_pci_insert_device(dev2, dev);
 			} else { /* already registered */
+				pci_name_set(dev2);
 				if (!rte_dev_is_probed(&dev2->device)) {
 					dev2->kdrv = dev->kdrv;
 					dev2->max_vfs = dev->max_vfs;
-					pci_name_set(dev2);
 					memmove(dev2->mem_resource,
 						dev->mem_resource,
 						sizeof(dev->mem_resource));
-- 
2.17.1

[dpdk-dev] [PATCH 2/3] devargs: delay freeing previous devargs when overriding them

[Darek Stojaczyk]

In eal hotplug path, the previous devargs may be still
referenced by device structs at the time the rte_devargs_insert()
is called. Those references are updated almost immediately
afterwards, but in cases something goes wrong and they cannot
be updated, we might want to still keep the old devargs around.

This patch modifies rte_devargs_insert() so that it returns
a pointer to previous devargs that are being overridden. In
case something in the EAL hotplug path goes wrong, we can now
remove the newly inserted devargs and re-insert the old ones.

Note: Functional changes will come in a subsequent patch. This
      one only extends the API.

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 drivers/bus/vdev/vdev.c                     | 12 +++++++++---
 lib/librte_eal/common/eal_common_dev.c      | 11 +++++++----
 lib/librte_eal/common/eal_common_devargs.c  | 21 ++++++++++++++-------
 lib/librte_eal/common/include/rte_devargs.h |  8 ++------
 4 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 9c66bdc78..bbdae2314 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -9,6 +9,7 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include <sys/queue.h>
+#include <assert.h>
 
 #include <rte_eal.h>
 #include <rte_dev.h>
@@ -207,7 +208,7 @@ insert_vdev(const char *name, const char *args,
 		bool init)
 {
 	struct rte_vdev_device *dev;
-	struct rte_devargs *devargs;
+	struct rte_devargs *devargs, *prev_devargs;
 	int ret;
 
 	if (name == NULL)
@@ -239,8 +240,13 @@ insert_vdev(const char *name, const char *args,
 	}
 
 	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
-	if (init)
-		rte_devargs_insert(devargs);
+	if (init) {
+		rte_devargs_insert(devargs, &prev_devargs);
+		 /* any previous devargs should have been caught by the above
+		  * find_vdev(name) check
+		  */
+		assert(prev_devargs == NULL);
+	}
 
 	if (p_dev)
 		*p_dev = dev;
diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 62e9ed477..4cb424df1 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -131,7 +131,7 @@ int
 local_dev_probe(const char *devargs, struct rte_device **new_dev)
 {
 	struct rte_device *dev;
-	struct rte_devargs *da;
+	struct rte_devargs *da, *prev_da;
 	int ret;
 
 	*new_dev = NULL;
@@ -150,9 +150,12 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 		goto err_devarg;
 	}
 
-	ret = rte_devargs_insert(da);
-	if (ret)
-		goto err_devarg;
+	rte_devargs_insert(da, &prev_da);
+
+	if (prev_da != NULL) {
+		free(prev_da->args);
+		free(prev_da);
+	}
 
 	ret = da->bus->scan();
 	if (ret)
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index b7b9cb69e..c46365b69 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -262,16 +262,23 @@ rte_devargs_parsef(struct rte_devargs *da, const char *format, ...)
 	return ret;
 }
 
-int __rte_experimental
-rte_devargs_insert(struct rte_devargs *da)
+void __rte_experimental
+rte_devargs_insert(struct rte_devargs *da, struct rte_devargs **prev_da)
 {
-	int ret;
+	struct rte_devargs *d;
+	void *tmp;
+
+	*prev_da = NULL;
+	TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
+		if (strcmp(d->bus->name, da->bus->name) == 0 &&
+		    strcmp(d->name, da->name) == 0) {
+			TAILQ_REMOVE(&devargs_list, d, next);
+			*prev_da = d;
+			break;
+		}
+	}
 
-	ret = rte_devargs_remove(da);
-	if (ret < 0)
-		return ret;
 	TAILQ_INSERT_TAIL(&devargs_list, da, next);
-	return 0;
 }
 
 /* store a whitelist parameter for later parsing */
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index b1f121f83..753cf7f54 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -146,14 +146,10 @@ __attribute__((format(printf, 2, 0)));
  *
  * @param da
  *  The devargs structure to insert.
- *
- * @return
- *   - 0 on success
- *   - Negative on error.
  */
 __rte_experimental
-int
-rte_devargs_insert(struct rte_devargs *da);
+void
+rte_devargs_insert(struct rte_devargs *da, struct rte_devargs **prev_da);
 
 /**
  * Add a device to the user device list
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/3] devargs: delay freeing previous devargs when overriding them

[Thomas Monjalon]

Hi,

05/11/2018 08:04, Darek Stojaczyk:
> -int __rte_experimental
> -rte_devargs_insert(struct rte_devargs *da)
> +void __rte_experimental
> +rte_devargs_insert(struct rte_devargs *da, struct rte_devargs **prev_da)

You should update the API section of the release notes.

>  {
> -       int ret;
> +       struct rte_devargs *d;
> +       void *tmp;
> +
> +       *prev_da = NULL;
> +       TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> +               if (strcmp(d->bus->name, da->bus->name) == 0 &&
> +                   strcmp(d->name, da->name) == 0) {
> +                       TAILQ_REMOVE(&devargs_list, d, next);
> +                       *prev_da = d;
> +                       break;
> +               }
> +       }
>  
> -       ret = rte_devargs_remove(da);
> -       if (ret < 0)
> -               return ret;

Why not updating rte_devargs_remove instead of duplicating its code?

Re: [dpdk-dev] [PATCH 2/3] devargs: delay freeing previous devargs when overriding them

[Stojaczyk, Dariusz]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, November 5, 2018 8:31 AM
> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 2/3] devargs: delay freeing previous devargs when
> overriding them
> 
> Hi,
> 
> 05/11/2018 08:04, Darek Stojaczyk:
> > -int __rte_experimental
> > -rte_devargs_insert(struct rte_devargs *da)
> > +void __rte_experimental
> > +rte_devargs_insert(struct rte_devargs *da, struct rte_devargs
> **prev_da)
> 
> You should update the API section of the release notes.

Even for experimental API? OK, I didn't know it's needed.

> 
> >  {
> > -       int ret;
> > +       struct rte_devargs *d;
> > +       void *tmp;
> > +
> > +       *prev_da = NULL;
> > +       TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> > +               if (strcmp(d->bus->name, da->bus->name) == 0 &&
> > +                   strcmp(d->name, da->name) == 0) {
> > +                       TAILQ_REMOVE(&devargs_list, d, next);
> > +                       *prev_da = d;
> > +                       break;
> > +               }
> > +       }
> >
> > -       ret = rte_devargs_remove(da);
> > -       if (ret < 0)
> > -               return ret;
> 
> Why not updating rte_devargs_remove instead of duplicating its code?
> 

We still want to preserve the functionality of rte_devargs_remove.
rte_devargs_remove does TAILQ_REMOVE + free; rte_devargs_insert does just TAILQ_REMOVE. (I think I also forgot to update rte_devargs_insert documentation, I'll  do that in V2)

Since you've mentioned it:
Eventually I'd see rte_devargs_remove to accept the exact same devargs parameter that was passed to rte_devargs_insert. Then rte_devargs_remove wouldn't do any sort of lookup. Maybe additional rte_devargs_find(const char *name) could be added for existing cases where the original devargs struct is not available. However, I'm not familiar enough with this code to perform the refactor and am just trying to fix the stuff. Still, how does it sound?

D.

Re: [dpdk-dev] [PATCH 2/3] devargs: delay freeing previous devargs when overriding them

[Thomas Monjalon]

05/11/2018 09:25, Stojaczyk, Dariusz:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 05/11/2018 08:04, Darek Stojaczyk:
> > > -int __rte_experimental
> > > -rte_devargs_insert(struct rte_devargs *da)
> > > +void __rte_experimental
> > > +rte_devargs_insert(struct rte_devargs *da, struct rte_devargs
> > **prev_da)
> > 
> > You should update the API section of the release notes.
> 
> Even for experimental API? OK, I didn't know it's needed.

Yes, even for experimental API, the API changes must documented.

> > >  {
> > > -       int ret;
> > > +       struct rte_devargs *d;
> > > +       void *tmp;
> > > +
> > > +       *prev_da = NULL;
> > > +       TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> > > +               if (strcmp(d->bus->name, da->bus->name) == 0 &&
> > > +                   strcmp(d->name, da->name) == 0) {
> > > +                       TAILQ_REMOVE(&devargs_list, d, next);
> > > +                       *prev_da = d;
> > > +                       break;
> > > +               }
> > > +       }
> > >
> > > -       ret = rte_devargs_remove(da);
> > > -       if (ret < 0)
> > > -               return ret;
> > 
> > Why not updating rte_devargs_remove instead of duplicating its code?
> 
> We still want to preserve the functionality of rte_devargs_remove.
> rte_devargs_remove does TAILQ_REMOVE + free; rte_devargs_insert does just TAILQ_REMOVE. (I think I also forgot to update rte_devargs_insert documentation, I'll  do that in V2)

Yes, because of the rollback, OK.
Please mention in devargs_insert doc that the old devargs
can be used for rollback.

> Since you've mentioned it:
> Eventually I'd see rte_devargs_remove to accept the exact same devargs parameter that was passed to rte_devargs_insert. Then rte_devargs_remove wouldn't do any sort of lookup. Maybe additional rte_devargs_find(const char *name) could be added for existing cases where the original devargs struct is not available. However, I'm not familiar enough with this code to perform the refactor and am just trying to fix the stuff. Still, how does it sound?

I think we can keep it as is.

We can re-think the whole thing in the next release.
I think we should not play with devargs list as we do.
There should be only lists for scanned devices of each bus and that's all.

Re: [dpdk-dev] [PATCH 2/3] devargs: delay freeing previous devargs when overriding them

[Gaëtan Rivet]

On Mon, Nov 05, 2018 at 10:46:39AM +0100, Thomas Monjalon wrote:
> 05/11/2018 09:25, Stojaczyk, Dariusz:
> > Hi Thomas,
> > 
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 05/11/2018 08:04, Darek Stojaczyk:
> > > > -int __rte_experimental
> > > > -rte_devargs_insert(struct rte_devargs *da)
> > > > +void __rte_experimental
> > > > +rte_devargs_insert(struct rte_devargs *da, struct rte_devargs
> > > **prev_da)
> > > 
> > > You should update the API section of the release notes.
> > 
> > Even for experimental API? OK, I didn't know it's needed.
> 
> Yes, even for experimental API, the API changes must documented.
> 
> > > >  {
> > > > -       int ret;
> > > > +       struct rte_devargs *d;
> > > > +       void *tmp;
> > > > +
> > > > +       *prev_da = NULL;
> > > > +       TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> > > > +               if (strcmp(d->bus->name, da->bus->name) == 0 &&
> > > > +                   strcmp(d->name, da->name) == 0) {
> > > > +                       TAILQ_REMOVE(&devargs_list, d, next);
> > > > +                       *prev_da = d;
> > > > +                       break;
> > > > +               }
> > > > +       }
> > > >
> > > > -       ret = rte_devargs_remove(da);
> > > > -       if (ret < 0)
> > > > -               return ret;
> > > 
> > > Why not updating rte_devargs_remove instead of duplicating its code?
> > 
> > We still want to preserve the functionality of rte_devargs_remove.
> > rte_devargs_remove does TAILQ_REMOVE + free; rte_devargs_insert does just TAILQ_REMOVE. (I think I also forgot to update rte_devargs_insert documentation, I'll  do that in V2)
> 
> Yes, because of the rollback, OK.
> Please mention in devargs_insert doc that the old devargs
> can be used for rollback.
> 

This is not ok. You are leaking those devargs that are not freed.
Once a devargs is inserted, the current API considers them to be managed
by EAL - i.e. no one else should manipulate them (get them out of the
list or freeing them). They are meant afterward to be linked by
rte_device, so the only moment they can be removed is right before a
bus scan (which will rebuild the devargs -> device mapping).

Returning the prev_devargs from devargs_insert() is a bad construct.
Insert only does insertion, not search+insert.

Either, devargs_insert() finds a previous occurence, and returns an
error EEXIST before aborting, or it forces the insertion. But it should
not do something else to avoid a function call.

> > Since you've mentioned it:
> > Eventually I'd see rte_devargs_remove to accept the exact same devargs parameter that was passed to rte_devargs_insert. Then rte_devargs_remove wouldn't do any sort of lookup. Maybe additional rte_devargs_find(const char *name) could be added for existing cases where the original devargs struct is not available. However, I'm not familiar enough with this code to perform the refactor and am just trying to fix the stuff. Still, how does it sound?
> 
> I think we can keep it as is.
> 
> We can re-think the whole thing in the next release.
> I think we should not play with devargs list as we do.
> There should be only lists for scanned devices of each bus and that's all.
> 
> 
> 

devargs_{insert,remove} currently identifies a devargs from its
(bus, name) pair. This pair is unique in the list. Accepting the exact
same devargs parameter is not possible, because sometimes you won't have
this devargs handle, you can only identify it by the pair above.

I think devargs_find() would give the wrong idea (returning a devargs
handle that is still in the devargs list: a user should not think it can
be manipulated this way).

Instead I'd propose:

devargs_insert() returns -EEXIST if the devargs identifying pair is found
                 in the list.
devargs_extract() will remove this devargs from the list and return it
                  if found. devargs_insert() afterward would work.
                  Once extracted, the devargs is the responsibility of
                  the caller (so should be freed if not needed anymore).
devargs_remove() stays the same, finding the devargs and freeing it, or
                 returning >0 if the devargs was not within the list.

Remaining issue is that extract() would invalidate a bunch of rte_device
pointers, which is not acceptable. Either a back-reference is made from
devargs to device, so that rte_devices are updated accordingly, or we must
ensure each device bus scan is called right after to make sure the mapping
is fixed.

-- 
Gaëtan Rivet
6WIND

[dpdk-dev] [PATCH 3/3] eal: handle bus rescan failures during hotplug

[Darek Stojaczyk]

Fixes for devargs dangling pointers and various segfaults
caused by failed hotplug requests.

If rescan failed, we have one the following scenarios
to cover:
 a) the device was never scanned at all; the devargs that
    were just allocated are not referenced anywhere and
    can be removed straight away. Previous devargs (if any)
    need to be re-inserted as they are still in use.
 b) the device was scanned before, but the rescan failed
    before it reached our device, meaning that its devargs
    were not overridden and we can remove our newly allocated
    ones. Previous devargs need to be re-inserted.
 c) we managed to rescan our device and only failed on
    something after that, which means we have to free the
    memory of previous devargs and stick with the new ones,
    despite the fact that the hotplug still needs to be
    failed.

Fixes: 7e8b26650146 ("eal: fix hotplug add / remove")
Cc: gaetan.rivet@6wind.com
Cc: qi.z.zhang@intel.com

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 lib/librte_eal/common/eal_common_dev.c | 30 ++++++++++++++++++--------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 4cb424df1..b090295b9 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -7,6 +7,7 @@
 #include <string.h>
 #include <inttypes.h>
 #include <sys/queue.h>
+#include <assert.h>
 
 #include <rte_compat.h>
 #include <rte_bus.h>
@@ -141,32 +142,33 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 
 	ret = rte_devargs_parse(da, devargs);
 	if (ret)
-		goto err_devarg;
+		goto err_remove_devargs;
 
 	if (da->bus->plug == NULL) {
 		RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
 			da->bus->name);
 		ret = -ENOTSUP;
-		goto err_devarg;
+		goto err_remove_devargs;
 	}
 
 	rte_devargs_insert(da, &prev_da);
 
+	ret = da->bus->scan();
+	if (ret)
+		goto err_rollback_devargs;
+
 	if (prev_da != NULL) {
 		free(prev_da->args);
 		free(prev_da);
+		prev_da = NULL;
 	}
 
-	ret = da->bus->scan();
-	if (ret)
-		goto err_devarg;
-
 	dev = da->bus->find_device(NULL, cmp_dev_name, da->name);
 	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find device (%s)\n",
 			da->name);
 		ret = -ENODEV;
-		goto err_devarg;
+		goto err_remove_devargs;
 	}
 
 	ret = dev->bus->plug(dev);
@@ -175,13 +177,23 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 			return ret; /* no rollback */
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
-		goto err_devarg;
+		goto err_remove_devargs;
 	}
 
 	*new_dev = dev;
 	return 0;
 
-err_devarg:
+err_rollback_devargs:
+	dev = da->bus->find_device(NULL, cmp_dev_name, da->name);
+	if (prev_da != NULL && (dev == NULL || dev->devargs != da)) {
+		/* either the device wasn't scanned, or we didn't
+		 * manage yet to update its devargs.
+		 */
+		rte_devargs_insert(prev_da, &da);
+		assert(da != NULL);
+	}
+
+err_remove_devargs:
 	if (rte_devargs_remove(da) != 0) {
 		free(da->args);
 		free(da);
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/3] bus/pci: update device devargs on each rescan

[Gaëtan Rivet]

Hi Darek,

On Mon, Nov 05, 2018 at 08:04:45AM +0100, Darek Stojaczyk wrote:
> Bus rescan is done e.g. during the device hotplug,
> where devargs are re-allocated. By not updating the
> rte_device->devargs pointer we potentially make it
> a dangling one, as previous devargs could have been
> (or will be soon) freed.
> 
> Fixes: 55e411b301c3 ("bus/pci: fix resource mapping override")
> Cc: qi.z.zhang@intel.com
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>  drivers/bus/pci/linux/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> index f87533c5c..f3172960e 100644
> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -349,10 +349,10 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
>  			if (ret < 0) {
>  				rte_pci_insert_device(dev2, dev);
>  			} else { /* already registered */
> +				pci_name_set(dev2);

This is rather unfortunate to call pci_name_set()
to trigger the mapping devargs <-> devices.

pci_devargs_lookup could be made non-static instead,
if that's sufficient. Given that the PCI id matches because the device
is a duplicate (already registered), then the name itself probably does
not need to be updated.

>  				if (!rte_dev_is_probed(&dev2->device)) {
>  					dev2->kdrv = dev->kdrv;
>  					dev2->max_vfs = dev->max_vfs;
> -					pci_name_set(dev2);
>  					memmove(dev2->mem_resource,
>  						dev->mem_resource,
>  						sizeof(dev->mem_resource));
> -- 
> 2.17.1
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH 1/3] bus/pci: update device devargs on each rescan

[Stojaczyk, Dariusz]

Hi Gaetan,

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Subject: Re: [dpdk-dev] [PATCH 1/3] bus/pci: update device devargs on each
> rescan
> 
> Hi Darek,
> 
> On Mon, Nov 05, 2018 at 08:04:45AM +0100, Darek Stojaczyk wrote:
> > Bus rescan is done e.g. during the device hotplug,
> > where devargs are re-allocated. By not updating the
> > rte_device->devargs pointer we potentially make it
> > a dangling one, as previous devargs could have been
> > (or will be soon) freed.
> >
> > Fixes: 55e411b301c3 ("bus/pci: fix resource mapping override")
> > Cc: qi.z.zhang@intel.com
> >
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > ---
> >  drivers/bus/pci/linux/pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> > index f87533c5c..f3172960e 100644
> > --- a/drivers/bus/pci/linux/pci.c
> > +++ b/drivers/bus/pci/linux/pci.c
> > @@ -349,10 +349,10 @@ pci_scan_one(const char *dirname, const struct
> rte_pci_addr *addr)
> >  			if (ret < 0) {
> >  				rte_pci_insert_device(dev2, dev);
> >  			} else { /* already registered */
> > +				pci_name_set(dev2);
> 
> This is rather unfortunate to call pci_name_set()
> to trigger the mapping devargs <-> devices.
> 
> pci_devargs_lookup could be made non-static instead,
> if that's sufficient. 

This is unfortunately trigerred by the generic eal device hotplug path which looks as follows:

da = calloc(sizeof rte_devargs);
[...]
rte_devargs_insert(da); # frees the previous devargs that rte_device still references
da->bus->scan(); # this should update the dangling devargs/name pointer in rte_device

Instead of making pci_devargs_lookup public, we would have to introduce a new bus callback for updating devargs of only a single device and that would be a really, realy ugly overkill. Thomas already mentioned in the subsequent patch within this series that we might want to re-think the devargs design in the next release.

> Given that the PCI id matches because the device
> is a duplicate (already registered), then the name itself probably does
> not need to be updated.

The issue here was that old devargs/name could have been freed.
The new name is the same as the old one, it's just in a different buffer.

I know calling pci_name_set() on each device during scan is not perfect, but IMO it's enforced by the current rte_devargs design. Besides, this used to be the original behavior before 55e411b301c3 ("bus/pci: fix resource mapping override").

Thanks,
D.

> 
> >  <snip>
> 
> --
> Gaëtan Rivet
> 6WIND

Re: [dpdk-dev] [PATCH 1/3] bus/pci: update device devargs on each rescan

[Gaëtan Rivet]

On Mon, Nov 05, 2018 at 02:52:00PM +0000, Stojaczyk, Dariusz wrote:
> Hi Gaetan,
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Subject: Re: [dpdk-dev] [PATCH 1/3] bus/pci: update device devargs on each
> > rescan
> > 
> > Hi Darek,
> > 
> > On Mon, Nov 05, 2018 at 08:04:45AM +0100, Darek Stojaczyk wrote:
> > > Bus rescan is done e.g. during the device hotplug,
> > > where devargs are re-allocated. By not updating the
> > > rte_device->devargs pointer we potentially make it
> > > a dangling one, as previous devargs could have been
> > > (or will be soon) freed.
> > >
> > > Fixes: 55e411b301c3 ("bus/pci: fix resource mapping override")
> > > Cc: qi.z.zhang@intel.com
> > >
> > > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > > ---
> > >  drivers/bus/pci/linux/pci.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> > > index f87533c5c..f3172960e 100644
> > > --- a/drivers/bus/pci/linux/pci.c
> > > +++ b/drivers/bus/pci/linux/pci.c
> > > @@ -349,10 +349,10 @@ pci_scan_one(const char *dirname, const struct
> > rte_pci_addr *addr)
> > >  			if (ret < 0) {
> > >  				rte_pci_insert_device(dev2, dev);
> > >  			} else { /* already registered */
> > > +				pci_name_set(dev2);
> > 
> > This is rather unfortunate to call pci_name_set()
> > to trigger the mapping devargs <-> devices.
> > 
> > pci_devargs_lookup could be made non-static instead,
> > if that's sufficient. 
> 
> This is unfortunately trigerred by the generic eal device hotplug path which looks as follows:
> 
> da = calloc(sizeof rte_devargs);
> [...]
> rte_devargs_insert(da); # frees the previous devargs that rte_device still references
> da->bus->scan(); # this should update the dangling devargs/name pointer in rte_device
> 
> Instead of making pci_devargs_lookup public, we would have to introduce a new bus callback for updating devargs of only a single device and that would be a really, realy ugly overkill. Thomas already mentioned in the subsequent patch within this series that we might want to re-think the devargs design in the next release.
> 

No, of course, I'm not saying to make pci_devargs_lookup public, only
private to the PCI bus, and within the bus, non-static. This way,
instead of calling pci_name_set(), you call instead only
pci_devargs_lookup().

> > Given that the PCI id matches because the device
> > is a duplicate (already registered), then the name itself probably does
> > not need to be updated.
> 
> The issue here was that old devargs/name could have been freed.
> The new name is the same as the old one, it's just in a different buffer.
> 
> I know calling pci_name_set() on each device during scan is not perfect, but IMO it's enforced by the current rte_devargs design. Besides, this used to be the original behavior before 55e411b301c3 ("bus/pci: fix resource mapping override").
> 
> Thanks,
> D.
> 
> > 
> > >  <snip>
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND

[dpdk-dev] [PATCH v2] bus/pci: update device devargs on each rescan

[Dariusz Stojaczyk]

From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Bus rescan is done e.g. during the device hotplug,
where devargs are re-allocated. By not updating the
rte_device->devargs pointer we potentially make it
a dangling one, as previous devargs could have been
(or will be soon) freed.

Fixes: 55e411b301c3 ("bus/pci: fix resource mapping override")
Cc: qi.z.zhang@intel.com

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
Changes since v1:
  * introduce additional pci_devargs_lookup() check to make
    the code slightly more readable (Gaetan Rivet)

 drivers/bus/pci/linux/pci.c  |  5 +++++
 drivers/bus/pci/pci_common.c |  3 ++-
 drivers/bus/pci/private.h    | 11 +++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index c99d523f0..a32d4848e 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -379,6 +379,11 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 						RTE_LOG(ERR, EAL, "Unexpected device scan at %s!\n",
 							filename);
 				}
+
+				if (pci_devargs_lookup(dev2) !=
+						dev2->device.devargs)
+					pci_name_set(dev2);
+
 				free(dev);
 			}
 			return 0;
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 6276e5d69..fcaf76ed2 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -42,7 +42,8 @@ const char *rte_pci_get_sysfs_path(void)
 	return path;
 }
 
-static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
+struct rte_devargs *
+pci_devargs_lookup(struct rte_pci_device *dev)
 {
 	struct rte_devargs *devargs;
 	struct rte_pci_addr addr;
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index 13c3324bb..8b2e904f0 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -43,6 +43,17 @@ void
 pci_name_set(struct rte_pci_device *dev);
 
 /**
+ * Find matching up-to-date devargs for a given device.
+ *
+ * @param pci_dev
+ *   PCI device
+ * @return
+ *   A devargs pointer or NULL
+ */
+struct rte_devargs *
+pci_devargs_lookup(struct rte_pci_device *dev);
+
+/**
  * Add a PCI device to the PCI Bus (append to PCI Device list). This function
  * also updates the bus references of the PCI Device (and the generic device
  * object embedded within.
-- 
2.11.0

Re: [dpdk-dev] [PATCH v2] bus/pci: update device devargs on each rescan

[Zhang, Qi Z]

> -----Original Message-----
> From: Dariusz Stojaczyk [mailto:darek.stojaczyk@gmail.com]
> Sent: Monday, November 5, 2018 10:40 PM
> To: dev@dpdk.org; gaetan.rivet@6wind.com
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: [PATCH v2] bus/pci: update device devargs on each rescan
> 
> From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> 
> Bus rescan is done e.g. during the device hotplug, where devargs are
> re-allocated. By not updating the rte_device->devargs pointer we potentially
> make it a dangling one, as previous devargs could have been (or will be soon)
> freed.

Yes, this is the similar issue we met on vdev.

The key problem is we have rte_devargs_insert will destroy a devargs which is still referenced by a rte_device
I'm thinking , why not just keep always keep a snapshot of devargs in rte_device to make things simple?
We could introduce a API like rte_dev_assign_devargs(dev, devargs) which handled the clone and destroy stuff and can be used for all buses.
If that idea is acceptable, I would prefer the issue in this patch could be fixed in pci_name_set by clone a new copy as workaround.

> 
> Fixes: 55e411b301c3 ("bus/pci: fix resource mapping override")
> Cc: qi.z.zhang@intel.com
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
> Changes since v1:
>   * introduce additional pci_devargs_lookup() check to make
>     the code slightly more readable (Gaetan Rivet)
> 
>  drivers/bus/pci/linux/pci.c  |  5 +++++  drivers/bus/pci/pci_common.c |
> 3 ++-
>  drivers/bus/pci/private.h    | 11 +++++++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c index
> c99d523f0..a32d4848e 100644
> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -379,6 +379,11 @@ pci_scan_one(const char *dirname, const struct
> rte_pci_addr *addr)
>  						RTE_LOG(ERR, EAL, "Unexpected device scan
> at %s!\n",
>  							filename);
>  				}
> +
> +				if (pci_devargs_lookup(dev2) !=
> +						dev2->device.devargs)
> +					pci_name_set(dev2);
> +
>  				free(dev);
>  			}
>  			return 0;
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index 6276e5d69..fcaf76ed2 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -42,7 +42,8 @@ const char *rte_pci_get_sysfs_path(void)
>  	return path;
>  }
> 
> -static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
> +struct rte_devargs *
> +pci_devargs_lookup(struct rte_pci_device *dev)
>  {
>  	struct rte_devargs *devargs;
>  	struct rte_pci_addr addr;
> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h index
> 13c3324bb..8b2e904f0 100644
> --- a/drivers/bus/pci/private.h
> +++ b/drivers/bus/pci/private.h
> @@ -43,6 +43,17 @@ void
>  pci_name_set(struct rte_pci_device *dev);
> 
>  /**
> + * Find matching up-to-date devargs for a given device.
> + *
> + * @param pci_dev
> + *   PCI device
> + * @return
> + *   A devargs pointer or NULL
> + */
> +struct rte_devargs *
> +pci_devargs_lookup(struct rte_pci_device *dev);
> +
> +/**
>   * Add a PCI device to the PCI Bus (append to PCI Device list). This function
>   * also updates the bus references of the PCI Device (and the generic device
>   * object embedded within.
> --
> 2.11.0

Re: [dpdk-dev] [PATCH v2] bus/pci: update device devargs on each rescan

[Gaëtan Rivet]

On Tue, Nov 06, 2018 at 10:21:38PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Dariusz Stojaczyk [mailto:darek.stojaczyk@gmail.com]
> > Sent: Monday, November 5, 2018 10:40 PM
> > To: dev@dpdk.org; gaetan.rivet@6wind.com
> > Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Subject: [PATCH v2] bus/pci: update device devargs on each rescan
> > 
> > From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > 
> > Bus rescan is done e.g. during the device hotplug, where devargs are
> > re-allocated. By not updating the rte_device->devargs pointer we potentially
> > make it a dangling one, as previous devargs could have been (or will be soon)
> > freed.
> 
> Yes, this is the similar issue we met on vdev.
> 
> The key problem is we have rte_devargs_insert will destroy a devargs which is still referenced by a rte_device
> I'm thinking , why not just keep always keep a snapshot of devargs in rte_device to make things simple?
> We could introduce a API like rte_dev_assign_devargs(dev, devargs) which handled the clone and destroy stuff and can be used for all buses.
> If that idea is acceptable, I would prefer the issue in this patch could be fixed in pci_name_set by clone a new copy as workaround.
> 

I agree that it would be useful to have

    rte_devargs_map(devargs, device);

That will link safely a devargs to a device, but cloning is overkill.

I disagree that dangling pointers and loose references is fixed by
introducing more cloning and copies of stuff here and there.

We must tighten the device -> devargs coupling, not loosen it.

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v2] bus/pci: update device devargs on each rescan

[Thomas Monjalon]

07/11/2018 00:40, Gaëtan Rivet:
> On Tue, Nov 06, 2018 at 10:21:38PM +0000, Zhang, Qi Z wrote:
> > From: Dariusz Stojaczyk [mailto:darek.stojaczyk@gmail.com]
> > > From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > > 
> > > Bus rescan is done e.g. during the device hotplug, where devargs are
> > > re-allocated. By not updating the rte_device->devargs pointer we potentially
> > > make it a dangling one, as previous devargs could have been (or will be soon)
> > > freed.
> > 
> > Yes, this is the similar issue we met on vdev.
> > 
> > The key problem is we have rte_devargs_insert will destroy a devargs which is still referenced by a rte_device
> > I'm thinking , why not just keep always keep a snapshot of devargs in rte_device to make things simple?
> > We could introduce a API like rte_dev_assign_devargs(dev, devargs) which handled the clone and destroy stuff and can be used for all buses.
> > If that idea is acceptable, I would prefer the issue in this patch could be fixed in pci_name_set by clone a new copy as workaround.
> > 
> 
> I agree that it would be useful to have
> 
>     rte_devargs_map(devargs, device);
> 
> That will link safely a devargs to a device, but cloning is overkill.
> 
> I disagree that dangling pointers and loose references is fixed by
> introducing more cloning and copies of stuff here and there.
> 
> We must tighten the device -> devargs coupling, not loosen it.

This issue is fixed with a different approach:
	http://git.dpdk.org/dpdk/commit/?id=c7ad7754
	devargs: do not replace already inserted device