> -----Original Message-----
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Wednesday, July 4, 2018 3:27 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>
> Subject: Re: [dpdk-dev] [PATCH v8 04/19] ethdev: introduce device lock
>
> 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.
> > >
> > >
> > > > > I guess it is only an application inter-process management.
> > > > > If we really want to provide such helper in DPDK, it should not
> > > > > be limited to ethdev.
> > > >
> > > > Once we move to eal layer, we will have
> > > > rte_eal_dev_lock/unlock(devname,
> > > busname).
> > > > But its also better we keep rte_eth_dev_lock/unlock to make ethdev
> > > > API more completed (any port be locked means underline rte_device
> > > > also be
> > > locked?) and this is same for other device family.
> > >
> > > No. Again, a port is not exactly a device.
> > > There can be several ports per device.
> >
> > Yes, I know that.
> > what I mean is, we should assume lock any port of that rte_device will
> prevent the device be detached.
> >
> > >
> > > I think the right place for this hook is in port-level API (ethdev,
> > > crypto, etc). And as we improve only ethdev currently, without any
> > > common genericity for other device classes, it is probably fine in ethdev
> > > for
> now.
> > > >
> > > > > (for info, see class implementation:
> > > > > https://patches.dpdk.org/patch/41605/)
> > > > >
> > > > > What about hardware unplug?
> > > > > Can we detach the locked ports associated to the unplugged device?
> > > >
> > > > NO, we can't.
> > > > But do you think, we need to introduce a "force detach" version,
> > > > which will
> > > ignore all locks.
> > >
> > > No, I don't think so.
> > > I am just trying to show that you cannot really "lock" a port.
> > > Maybe you should just rename those functions.
> > > 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
So , do you mean we can remove
_rte_eth_dev_callback_process(RTE_ETH_EVENT_DESTROPY) in
rte_eth_dev_release_port
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.
probably we need to call this at the beginning of each PMD driver->remove?,
that means, we need to change all PMD drivers?
>
> > But in _rte_eth_dev_callback_process we need to add a code branch for
> PRE_DETACH handle.
> >
> > If (event = RTE_ETH_EVENT_PRE_DETACH)
> > <...>.
> > else {
> > <....>
> > }
> >
> > And we may also need to keep rte_eth_dev_lock/unlock which will register a
> default callback for PRE_DETACH.
>
> The default callback can be registered by the application.
OK.
>
> > What do you think about?
>
>
