Re: [dpdk-dev] [PATCH v2 02/10] kni: separate releasing netdev from freeing KNI interface

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/10/2018 7:18 PM, Dan Gora wrote:
> On Wed, Oct 10, 2018 at 2:25 PM Ferruh Yigit <ferruh.yigit@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
>