> -----Original Message----- > From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > Sent: Friday, March 13, 2015 12:11 AM > To: Iremonger, Bernard > Cc: John W. Linville; dev at dpdk.org > Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug > > 2015-03-13 2:05 GMT+09:00 Iremonger, Bernard <bernard.iremonger at intel.com>: > > > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of John W. Linville > >> Sent: Tuesday, March 10, 2015 6:36 PM > >> To: dev at dpdk.org > >> Subject: [dpdk-dev] [RFC] af_packet: support port hotplug > >> > >> This patch adds finalization code to free resources allocated by the > >> PMD. This is based on earlier patches for other PMDs by Tetsuya > >> Mukawa <mukawa at igel.co.jp>, with corrections related to data- > >> >name. > >> > >> Signed-off-by: John W. Linville <linville at tuxdriver.com> > >> Cc: Tetsuya Mukawa <mukawa at igel.co.jp> > >> --- > >> lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56 > >> ++++++++++++++++++++++++++++ > >> 1 file changed, 56 insertions(+) > >> > >> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > >> b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > >> index 80e9bdf7f819..57998ab19c96 100644 > >> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > >> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > >> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops = { > >> .stats_reset = eth_stats_reset, }; > >> > >> +static struct eth_driver rte_af_packet_pmd = { > >> + .pci_drv = { > >> + .name = "rte_af_packet_pmd", > >> + .drv_flags = RTE_PCI_DRV_DETACHABLE, > >> + }, > >> +}; > >> + > >> /* > >> * Opens an AF_PACKET socket > >> */ > >> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name, > >> if (*eth_dev == NULL) > >> goto error; > >> > >> + /* check length of device name */ > >> + if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name)) > >> + goto error; > >> + > >> /* > >> * now put it all together > >> * - store queue data in internals, @@ -669,12 +680,14 @@ > >> rte_pmd_init_internals(const char *name, > >> data->nb_tx_queues = (uint16_t)nb_queues; > >> data->dev_link = pmd_link; > >> data->mac_addrs = &(*internals)->eth_addr; > >> + strncpy(data->name, (*eth_dev)->data->name, > >> +strlen((*eth_dev)->data->name)); > >> > >> pci_dev->numa_node = numa_node; > >> > >> (*eth_dev)->data = data; > >> (*eth_dev)->dev_ops = &ops; > >> (*eth_dev)->pci_dev = pci_dev; > >> + (*eth_dev)->driver = &rte_af_packet_pmd; > >> > >> return 0; > >> > >> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const > >> char *params) > >> return 0; > >> } > >> > >> +static int > >> +rte_pmd_af_packet_devuninit(const char *name) { > >> + struct rte_eth_dev *eth_dev = NULL; > >> + struct pmd_internals *internals; > >> + struct tpacket_req req; > >> + > >> + unsigned q; > >> + > >> + RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n", > >> + rte_socket_id()); > >> + > >> + if (name == NULL) > >> + return -1; > > > > Hi Tetsuya, John, > > > > Before detaching a port, the port must be stopped and closed. > > The stop and close are only allowed for RTE_PROC_PRIMARY. > > Should there be a check for process_type here? > > > > if (rte_eal_process_type() != RTE_PROC_PRIMARY) > > return -EPERM; > > > > Regards, > > > > Bernard > > > > Hi Bernard, > > I agree with stop() and close() are only called by primary process, but it > may not need to add like > above. > Could you please check rte_ethdev.c? > > - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared between > processes. > So we need to initialize of finalize carefully like you said. > > - struct rte_eth_dev rte_eth_devices[] > This array is per process. > And 'data' variable of this structure indicates a pointer of rte_eth_dev_data. > > All PMDs for physical NIC allocates like above when PMDs are initialized. > (Even when a process is secondary, initialization function of PMDs will be > called) But virtual device > PMDs allocate rte_eth_dev_data and overwrite 'data' > variable of rte_eth_devices while initialization. > > As a result, primary and secondary process has their own 'rte_eth_dev_data' > for a virtual device. > So I guess all processes need to free it not to leak memory. > > Thanks, > Tetsuya > Hi Tetsuya,
In rte_ethdev.c both rte_eth_dev_stop() and rte_eth_dev_close() use the macro PROC_PRIMARY_OR_RET(). So for secondary processes both functions return without doing anything. Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ? For the Physical/Virtual Functions of the NIC a lot of the finalization is done in the dev->dev_ops->dev_stop() and dev->dev_ops->dev_close() functions. To complete the finalization the dev_uninit() function is called, this should probably do nothing for secondary processes as the dev_stop() and dev_close() functions will not have been executed. For the Physical/Virtual Functions of the NIC the dev_init() is called for both primary and secondary processes, however a subset of the function only is executed for secondary processes. Regards, Bernard.