From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
Sent: Friday, June 29, 2018 12:47 AM
To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>
Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Shelton, Benjamin H <benjamin.h.shelton@intel.com>; Vangati, Narender <narender.vangati@intel.com>
Subject: Re: [dpdk-dev] [PATCH v7 04/19] ethdev: introduce device lock
On 06/28/2018 03:56 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.
I think that locking deserves a bit more details on why it is needed.
When/why should it be used by applications or other libraries.
Right now the description is too generic and real usecases are unclear.
[Qi], the typical use case is described in cover letter.
Primary works as resource management process and secondary process do the network stuff.
And we need handshake to prevent a running device be detached unexpected.
So we introduce the lock/unlock API for this requirement,
I can add these information in commit log, if you think it’s necessary.
Should applications always lock device if some data cores are polling
its Rx/Tx queues?
[Qi] Application should know if it is possible to detach a running device on a separate process or separate thread.
The lock API helps application to handle such kind of case.
Does it imply that all apps which would like to be
hotplug-aware should be updated accordingly?
Do you have guidelines or is it too early stage for now?
Basically we didn’t break any thing what we already have, we just provide a new option which looks helpful to simplify
the development of application that need to handle hotplug.
Aslo introduce the new API rte_eth_dev_lock_with_callback and
rte_eth_dev_unlock_with callback to 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.
What should/will happen if two callbacks are registered and the
first says OK, but the second denies detach. The device will be
detached from the first callback point of view, but finally remains.
[Qi] Though I’m not very sure if this will be the real case, but if that happens, one method for
the lock owner to know that a device is exactly detached or not is also register a callback for
RTE_ETH_EVENT_DESTROY, so it will be notified when device is detached and do some following cleanup
But yes there will be an issue if some resource is freed during the first callback, it will not rollback.
So, application should consider this.
Thanks
Qi
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com><mailto:qi.z.zhang@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com><mailto:anatoly.burakov@intel.com>
@@ -20,6 +20,7 @@ LIBABIVER := 9
SRCS-y += rte_ethdev.c
SRCS-y += ethdev_mp.c
+SRCS-y += ethdev_lock.c
SRCS-y += rte_flow.c
SRCS-y += rte_tm.c
SRCS-y += rte_mtr.c
new file mode 100644
@@ -0,0 +1,140 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+#include "ethdev_lock.h"
+
+struct lock_entry {
+ TAILQ_ENTRY(lock_entry) next;
+ rte_eth_dev_lock_callback_t callback;
+ uint16_t port_id;
+ void *user_args;
+ int ref_count;
+};
+
+TAILQ_HEAD(lock_entry_list, lock_entry);
+static struct lock_entry_list lock_entry_list =
+ TAILQ_HEAD_INITIALIZER(lock_entry_list);
+static rte_spinlock_t lock_entry_lock = RTE_SPINLOCK_INITIALIZER;
+
+int
+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 == NULL) {
+ le = calloc(1, sizeof(struct lock_entry));
+ if (le == NULL) {
+ rte_spinlock_unlock(&lock_entry_lock);
+ return -ENOMEM;
+ }
+ le->callback = callback;
+ le->port_id = port_id;
+ le->user_args = user_args;
+ TAILQ_INSERT_TAIL(&lock_entry_list, le, next);
+ }
+ le->ref_count++;
+
+ rte_spinlock_unlock(&lock_entry_lock);
+ return 0;
+}
+
+int
+unregister_lock_callback(uint16_t port_id,
+ rte_eth_dev_lock_callback_t callback,
+ void *user_args)
+{
+ struct lock_entry *le;
+ int ret = 0;
+
+ 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 != NULL) {
+ le->ref_count--;
+ if (le->ref_count == 0) {
+ TAILQ_REMOVE(&lock_entry_list, le, next);
+ free(le);
+ }
+ } else {
+ ret = -ENOENT;
+ }
+
+ rte_spinlock_unlock(&lock_entry_lock);
+ return ret;
+}
+
+static int clean_lock_callback_one(uint16_t port_id)
+{
+ struct lock_entry *le;
+ int ret = 0;
+
+ TAILQ_FOREACH(le, &lock_entry_list, next) {
+ if (le->port_id == port_id)
+ break;
+ }
+
+ if (le != NULL) {
+ le->ref_count--;
+ if (le->ref_count == 0) {
+ TAILQ_REMOVE(&lock_entry_list, le, next);
+ free(le);
+ }
+ } else {
+ ret = -ENOENT;
+ }
+
+ return ret;
+
+}
+
+void clean_lock_callback(uint16_t port_id)
+{
+ int ret;
+
+ rte_spinlock_lock(&lock_entry_lock);
+
+ for (;;) {
+ ret = clean_lock_callback_one(port_id);
+ if (ret == -ENOENT)
+ break;
+ }
+
+ rte_spinlock_unlock(&lock_entry_lock);
+}
+
+int process_lock_callbacks(uint16_t port_id)
+{
+ struct lock_entry *le;
+
+ rte_spinlock_lock(&lock_entry_lock);
+
+ TAILQ_FOREACH(le, &lock_entry_list, next) {
+ if (le->port_id != port_id)
+ continue;
+
+ if (le->callback(port_id, le->user_args)) {
+ rte_spinlock_unlock(&lock_entry_lock);
+ return -EBUSY;
+ }
+ }
+
+ rte_spinlock_unlock(&lock_entry_lock);
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+
+#ifndef _RTE_ETHDEV_LOCK_H_
+#define _RTE_ETHDEV_LOCK_H_
+
+#include "rte_ethdev.h"
+
+/* Register lock callback function on specific port */
+int
+register_lock_callback(uint16_t port_id,
+ rte_eth_dev_lock_callback_t callback,
+ void *user_args);
+
+/* Unregister lock callback function on specific port */
+int
+unregister_lock_callback(uint16_t port_id,
+ rte_eth_dev_lock_callback_t callback,
+ void *user_args);
+
+/**
+ * Unregister all callback function on specific port.
+ * This will be called when a device is detached.
+ */
+void clean_lock_callback(uint16_t port_id);
+
+/* Run each callback one by one. */
+int process_lock_callbacks(uint16_t port_id);
+
+#endif
@@ -6,6 +6,7 @@
#include "rte_ethdev_driver.h"
#include "ethdev_mp.h"
+#include "ethdev_lock.h"
#define MP_TIMEOUT_S 5 /**< 5 seconds timeouts */
@@ -108,7 +109,7 @@ static void __handle_primary_request(void *param)
ret = attach_on_secondary(req->devargs, req->port_id);
break;
case REQ_TYPE_PRE_DETACH:
- ret = 0;
+ ret = process_lock_callbacks(req->port_id);
break;
case REQ_TYPE_DETACH:
case REQ_TYPE_ATTACH_ROLLBACK:
@@ -6,6 +6,7 @@ version = 9
allow_experimental_apis = true
sources = files('ethdev_profile.c',
'ethdev_mp.c'
+ 'ethdev_lock.c'
'rte_ethdev.c',
'rte_flow.c',
'rte_mtr.c',
@@ -43,6 +43,7 @@
#include "ethdev_profile.h"
#include "ethdev_mp.h"
#include "ethdev_private.h"
+#include "ethdev_lock.h"
int ethdev_logtype;
@@ -734,6 +735,7 @@ do_eth_dev_detach(uint16_t port_id)
if (ret < 0)
return ret;
+ clean_lock_callback(port_id);
if (solid_release)
return rte_eth_dev_release_port(&rte_eth_devices[port_id]);
else
@@ -802,7 +804,6 @@ rte_eth_dev_attach(const char *devargs, uint16_t *port_id)
int
rte_eth_dev_attach_private(const char *devargs, uint16_t *port_id)
{
-
if (rte_eal_process_type() == RTE_PROC_PRIMARY)
return -ENOTSUP;
@@ -844,6 +845,10 @@ rte_eth_dev_detach(uint16_t port_id, char *name __rte_unused)
return req.result;
}
+ ret = process_lock_callbacks(port_id);
+ if (ret)
+ return ret;
+
/* check pre_detach */
req.t = REQ_TYPE_PRE_DETACH;
req.port_id = port_id;
@@ -890,6 +895,7 @@ int
rte_eth_dev_detach_private(uint16_t port_id, char *name __rte_unused)
{
uint32_t dev_flags;
+ int ret;
if (rte_eal_process_type() == RTE_PROC_PRIMARY)
return -ENOTSUP;
@@ -903,6 +909,10 @@ rte_eth_dev_detach_private(uint16_t port_id, char *name __rte_unused)
return -ENOTSUP;
}
+ ret = process_lock_callbacks(port_id);
+ if (ret)
+ return ret;
+
return do_eth_dev_detach(port_id);
}
@@ -4705,6 +4715,54 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
return result;
}
+static int
+dev_is_busy(uint16_t port_id __rte_unused, void *user_args __rte_unused)
+{
+ return -EBUSY;
+}
+
+int
+rte_eth_dev_lock(uint16_t port_id)
+{
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+ return register_lock_callback(port_id, dev_is_busy, NULL);
+}
+
+int
+rte_eth_dev_lock_with_callback(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 -EINVAL;
+
+ return register_lock_callback(port_id, callback, user_args);
+}
+
+int
+rte_eth_dev_unlock(uint16_t port_id)
+{
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+ return unregister_lock_callback(port_id, dev_is_busy, NULL);
+}
+
+int
+rte_eth_dev_unlock_with_callback(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 -EINVAL;
+
+ return unregister_lock_callback(port_id, callback, user_args);
+}
+
RTE_INIT(ethdev_init_log);
static void
ethdev_init_log(void)
@@ -4364,6 +4364,130 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t queue_id,
return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
}
+/**
+ * Callback function before device is detached.
+ *
+ * This type of function will be added into a function list, and will be
+ * invoked before device be detached. Application can register a callback
+ * function so it can be notified and do some cleanup before detach happen.
+ * Also, any callback function return !0 value will prevent device be
+ * detached (ref. rte_eth_dev_lock_with_callback and
+ * rte_eth_dev_unlock_with_callback).
+ *
+ * @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.
+ */
+typedef int (*rte_eth_dev_lock_callback_t)(uint16_t port_id, void *user_args);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Lock an Ethernet Device, this help application to prevent a device
+ * be detached unexpectedly.
+ *
+ * @note
+ * In multi-process situation, any process lock a share device will
+ * prevent it be detached from all process. Also this is per-process
+ * lock, which means unlock a device from process A take no effect
+ * if the device is locked from process B.
+ *
+ * @note
+ * Lock a device multiple times will increase a ref_count, and
+ * corresponding unlock decrease the ref_count, the device will be
+ * unlocked when ref_count reach 0.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ *
+ * @return
+ * 0 on success, negative on error.
+ */
+int __rte_experimental rte_eth_dev_lock(uint16_t port_id);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Lock an Ethernet device base on a callback function which can performs
+ * condition check at the moment before device be detached. if the
+ * condition check not pass, the device will not be detached, else,
+ * continue to detach or not rely on return value of other callbacks
+ * on the same port.
+ *
+ * @note
+ * Same as rte_eth_dev_lock, it is per-process lock.
+ *
+ * @note
+ * Lock a device with different callback or user_args will add different
+ * lock entries (<callback, user_args> pair) in a list. Lock a device
+ * multiple times with same callback and args will only increase a
+ * ref_count of specific lock entry, and corresponding unlock decrease
+ * the ref_count, an entry will be removed if its ref_count reach 0.
+ *
+ * @note
+ * All callbacks be attached to specific port will be removed
+ * automatically if the device is detached.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param callback
+ * 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.
+ * @param user_args
+ * parameter will be parsed to callback function.
+ *
+ * @return
+ * 0 on success, negative on error.
+ */
+int __rte_experimental
+rte_eth_dev_lock_with_callback(uint16_t port_id,
+ rte_eth_dev_lock_callback_t callback,
+ void *user_args);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Reverse operation of rte_eth_dev_lock.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ *
+ * @return
+ * 0 on success, negative on error.
+ */
+int __rte_experimental rte_eth_dev_unlock(uint16_t port_id);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Reverse operation of rte_eth_dev_lock_with_callback.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param callback
+ * parameter to match a lock entry.
+ * @param user_args
+ * parameter to match a lock entry.
+ *
+ * @return
+ * 0 on success, negative on error.
+ */
+int __rte_experimental
+rte_eth_dev_unlock_with_callback(uint16_t port_id,
+ rte_eth_dev_lock_callback_t callback,
+ void *user_args);
+
#ifdef __cplusplus
}
#endif
@@ -225,6 +225,7 @@ EXPERIMENTAL {
rte_eth_dev_get_module_eeprom;
rte_eth_dev_get_module_info;
rte_eth_dev_is_removed;
+ rte_eth_dev_lock;
rte_eth_dev_owner_delete;
rte_eth_dev_owner_get;
rte_eth_dev_owner_new;
@@ -232,6 +233,7 @@ EXPERIMENTAL {
rte_eth_dev_owner_unset;
rte_eth_dev_rx_offload_name;
rte_eth_dev_tx_offload_name;
+ rte_eth_dev_unlock;
rte_eth_switch_domain_alloc;
rte_eth_switch_domain_free;
rte_mtr_capabilities_get;