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

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

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