On 10/10/2018 7:18 PM, Dan Gora wrote: > On Wed, Oct 10, 2018 at 2:25 PM Ferruh Yigit <ferruh.yi...@intel.com> wrote: >> >> On 9/4/2018 1:36 AM, Dan Gora wrote: >>> Hi Ferruh, >>> >>> I remembered now the motivation behind separating rte_kni_release() >>> and rte_kni_free(). >>> >>> The problem is that the DPDK thread which calls rte_kni_release() >>> _cannot_ be the same thread which handles callbacks from the KNI >>> driver via rte_kni_handle_request(). This is because the thread which >>> calls rte_kni_release() will be stuck down in >>> ioctl(RTE_KNI_IOCTL_RELEASE) when the kernel calls the >>> RTE_KNI_REQ_CFG_NETWORK_IF callback to the DPDK application. Since >>> that thread cannot call rte_kni_handle_request(), the callback would >>> then just timeout unless some other thread calls >>> rte_kni_handle_request(). >>> >>> So then you are in a bit of a chicken and egg situation. You _have_ >>> to have a separate thread calling rte_kni_handle_request periodically, >>> but that thread also _cannot_ run after rte_kni_release returns >>> (actually it's worse than that because it's actually after the >>> ioctl(RTE_KNI_IOCTL_RELEASE) returns and the fifos are freed). >> >> I see, so we have problem in both end, -userspace side and kernel side. >> >> Agreed that separating release & free may help, but I am not sure about >> adding a >> new API for KNI. >> >> Very simply, what about prevent kni_net_release() send callback to userspace? > > No, because how is the DPDK application going to know when the user > does 'ip link set down dev <kniX>'? It's important for the DPDK > application to know when the KNI interface is marked down.
I mean kni_net_release() called because of unregister_netdev(), it is possible to set a flag in kni_dev_remove(), before unregister_netdev(), and in kni_net_release() don't call kni_net_process_request() if flag is set. Looks like it can work and only a few lines of code, what do you think? > >> This is already not working and removing it resolves the issues you >> mentioned. > > Huh? How is it not working? Of course it works. The kni_net_release() called because of unregister_netdev() is not working, as you explained in userspace the thread handles request already terminated, even if not in kernel side response not received and timed off because of lock... > >> Sample application calls rte_eth_dev_stop() after release itself, so behavior >> will be same. > > Huh? in kni sample app, in kni_free_kni() rte_eth_dev_stop() is called after rte_kni_release(). So if you prevent kni_net_release() called because of unregister_netdev() to send callback it won't be problem because of existing rte_eth_dev_stop() > >> But the issues in kernel you mentioned, using `dev` after free_netdev() >> called >> should be addressed. > > Yes, that's why I fixed them in the patches that I sent. > > d >
