> -----Original Message-----
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Thursday, July 5, 2018 6:55 PM
> To: Zhang, Qi Z <qi.z.zh...@intel.com>
> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.bura...@intel.com>; Ananyev,
> Konstantin <konstantin.anan...@intel.com>; Richardson, Bruce
> <bruce.richard...@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com>; Shelton,
> Benjamin H <benjamin.h.shel...@intel.com>; Vangati, Narender
> <narender.vang...@intel.com>; arybche...@solarflare.com
> Subject: Re: [dpdk-dev] [PATCH v8 04/19] ethdev: introduce device lock
>
> 05/07/2018 11:54, Zhang, Qi Z:
> > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > > 05/07/2018 05:37, Zhang, Qi Z:
> > > > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > > > > 05/07/2018 03:38, Zhang, Qi Z:
> > > > > > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > > > > > > 04/07/2018 12:49, Zhang, Qi Z:
> > > > > > > > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > > > > > > > > 04/07/2018 03:47, Zhang, Qi Z:
> > > > > > > > > > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > > > > > > > > > > 03/07/2018 17:08, Zhang, Qi Z:
> > > > > > > > > > > > From: Thomas Monjalon [mailto:tho...@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"?
> > > > > >
> > > > > > I think the confirmation should before any cleanup stage, it
> > > > > > should at the
> > > > > beginning of driver->remove.
> > > > >
> > > > > So you stop a port, even if the app policy is against detaching it?
> > > >
> > > > My understanding is, stop and detach is different, we may stop a
> > > > device and
> > > reconfigure it then restart it.
> > > > but for detach, properly we will not use it, unless it be probed again.
> > > > For dev_close , it should be called after dev_stop.
> > > > so we have to like below.
> > > >
> > > > If (dev->started) {
> > > > dev_stop /* but still problem here, if traffic is ongoing */
> > > > if (dev_close()) {
> > > > dev_start()
> > > > return -EBUSY.
> > > > }
> > > > } else {
> > > > If (dev_close())
> > > > Return _EBUSY
> > > > }
> > > >
> > > > So for me, neither rte_eth_dev_stop and rte_eth_dev_close is the
> > > > right place
> > > to check this.
> > > > But rte_eth_dev_destroy looks like a good one. We can put all the
> > > > ethdev general logic into it, and PMD specific dev_unit will be
> > > > called at last
> > >
> > > If you want to detach a port, you need to stop it.
> > > If one process try to detach a port, but another process decides
> > > (via callback) that the port should not be detached, you will have
> > > stopped a port for no good reason.
> > > To me it is a real design issue.
> >
> > Yes, so I think we still need two iterates for detach.
> > First iterate to get agreement on all processes.
> > Secondary iterate to do the detach.
> >
> > But how?
>
> An option is to let the application manages itself its process synchronization
> and authorization for detaching devices.
Yes, I agree
Before we figure out a comprehensive solution, leave this to application's
handle so far.
I will remove this one from patchset.
>