[dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Qi Zhang]

When probe the same device at second time, devargs will be
replaced in devargs_list, old version is destoried but they
are still referenced by vdev->device. So we break the link
between vdev->device to devargs_list by clone, and the copy
one will be freed by vdev bus itself.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/bus/vdev/vdev.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 9c66bdc78..2e2b6dc57 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -176,6 +176,21 @@ find_vdev(const char *name)
 }
 
 static struct rte_devargs *
+clone_devargs(struct rte_devargs *devargs)
+{
+	struct rte_devargs *da;
+
+	da = calloc(1, sizeof(*devargs));
+	if (da == NULL)
+		return NULL;
+
+	*da = *devargs;
+	da->args = strdup(devargs->args);
+
+	return da;
+}
+
+static struct rte_devargs *
 alloc_devargs(const char *name, const char *args)
 {
 	struct rte_devargs *devargs;
@@ -208,6 +223,7 @@ insert_vdev(const char *name, const char *args,
 {
 	struct rte_vdev_device *dev;
 	struct rte_devargs *devargs;
+	struct rte_devargs *devargs2;
 	int ret;
 
 	if (name == NULL)
@@ -217,6 +233,13 @@ insert_vdev(const char *name, const char *args,
 	if (!devargs)
 		return -ENOMEM;
 
+	devargs2 = clone_devargs(devargs);
+	if (!devargs2) {
+		free(devargs->args);
+		free(devargs);
+		return -ENOMEM;
+	}
+
 	dev = calloc(1, sizeof(*dev));
 	if (!dev) {
 		ret = -ENOMEM;
@@ -224,9 +247,9 @@ insert_vdev(const char *name, const char *args,
 	}
 
 	dev->device.bus = &rte_vdev_bus;
-	dev->device.devargs = devargs;
+	dev->device.devargs = devargs2;
 	dev->device.numa_node = SOCKET_ID_ANY;
-	dev->device.name = devargs->name;
+	dev->device.name = devargs2->name;
 
 	if (find_vdev(name)) {
 		/*
@@ -249,6 +272,8 @@ insert_vdev(const char *name, const char *args,
 fail:
 	free(devargs->args);
 	free(devargs);
+	free(devargs2->args);
+	free(devargs2);
 	free(dev);
 	return ret;
 }
@@ -269,6 +294,8 @@ rte_vdev_init(const char *name, const char *args)
 			/* If fails, remove it from vdev list */
 			TAILQ_REMOVE(&vdev_device_list, dev, next);
 			rte_devargs_remove(dev->device.devargs);
+			free(dev->device.devargs->args);
+			free(dev->device.devargs);
 			free(dev);
 		}
 	}
@@ -315,6 +342,8 @@ rte_vdev_uninit(const char *name)
 
 	TAILQ_REMOVE(&vdev_device_list, dev, next);
 	rte_devargs_remove(dev->device.devargs);
+	free(dev->device.devargs->args);
+	free(dev->device.devargs);
 	free(dev);
 
 unlock:
@@ -404,6 +433,7 @@ vdev_scan(void)
 {
 	struct rte_vdev_device *dev;
 	struct rte_devargs *devargs;
+	struct rte_devargs *devargs2;
 	struct vdev_custom_scan *custom_scan;
 
 	if (rte_mp_action_register(VDEV_MP_KEY, vdev_action) < 0 &&
@@ -457,18 +487,24 @@ vdev_scan(void)
 		if (!dev)
 			return -1;
 
+		devargs2 = clone_devargs(devargs);
+		if (!devargs2) {
+			free(dev);
+			return -1;
+		}
 		rte_spinlock_recursive_lock(&vdev_device_list_lock);
 
 		if (find_vdev(devargs->name)) {
 			rte_spinlock_recursive_unlock(&vdev_device_list_lock);
+			free(devargs2);
 			free(dev);
 			continue;
 		}
 
 		dev->device.bus = &rte_vdev_bus;
-		dev->device.devargs = devargs;
+		dev->device.devargs = devargs2;
 		dev->device.numa_node = SOCKET_ID_ANY;
-		dev->device.name = devargs->name;
+		dev->device.name = devargs2->name;
 
 		TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
 
-- 
2.13.6

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Gaëtan Rivet]

Hi,

On Tue, Nov 06, 2018 at 08:31:50AM +0800, Qi Zhang wrote:
> When probe the same device at second time, devargs will be
> replaced in devargs_list, old version is destoried but they
> are still referenced by vdev->device. So we break the link
> between vdev->device to devargs_list by clone, and the copy
> one will be freed by vdev bus itself.
> 

Please don't add even more cruft that will need to be cleaned from vdev
anyway.

This subject was already discussed[1], the solution is not to add
a devargs-dependent function clone that you did not declare part of the
devargs API to simplify its inclusion, but still relies on the devargs
structure and operation (meaning the same maintenance cost, only
deported to vdev and that future devargs work will need to follow).

You see a feature as such missing, propose a new experimental API in
rte_devargs, it will be cleaner and simpler, and might also be useful
to others.

[1]: https://mails.dpdk.org/archives/dev/2018-November/118274.html

> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  drivers/bus/vdev/vdev.c | 44 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> index 9c66bdc78..2e2b6dc57 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -176,6 +176,21 @@ find_vdev(const char *name)
>  }
>  
>  static struct rte_devargs *
> +clone_devargs(struct rte_devargs *devargs)
> +{
> +	struct rte_devargs *da;
> +
> +	da = calloc(1, sizeof(*devargs));
> +	if (da == NULL)
> +		return NULL;
> +
> +	*da = *devargs;
> +	da->args = strdup(devargs->args);
> +
> +	return da;
> +}
> +
> +static struct rte_devargs *
>  alloc_devargs(const char *name, const char *args)
>  {
>  	struct rte_devargs *devargs;
> @@ -208,6 +223,7 @@ insert_vdev(const char *name, const char *args,
>  {
>  	struct rte_vdev_device *dev;
>  	struct rte_devargs *devargs;
> +	struct rte_devargs *devargs2;
>  	int ret;
>  
>  	if (name == NULL)
> @@ -217,6 +233,13 @@ insert_vdev(const char *name, const char *args,
>  	if (!devargs)
>  		return -ENOMEM;
>  
> +	devargs2 = clone_devargs(devargs);
> +	if (!devargs2) {
> +		free(devargs->args);
> +		free(devargs);
> +		return -ENOMEM;
> +	}
> +
>  	dev = calloc(1, sizeof(*dev));
>  	if (!dev) {
>  		ret = -ENOMEM;
> @@ -224,9 +247,9 @@ insert_vdev(const char *name, const char *args,
>  	}
>  
>  	dev->device.bus = &rte_vdev_bus;
> -	dev->device.devargs = devargs;
> +	dev->device.devargs = devargs2;
>  	dev->device.numa_node = SOCKET_ID_ANY;
> -	dev->device.name = devargs->name;
> +	dev->device.name = devargs2->name;
>  
>  	if (find_vdev(name)) {
>  		/*
> @@ -249,6 +272,8 @@ insert_vdev(const char *name, const char *args,
>  fail:
>  	free(devargs->args);
>  	free(devargs);
> +	free(devargs2->args);
> +	free(devargs2);
>  	free(dev);
>  	return ret;
>  }
> @@ -269,6 +294,8 @@ rte_vdev_init(const char *name, const char *args)
>  			/* If fails, remove it from vdev list */
>  			TAILQ_REMOVE(&vdev_device_list, dev, next);
>  			rte_devargs_remove(dev->device.devargs);
> +			free(dev->device.devargs->args);
> +			free(dev->device.devargs);
>  			free(dev);
>  		}
>  	}
> @@ -315,6 +342,8 @@ rte_vdev_uninit(const char *name)
>  
>  	TAILQ_REMOVE(&vdev_device_list, dev, next);
>  	rte_devargs_remove(dev->device.devargs);
> +	free(dev->device.devargs->args);
> +	free(dev->device.devargs);
>  	free(dev);
>  
>  unlock:
> @@ -404,6 +433,7 @@ vdev_scan(void)
>  {
>  	struct rte_vdev_device *dev;
>  	struct rte_devargs *devargs;
> +	struct rte_devargs *devargs2;
>  	struct vdev_custom_scan *custom_scan;
>  
>  	if (rte_mp_action_register(VDEV_MP_KEY, vdev_action) < 0 &&
> @@ -457,18 +487,24 @@ vdev_scan(void)
>  		if (!dev)
>  			return -1;
>  
> +		devargs2 = clone_devargs(devargs);
> +		if (!devargs2) {
> +			free(dev);
> +			return -1;
> +		}
>  		rte_spinlock_recursive_lock(&vdev_device_list_lock);
>  
>  		if (find_vdev(devargs->name)) {
>  			rte_spinlock_recursive_unlock(&vdev_device_list_lock);
> +			free(devargs2);
>  			free(dev);
>  			continue;
>  		}
>  
>  		dev->device.bus = &rte_vdev_bus;
> -		dev->device.devargs = devargs;
> +		dev->device.devargs = devargs2;
>  		dev->device.numa_node = SOCKET_ID_ANY;
> -		dev->device.name = devargs->name;
> +		dev->device.name = devargs2->name;
>  
>  		TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
>  
> -- 
> 2.13.6
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Thomas Monjalon]

Hi,

06/11/2018 01:31, Qi Zhang:
> When probe the same device at second time

Sorry I stop on this first sentence.
How and why do you probe a vdev twice?

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Zhang, Qi Z]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, November 6, 2018 2:01 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; gaetan.rivet@6wind.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice
> 
> Hi,
> 
> 06/11/2018 01:31, Qi Zhang:
> > When probe the same device at second time
> 
> Sorry I stop on this first sentence.
> How and why do you probe a vdev twice?
> 
> 

if we do rte_dev_hotplug_add or rte_dev_proble on a probed device. (yes, this is not usually what an application want, but it can happen by miss-operation, and this is covered by our test case, it make sense to me that hotplug API should be robust enough to handle that situation.)
we will failed at the second time as expected, 
but will not able to detach the device any more, since during the second scan, original vdev->device.devargs is corrupted.

Btw as Gaetan mentioned below proposal
https://mails.dpdk.org/archives/dev/2018-November/118274.html

is that will be covered in 18.11, I think it's better to have a fix for this issue in 18.11 anyway. 
even the proposed idea is not able be covered in this release, we can roll back the word around fix when its ready.

What do you think?

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Thomas Monjalon]

06/11/2018 16:46, Zhang, Qi Z:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 
> > Hi,
> > 
> > 06/11/2018 01:31, Qi Zhang:
> > > When probe the same device at second time
> > 
> > Sorry I stop on this first sentence.
> > How and why do you probe a vdev twice?
> 
> if we do rte_dev_hotplug_add or rte_dev_proble on a probed device. (yes, this is not usually what an application want, but it can happen by miss-operation, and this is covered by our test case, it make sense to me that hotplug API should be robust enough to handle that situation.)

Yes I agree we must handle this situation.

> we will failed at the second time as expected, 
> but will not able to detach the device any more, since during the second scan, original vdev->device.devargs is corrupted.

The root cause is we remove a devargs which was referenced.
Could we overwrite the first devargs instead of removing it?

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Gaëtan Rivet]

On Tue, Nov 06, 2018 at 09:36:22PM +0100, Thomas Monjalon wrote:
> 06/11/2018 16:46, Zhang, Qi Z:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 
> > > Hi,
> > > 
> > > 06/11/2018 01:31, Qi Zhang:
> > > > When probe the same device at second time
> > > 
> > > Sorry I stop on this first sentence.
> > > How and why do you probe a vdev twice?
> > 
> > if we do rte_dev_hotplug_add or rte_dev_proble on a probed device. (yes, this is not usually what an application want, but it can happen by miss-operation, and this is covered by our test case, it make sense to me that hotplug API should be robust enough to handle that situation.)
> 
> Yes I agree we must handle this situation.
> 
> > we will failed at the second time as expected, 
> > but will not able to detach the device any more, since during the second scan, original vdev->device.devargs is corrupted.
> 
> The root cause is we remove a devargs which was referenced.
> Could we overwrite the first devargs instead of removing it?
> 
> 

It's also possible to add a back-reference to an rte_device in [1],
but that can only work if only one rte_device references a devargs.
It seems to be the case now, but it might be good to enforce explicitly
that when a bus scans its devices, it should do a 1-to-1 map to devargs.

If mapping rte_device to rte_devargs needs to respect rules, it could
help bus developpers to have a function that will do the job: verify that
the devargs is not currently used, add the back-reference to the
rte_device.

With the proper back-reference, it is possible to clean-up the device
when removing the devargs (and also to add the rte_devargs_extract()
function that would allow keeping the original devargs and insert it back
if the hotplug fails, then the mapping must be restored).

[1]: https://mails.dpdk.org/archives/dev/2018-November/118274.html

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Zhang, Qi Z]

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Tuesday, November 6, 2018 4:34 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice
> 
> On Tue, Nov 06, 2018 at 09:36:22PM +0100, Thomas Monjalon wrote:
> > 06/11/2018 16:46, Zhang, Qi Z:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > >
> > > > Hi,
> > > >
> > > > 06/11/2018 01:31, Qi Zhang:
> > > > > When probe the same device at second time
> > > >
> > > > Sorry I stop on this first sentence.
> > > > How and why do you probe a vdev twice?
> > >
> > > if we do rte_dev_hotplug_add or rte_dev_proble on a probed device.
> > > (yes, this is not usually what an application want, but it can
> > > happen by miss-operation, and this is covered by our test case, it
> > > make sense to me that hotplug API should be robust enough to handle
> > > that situation.)
> >
> > Yes I agree we must handle this situation.
> >
> > > we will failed at the second time as expected, but will not able to
> > > detach the device any more, since during the second scan, original
> vdev->device.devargs is corrupted.
> >
> > The root cause is we remove a devargs which was referenced.
> > Could we overwrite the first devargs instead of removing it?
> >
> >
> 
> It's also possible to add a back-reference to an rte_device in [1], but that can
> only work if only one rte_device references a devargs.
> It seems to be the case now, but it might be good to enforce explicitly that
> when a bus scans its devices, it should do a 1-to-1 map to devargs.
> 
> If mapping rte_device to rte_devargs needs to respect rules, it could help bus
> developpers to have a function that will do the job: verify that the devargs is
> not currently used, add the back-reference to the rte_device.
> 
> With the proper back-reference, it is possible to clean-up the device when
> removing the devargs 

This may still not work for vdev, since the old reference is used in vdev_find to find a exist device by name during scan.
(For PCI device, we have pci_addr, but vdev we use devargs->name to identify device, anyway this can be fixed in vdev, but that required a clone on the device name also break the coupling somehow.)
I just don't understand "why we must tight the tighten the device -> devargs coupling, not loosen it"

(and also to add the rte_devargs_extract() function
> that would allow keeping the original devargs and insert it back if the hotplug
> fails, then the mapping must be restored).

> 
> [1]: https://mails.dpdk.org/archives/dev/2018-November/118274.html
> 
> --
> Gaëtan Rivet
> 6WIND

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Gaëtan Rivet]

On Wed, Nov 07, 2018 at 04:53:50PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Tuesday, November 6, 2018 4:34 PM
> > To: Thomas Monjalon <thomas@monjalon.net>
> > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yigit, Ferruh
> > <ferruh.yigit@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice
> > 
> > On Tue, Nov 06, 2018 at 09:36:22PM +0100, Thomas Monjalon wrote:
> > > 06/11/2018 16:46, Zhang, Qi Z:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > >
> > > > > Hi,
> > > > >
> > > > > 06/11/2018 01:31, Qi Zhang:
> > > > > > When probe the same device at second time
> > > > >
> > > > > Sorry I stop on this first sentence.
> > > > > How and why do you probe a vdev twice?
> > > >
> > > > if we do rte_dev_hotplug_add or rte_dev_proble on a probed device.
> > > > (yes, this is not usually what an application want, but it can
> > > > happen by miss-operation, and this is covered by our test case, it
> > > > make sense to me that hotplug API should be robust enough to handle
> > > > that situation.)
> > >
> > > Yes I agree we must handle this situation.
> > >
> > > > we will failed at the second time as expected, but will not able to
> > > > detach the device any more, since during the second scan, original
> > vdev->device.devargs is corrupted.
> > >
> > > The root cause is we remove a devargs which was referenced.
> > > Could we overwrite the first devargs instead of removing it?
> > >
> > >
> > 
> > It's also possible to add a back-reference to an rte_device in [1], but that can
> > only work if only one rte_device references a devargs.
> > It seems to be the case now, but it might be good to enforce explicitly that
> > when a bus scans its devices, it should do a 1-to-1 map to devargs.
> > 
> > If mapping rte_device to rte_devargs needs to respect rules, it could help bus
> > developpers to have a function that will do the job: verify that the devargs is
> > not currently used, add the back-reference to the rte_device.
> > 
> > With the proper back-reference, it is possible to clean-up the device when
> > removing the devargs 
> 
> This may still not work for vdev, since the old reference is used in vdev_find to find a exist device by name during scan.
> (For PCI device, we have pci_addr, but vdev we use devargs->name to identify device, anyway this can be fixed in vdev, but that required a clone on the device name also break the coupling somehow.)

A bus should keep device identifiers within a device, without relying on
objects belonging to the EAL.

> I just don't understand "why we must tight the tighten the device -> devargs coupling, not loosen it"
> 

My point is that we are seemingly having problems with loose pointers,
broken mappings, memory leaks. So managing seems already too
complicated. Adding clones and copies will only make it more difficult
to get right.

It seems we have identified in this thread problematic behaviors from
developpers, instead of giving them more tools to shoot feet we can
instead give helpers to do what they are trying to do, but properly.

The end-goal is not to have several devargs lying around, copies of each
other, it is to avoid breaking devargs references.

> (and also to add the rte_devargs_extract() function
> > that would allow keeping the original devargs and insert it back if the hotplug
> > fails, then the mapping must be restored).
> 
> > 
> > [1]: https://mails.dpdk.org/archives/dev/2018-November/118274.html
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Zhang, Qi Z]

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, November 7, 2018 10:15 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice
> 
> On Wed, Nov 07, 2018 at 04:53:50PM +0000, Zhang, Qi Z wrote:
> >
> >
> > > -----Original Message-----
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > Sent: Tuesday, November 6, 2018 4:34 PM
> > > To: Thomas Monjalon <thomas@monjalon.net>
> > > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yigit, Ferruh
> > > <ferruh.yigit@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device
> > > twice
> > >
> > > On Tue, Nov 06, 2018 at 09:36:22PM +0100, Thomas Monjalon wrote:
> > > > 06/11/2018 16:46, Zhang, Qi Z:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > 06/11/2018 01:31, Qi Zhang:
> > > > > > > When probe the same device at second time
> > > > > >
> > > > > > Sorry I stop on this first sentence.
> > > > > > How and why do you probe a vdev twice?
> > > > >
> > > > > if we do rte_dev_hotplug_add or rte_dev_proble on a probed device.
> > > > > (yes, this is not usually what an application want, but it can
> > > > > happen by miss-operation, and this is covered by our test case,
> > > > > it make sense to me that hotplug API should be robust enough to
> > > > > handle that situation.)
> > > >
> > > > Yes I agree we must handle this situation.
> > > >
> > > > > we will failed at the second time as expected, but will not able
> > > > > to detach the device any more, since during the second scan,
> > > > > original
> > > vdev->device.devargs is corrupted.
> > > >
> > > > The root cause is we remove a devargs which was referenced.
> > > > Could we overwrite the first devargs instead of removing it?
> > > >
> > > >
> > >
> > > It's also possible to add a back-reference to an rte_device in [1],
> > > but that can only work if only one rte_device references a devargs.
> > > It seems to be the case now, but it might be good to enforce
> > > explicitly that when a bus scans its devices, it should do a 1-to-1 map to
> devargs.
> > >
> > > If mapping rte_device to rte_devargs needs to respect rules, it
> > > could help bus developpers to have a function that will do the job:
> > > verify that the devargs is not currently used, add the back-reference to
> the rte_device.
> > >
> > > With the proper back-reference, it is possible to clean-up the
> > > device when removing the devargs
> >
> > This may still not work for vdev, since the old reference is used in vdev_find
> to find a exist device by name during scan.
> > (For PCI device, we have pci_addr, but vdev we use devargs->name to
> > identify device, anyway this can be fixed in vdev, but that required a
> > clone on the device name also break the coupling somehow.)
> 
> A bus should keep device identifiers within a device, without relying on
> objects belonging to the EAL.
> 
> > I just don't understand "why we must tight the tighten the device ->
> devargs coupling, not loosen it"
> >
> 
> My point is that we are seemingly having problems with loose pointers,
> broken mappings, memory leaks. So managing seems already too
> complicated. Adding clones and copies will only make it more difficult to get
> right.

Clone is not a problem if they are encapsulated well, what we need here is some API like
rte_dev_set_devargs/rte_dev_clear_devargs, and developer just need to remember to use them but not assign devargs directly. 

The point here is remove an item in devargs should not destroy the content in rte_device at the same time (it happens on vdev and I didn't see a fix base on exist proposal), I have no objection for other way to fix this, but clone is the only way I can figure out right now.

> 
> It seems we have identified in this thread problematic behaviors from
> developpers, instead of giving them more tools to shoot feet we can instead
> give helpers to do what they are trying to do, but properly.
> 
> The end-goal is not to have several devargs lying around, copies of each
> other, it is to avoid breaking devargs references.
> 
> > (and also to add the rte_devargs_extract() function
> > > that would allow keeping the original devargs and insert it back if
> > > the hotplug fails, then the mapping must be restored).
> >
> > >
> > > [1]: https://mails.dpdk.org/archives/dev/2018-November/118274.html
> > >
> > > --
> > > Gaëtan Rivet
> > > 6WIND
> 
> --
> Gaëtan Rivet
> 6WIND

Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Thomas Monjalon]

07/11/2018 18:46, Zhang, Qi Z:
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > On Wed, Nov 07, 2018 at 04:53:50PM +0000, Zhang, Qi Z wrote:
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > > On Tue, Nov 06, 2018 at 09:36:22PM +0100, Thomas Monjalon wrote:
> > > > > 06/11/2018 16:46, Zhang, Qi Z:
> > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > 06/11/2018 01:31, Qi Zhang:
> > > > > > > > When probe the same device at second time
> > > > > > >
> > > > > > > Sorry I stop on this first sentence.
> > > > > > > How and why do you probe a vdev twice?
> > > > > >
> > > > > > if we do rte_dev_hotplug_add or rte_dev_proble on a probed device.
> > > > > > (yes, this is not usually what an application want, but it can
> > > > > > happen by miss-operation, and this is covered by our test case,
> > > > > > it make sense to me that hotplug API should be robust enough to
> > > > > > handle that situation.)
> > > > >
> > > > > Yes I agree we must handle this situation.
> > > > >
> > > > > > we will failed at the second time as expected, but will not able
> > > > > > to detach the device any more, since during the second scan,
> > > > > > original
> > > > vdev->device.devargs is corrupted.
> > > > >
> > > > > The root cause is we remove a devargs which was referenced.
> > > > > Could we overwrite the first devargs instead of removing it?
> > > > >
> > > > >
> > > >
> > > > It's also possible to add a back-reference to an rte_device in [1],
> > > > but that can only work if only one rte_device references a devargs.
> > > > It seems to be the case now, but it might be good to enforce
> > > > explicitly that when a bus scans its devices, it should do a 1-to-1 map to
> > devargs.
> > > >
> > > > If mapping rte_device to rte_devargs needs to respect rules, it
> > > > could help bus developpers to have a function that will do the job:
> > > > verify that the devargs is not currently used, add the back-reference to
> > the rte_device.
> > > >
> > > > With the proper back-reference, it is possible to clean-up the
> > > > device when removing the devargs
> > >
> > > This may still not work for vdev, since the old reference is used in vdev_find
> > to find a exist device by name during scan.
> > > (For PCI device, we have pci_addr, but vdev we use devargs->name to
> > > identify device, anyway this can be fixed in vdev, but that required a
> > > clone on the device name also break the coupling somehow.)
> > 
> > A bus should keep device identifiers within a device, without relying on
> > objects belonging to the EAL.
> > 
> > > I just don't understand "why we must tight the tighten the device ->
> > devargs coupling, not loosen it"
> > >
> > 
> > My point is that we are seemingly having problems with loose pointers,
> > broken mappings, memory leaks. So managing seems already too
> > complicated. Adding clones and copies will only make it more difficult to get
> > right.
> 
> Clone is not a problem if they are encapsulated well, what we need here is some API like
> rte_dev_set_devargs/rte_dev_clear_devargs, and developer just need to remember to use them but not assign devargs directly. 
> 
> The point here is remove an item in devargs should not destroy the content in rte_device at the same time (it happens on vdev and I didn't see a fix base on exist proposal), I have no objection for other way to fix this, but clone is the only way I can figure out right now.
> 
> > 
> > It seems we have identified in this thread problematic behaviors from
> > developpers, instead of giving them more tools to shoot feet we can instead
> > give helpers to do what they are trying to do, but properly.
> > 
> > The end-goal is not to have several devargs lying around, copies of each
> > other, it is to avoid breaking devargs references.
> > 
> > > (and also to add the rte_devargs_extract() function
> > > > that would allow keeping the original devargs and insert it back if
> > > > the hotplug fails, then the mapping must be restored).
> > >
> > > >
> > > > [1]: https://mails.dpdk.org/archives/dev/2018-November/118274.html

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