As you may notice, I have included a slightly modified version
of this patch in my series in order to cover the full picture:
https://patches.dpdk.org/patch/77566/
Feel free to continue improving your patch in this thread or the other,
as you prefer, as long as the secondary process issue is adressed.
Thanks
13/09/2020 11:16, Thomas Monjalon:
> 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;
