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

Zhang, Qi Z qi.z.zhang at intel.com
Thu Jul 5 03:38:49 CEST 2018



> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net]
> Sent: Thursday, July 5, 2018 5:42 AM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>
> Cc: dev at dpdk.org; Burakov, Anatoly <anatoly.burakov at intel.com>; Ananyev,
> Konstantin <konstantin.ananyev at intel.com>; Richardson, Bruce
> <bruce.richardson at intel.com>; Yigit, Ferruh <ferruh.yigit at intel.com>; Shelton,
> Benjamin H <benjamin.h.shelton at intel.com>; Vangati, Narender
> <narender.vangati at intel.com>; arybchenko at solarflare.com
> Subject: Re: [dpdk-dev] [PATCH v8 04/19] ethdev: introduce device lock
> 
> 04/07/2018 12:49, Zhang, Qi Z:
> > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > 04/07/2018 03:47, Zhang, Qi Z:
> > > > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > > > 03/07/2018 17:08, Zhang, Qi Z:
> > > > > > From: Thomas Monjalon [mailto:thomas at 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.
Also we should not put it into rte_eth_dev_stop, because, rte_eth_dev_stop can invoked by application directly, in that case, we don't what any callback be invoked.

> 
> > 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.

Currently PMD implement driver->remove with different way, rte_eth_dev_stop / rte_eth_dev_close / rte_eth_dev_destroy is not always be invoked.
So Before we standardize what ethdev API and what sequence should be called in driver->remove (I think this is a separate task)
I will suggest 
1. Create another help function like _rte_eth_dev_allow_to_remove, 
2. the help function will call _rte_eth_dev_callback_process(RTE_ETH_EVENT_PRE_REMOVE) and update ret_param which contain a reject count.
3. the help function should to invoked at beginning at driver->remove and driver->remove will abort if the help function failed.

But once we standardized that , we can do cleanup to merge it into another rte_eth_xxx API in next step.

What do you think?

> 
> 



More information about the dev mailing list