[dpdk-dev] [PATCH 05/22] ethdev: introduce device lock

[Zhang, Qi Z]

Hi Anatoly:
	Sorry to miss this email and reply late.

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Friday, June 15, 2018 11:43 PM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>; thomas at monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org;
> 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>
> Subject: Re: [PATCH 05/22] ethdev: introduce device lock
> 
> On 07-Jun-18 1:38 PM, Qi Zhang wrote:
> > 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.
> >
> > Aslo the new API let application to register a callback function which
> > will be invoked before a device is going to be detached, the return
> > value of the function will decide if device will continue be detached
> > or not, this support application to do condition check at runtime.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang at intel.com>
> > ---
> 
> <snip>
> 
> > +
> > +int
> > +rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> > +		 void *user_args)
> > +{
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +
> > +	if (callback == NULL)
> > +		return register_lock_callback(port_id, dev_is_busy, NULL);
> > +	else
> > +		return register_lock_callback(port_id, callback, user_args);
> 
> As much as i don't like seeing negative errno values as return, the rest of
> ethdev library uses those, so this is OK :)
> 
> > +}
> > +
> > +int
> > +rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> > +		   void *user_args)
> > +{
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +
> 
> <snip>
> 
> > + * Also, any callback function return !0 value will prevent device be
> > + * detached(ref. rte_eth_dev_lock and rte_eth_dev_unlock).
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param user_args
> > + *   This is parameter "user_args" be saved when callback function is
> > + *   registered(rte_dev_eth_lock).
> > + *
> > + * @return
> > + *   0  device is allowed be detached.
> > + *   !0 device is not allowed be detached.
> 
> !0 can be negative or positive. Are we expecting positive return values from
> this API?

I have no strong opinion, but if you think below or other option is better, I can change

>=0  device is allowed be detached.
<0 device is not allowed be detached.

> 
> > + */
> > +typedef int (*rte_eth_dev_lock_callback_t)(uint16_t port_id, void
> > +*user_args);
> > +
> > +/**
> > + * Lock an Ethernet Device directly or register a callback function
> > + * for condition check at runtime, this help application to prevent
> > + * a device be detached unexpectedly 
> > + * NOTE: Lock a device multiple times with same parmeter will
> > +increase
> > + * a ref_count, and corresponding unlock decrease the ref_count, the
> > + * device will be unlocked when ref_count reach 0.
> 
> Nitpick: "note" sections should be done with @note marker.
> 
> Also, i would mention that this is a per-process lock that does not affect other
> processes (assuming i understood the code correctly, of course...).

OK, I will add more comment to explain this.

> 
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param callback
> > + *   !NULL the callback function will be added into a pre-detach list,
> > + *         it will be invoked when a device is going to be detached. The
> > + *         return value will decide if continue detach the device or not.
> > + *   NULL  lock the device directly, basically this just regiter a empty
> > + *         callback function(dev_is_busy) that return -EBUSY, so we can
> > + *         handle the pre-detach check in unified way.
> > + * @param user_args
> > + *   parameter will be parsed to callback function, only valid when
> > + *   callback != NULL.
> > + * @return
> > + *   0 on success, negative on error.
> > + */
> > +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t
> callback,
> > +		     void *user_args);
> 
> Nitpicks: DPDK style guide discourages using spaces as indentation (other parts
> of this patch, and other patches have this issue as well).

OK, will fix all.
> 
> > +
> > +/**
> > + * Reverse operation of rte_eth_dev_lock.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param callback
> > + *   NULL  decrease the ref_count of default callback function.
> > + *   !NULL decrease the ref_count of specific callback with matched
> > + *         user_args.
> > + * @param user_args
> > + *   parameter to match, only valid when callback != NULL.
> > + * @return
> > + *   0 on success, negative on error.
> > + */
> > +int rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t
> callback,
> > +		       void *user_args);
> > +
> >   #ifdef __cplusplus
> >   }
> >   #endif
> > diff --git a/lib/librte_ethdev/rte_ethdev_lock.c
> > b/lib/librte_ethdev/rte_ethdev_lock.c
> 
> rte_ethdev_lock.* seem to be internal-only files. Perhaps you should name
> them without the rte_ prefix to indicate that they're not exported?
> 
> > new file mode 100644
> > index 000000000..688d1d70a
> > --- /dev/null
> > +++ b/lib/librte_ethdev/rte_ethdev_lock.c
> > @@ -0,0 +1,102 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Intel Corporation  */ #include
> > +"rte_ethdev_lock.h"
> > +
> > +struct lock_entry {
> > +	TAILQ_ENTRY(lock_entry) next;
> > +	rte_eth_dev_lock_callback_t callback;
> > +	uint16_t port_id;
> 
> <snip>
> 
> > +register_lock_callback(uint16_t port_id,
> > +		       rte_eth_dev_lock_callback_t callback,
> > +		       void *user_args)
> > +{
> > +	struct lock_entry *le;
> > +
> > +	rte_spinlock_lock(&lock_entry_lock);
> > +
> > +	TAILQ_FOREACH(le, &lock_entry_list, next) {
> > +		if (le->port_id == port_id &&
> > +		    le->callback == callback &&
> > +		    le->user_args == user_args)
> > +			break;
> > +	}
> > +
> > +	if (!le) {
> > +		le = calloc(1, sizeof(struct lock_entry));
> > +		if (!le) {
> 
> Nitpick: generally, DPDK style guide prefers "if (value)" or "if (!value)" to only
> be reserved for boolean values, and use explicit comparison (e.g. "if (value ==
> NULL)" or "if (value == 0)") for all other cases.

OK, will fix
> 
> --
> Thanks,
> Anatoly