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