Re: [PATCH 2/3] bus/vdev: revert fix devargs after multi-process bus scan

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 3/14/2024 10:36 AM, Mingjin Ye wrote:
> The asan tool detected a memory leak in the vdev driver alloc_devargs.
> The previous commit does not insert device arguments into devargs_list
> when attaching a device during a bus scan of a secondary process.
> This resulted in an existing memory leak when removing a vdev device,
> since rte_devargs_remove actually does nothing.
> 
> Therefore the following commit was reverted accordingly.
> 
> Fixes: f5b2eff0847d ("bus/vdev: fix devargs after multi-process bus scan")
> 
> Restoring this commit will fix the memory leak. There was an issue with
> device parameters using free devargs when inserting a vdev device when
> devargs_list already existed, resulting in a core dump. A new patch
> will fix this issue.
> 
> Fixes: f5b2eff0847d ("bus/vdev: fix devargs after multi-process bus scan")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---

Hi Mingjin,

I've been trying to understand what this patchset does.

>   drivers/bus/vdev/vdev.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> index 38d05a9fe9..1a6cc7d12d 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -264,9 +264,7 @@ alloc_devargs(const char *name, const char *args)
>   }
>   
>   static int
> -insert_vdev(const char *name, const char *args,
> -		struct rte_vdev_device **p_dev,
> -		bool init)
> +insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
>   {
>   	struct rte_vdev_device *dev;
>   	struct rte_devargs *devargs;
> @@ -300,8 +298,7 @@ insert_vdev(const char *name, const char *args,
>   		goto fail;
>   	}
>   
> -	if (init)
> -		rte_devargs_insert(&devargs);
> +	rte_devargs_insert(&devargs);

The original commit message states that "it is not necessary" to add 
devargs to the list in secondary processes, but does not state why it is 
unnecessary and what issue was that fix for. With that understanding, I 
think the commit message for this patch can be reworded like follows:

---

This patch reverts commit f5b2eff0847d ("bus/vdev: fix devargs after 
multi-process bus scan")

With current code, we do not add devargs to devargs list when we add a 
vdev in secondary process (because `init` flag is set to `false`).

Because of this, when we do vdev_uninit, we call rte_devargs_remove on 
the &devargs pointer (the one we didn't add to devargs list but did save 
inside rte_vdev_device struct), but in secondary process, because 
devargs were not added to the list in the first place, devargs_remove 
does not find the associated devargs in its list and therefore does not 
free associated resources. As a result, we get a memory leak, because we 
free the rte_vdev_device but not its associated devargs.

Revert this patch to avoid leaking devargs on vdev uninit.

---

I think this would be clearer summation of what is going on in this patch.

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>


>   	dev->device.devargs = devargs;
>   	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
>   
> @@ -323,7 +320,7 @@ rte_vdev_init(const char *name, const char *args)
>   	int ret;
>   
>   	rte_spinlock_recursive_lock(&vdev_device_list_lock);
> -	ret = insert_vdev(name, args, &dev, true);
> +	ret = insert_vdev(name, args, &dev);
>   	if (ret == 0) {
>   		ret = vdev_probe_all_drivers(dev);
>   		if (ret) {
> @@ -449,7 +446,7 @@ vdev_action(const struct rte_mp_msg *mp_msg, const void *peer)
>   		break;
>   	case VDEV_SCAN_ONE:
>   		VDEV_LOG(INFO, "receive vdev, %s", in->name);
> -		ret = insert_vdev(in->name, NULL, NULL, false);
> +		ret = insert_vdev(in->name, NULL, NULL);
>   		if (ret == -EEXIST)
>   			VDEV_LOG(DEBUG, "device already exist, %s", in->name);
>   		else if (ret < 0)

-- 
Thanks,
Anatoly