On 2015/03/16 23:47, Iremonger, Bernard wrote: > >> -----Original Message----- >> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] >> Sent: Monday, March 16, 2015 8:57 AM >> To: Iremonger, Bernard >> Cc: John W. Linville; dev at dpdk.org >> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug >> >>>>>>> @@ -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. >>> Hi Bernard, >>> >>> Sorry for my English. >>> I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs. >>> Not a PMDs for virtual functions on NIC. >>> >>> For PMDs like a pcap and af_packet PMDs, all data structures are >>> allocated per processes. >>> (Actually I guess nothing is shared between primary and secondary >>> processes, because rte_eth_dev_data is overwritten by each processes.) >>> So we need to free per process data when detach() is called. >>> >>>> 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. >>> Because of above, we probably not be able to add PROC_PRIMARY_OR_RET() >>> to rte_eth_dev_detach(). >>> But I agree we should not call rte_eth_dev_detach() for secondary >>> process, if PMDs are like e1000 or ixgbe PMD. >> Correction: >> We should not process rte_eth_dev_detach() for secondary process, if PMDs >> are like e1000 or ixgbe >> PMD and if primary process hasn't called >> stop() and close() yet. >> >> Tetsuya >> >>> To work like above, how about changing drv_flags dynamically in >>> close() callback? >>> For example, when primary process calls rte_eth_dev_close(), a >>> callback of PMD will be called. >>> (In the case of e1000 PMD, eth_em_close() is the callback.) >>> >>> At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the >>> callback. >>> It means if primary process hasn't called close() yet, >>> rte_eth_dev_detach() will do nothing and return error. >>> How about doing like above? >>> >>> Regards, >>> Tetsuya > Hi Tetsuya, > For the e1000, igb and ixgbe PMD's it is probably simpler to just check for > the primary process in the uninit functions and just return without doing > anything for secondary processes.
Thanks for clarifying. In the case, is it okay for you to add PROC_PRIMARY_OR_RET() in e1000, igb and ixgbe PMD code? If it's okay, we may be able to ACK this patch. :) Regards, Tetsuya > > Regards, > > Bernard. > > > > >
