Re: [dpdk-dev] [PATCH v8 04/19] ethdev: introduce device lock

[Thomas Monjalon <thomas@monjalon.net>]

04/07/2018 12:49, Zhang, Qi Z:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 04/07/2018 03:47, Zhang, Qi Z:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 03/07/2018 17:08, Zhang, Qi Z:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > 02/07/2018 07:44, Qi Zhang:
> > > > > > > Introduce API rte_eth_dev_lock and rte_eth_dev_unlock to let
> > > > > > > application lock or unlock on specific ethdev, a locked device
> > > > > > > can't be detached, this help applicaiton to prevent unexpected
> > > > > > > device detaching, especially in multi-process envrionment.
> > > > > >
> > > > > > Trying to understand: a process of an application could try to
> > > > > > detach a port while another process is against this decision.
> > > > > > Why an application needs to be protected against itself?
> > > > >
> > > > > I think we can regard this as a help function, it help application
> > > > > to simplified
> > > > the situation when one process want to detach a device while another
> > > > one is still using it.
> > > > > Application can register a callback which can do to necessary
> > > > > clean up (like
> > > > stop traffic, release memory ...) before device be detached.
> > > >
> > > > Yes I agree such hook can be a good idea.
[...]
> > > > After all, it is just a pre-detach hook.
> > >
> > > > Wait, how is it different of RTE_ETH_EVENT_DESTROY callback?
> > > > Perhaps we just need to improve the handling of the DESTROY event?
> > >
> > > I have thought about this before.
> > > Not like RTE_ETH_EVENT_DESTROY and other event hook, the hook here
> > need to give feedback, pass or fail will impact the following behavior, this
> > make it special, so I separate it from all exist rte_eth_event_type handle
> > mechanism.
> > 
> > Look at _rte_eth_dev_callback_process, there is a "ret_param".
> 
> OK, that should work.
> > 
> > > The alternative solution is
> > > we just introduce a new event type like RTE_ETH_EVENT_PRE_DETACH and
> > > reuse all exist API
> > rte_eth_dev_callback_register/rte_eth_dev_callback_unregister.
> > 
> > I don't think we need a new event.
> > Let's try to use RTE_ETH_EVENT_DESTROY.
> 
> The problem is RTE_ETH_EVENT_DESTROY is used in rte_eth_dev_release_port already.
> And in PMD, rte_eth_dev_release_port is called after dev_uninit, that mean its too late to reject a detach

You're right.

It's a real mess currently.
The right order should be to remove ethdev ports before
removing the underlying EAL device. But it's strangely not the case.

We need to separate things.
The function rte_eth_dev_close can be used to remove an ethdev port
if we add a call to rte_eth_dev_release_port.
So we could call rte_eth_dev_close in PMD remove functions.
Is "close" a good time to ask confirmation to the application?
Or should we ask confirmation a step before, on "stop"?

> So , do you mean we can remove _rte_eth_dev_callback_process(RTE_ETH_EVENT_DESTROPY) in rte_eth_dev_release_port 

I would say we need RTE_ETH_EVENT_DESTROY to notify that the port
is really destroyed.
Maybe the right thing to do is to add a new event
RTE_ETH_EVENT_CLOSE_REQUEST or something else.
Note that we already have 2 removal events in ethdev:
	- RTE_ETH_EVENT_INTR_RMV when the port cannot be used anymore
	- RTE_ETH_EVENT_DESTROY when the port is going to be deleted

> And where is right place to call _rte_eth_dev_callback_process(RTE_ETH_EVENT_DESTROY)?
> If can't be called in rte_eth_dev_detach, because if device is removed by rte_eal_hotplug_remove, it will be skipped.

No, rte_eth_dev_detach and rte_eal_hotplug_remove are 2 different things.
One is a mix of ethdev and EAL (and should be deprecated),
the other one is for the underlying device at EAL level.

> probably we need to call this at the beginning of each PMD driver->remove?, that means, we need to change all PMD drivers?

Yes, we can call rte_eth_dev_stop and rte_eth_dev_close
at the beginning of PMD remove.
Note that there is already a helper rte_eth_dev_destroy called in
some PMD to achieve the removal, but curiously, it doesn't call
stop and close functions.