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

[Burakov, Anatoly]

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?

> + */
> +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 unexpectly.
> + * NOTE: Lock a device mutliple times with same parmeter will increase
> + * a ref_count, and coresponding 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...).

> + *
> + * @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).

> +
> +/**
> + * 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.

-- 
Thanks,
Anatoly