> -----Original Message----- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Sunday, September 13, 2020 5:17 PM > To: wangyunjian <wangyunj...@huawei.com> > Cc: dev@dpdk.org; ferruh.yi...@intel.com; Lilijun (Jerry) > <jerry.lili...@huawei.com>; xudingke <xudin...@huawei.com>; > keith.wi...@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; > >
