Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[Thomas Monjalon <thomas@monjalon.net>]

Hi,

28/08/2020 14:37, wangyunjian:
> @@ -1078,6 +1085,23 @@ tap_dev_close(struct rte_eth_dev *dev)
> +
> +	/* mac_addrs must not be freed alone because part of dev_private */
> +	dev->data->mac_addrs = NULL;
> +
> +	internals = dev->data->dev_private;
> +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> +		tuntap_types[internals->type], rte_socket_id());
> +
> +	if (internals->ioctl_sock != -1) {
> +		close(internals->ioctl_sock);
> +		internals->ioctl_sock = -1;
> +	}
> +	rte_free(dev->process_private);
> +	dev->process_private = NULL;
> +	if (tap_devices_count == 1)
> +		rte_mp_action_unregister(TAP_MP_KEY);
> +	tap_devices_count--;
>  }
[...]
> @@ -2446,12 +2466,11 @@ static int
>  rte_pmd_tap_remove(struct rte_vdev_device *dev)
>  {
>  
>         struct rte_eth_dev *eth_dev = NULL;
> 
> -       struct pmd_internals *internals;
> 
>         /* find the ethdev entry */
>         eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
>         if (!eth_dev)
> 
> -               return -ENODEV;
> +               return 0;
> 
>         /* mac_addrs must not be freed alone because part of dev_private */
>         eth_dev->data->mac_addrs = NULL;

The line above can be removed because mac_addrs is not freed
in secondary process, and it is redundant with tap_dev_close().

>  
> 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>  		return rte_eth_dev_release_port(eth_dev);

There is an inconsistency in secondary process
if tap_dev_close() is not called from .remove (unplug)
but can be called from .dev_close (rte_eth_dev_close).

I think the process type check must be done inside tap_dev_close(),
so the process private resources can be freed.

> 	
>  	tap_dev_close(eth_dev);
>  

This blank line can be removed in my opinion.

> -	internals = eth_dev->data->dev_private;
> -	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> -		tuntap_types[internals->type], rte_socket_id());
> -
> -	close(internals->ioctl_sock);
> -	rte_free(eth_dev->process_private);
> -	if (tap_devices_count == 1)
> -		rte_mp_action_unregister(TAP_MP_KEY);
> -	tap_devices_count--;
>  	rte_eth_dev_release_port(eth_dev);
>  
>  	return 0;