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

[wangyunjian <wangyunjian@huawei.com>]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Sunday, September 13, 2020 5:17 PM
> To: wangyunjian <wangyunjian@huawei.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> keith.wiles@intel.com
> Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
> 
> 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().

Thanks for the suggestion.
I have fixed it in v2.
https://patchwork.dpdk.org/patch/78048/

> 
> >
> > 	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.

OK

> 
> > -	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;
> 
>