> -----Original Message----- > From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > Sent: Tuesday, March 17, 2015 3:43 AM > To: Iremonger, Bernard > Cc: John W. Linville; dev at dpdk.org > Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug > > 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 > >>>> dev->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 >
Hi Tetsuya, I will add the process type check in the e1000, igb amd ixgbe PMD code. Regards, Bernard.
