[dpdk-dev] [PATCH V16 2/4] eal: add device event monitor framework
Tan, Jianfeng
jianfeng.tan at intel.com
Wed Mar 28 05:39:19 CEST 2018
> -----Original Message-----
> From: Guo, Jia
> Sent: Monday, March 26, 2018 7:21 PM
> To: stephen at networkplumber.org; Richardson, Bruce; Yigit, Ferruh;
> Ananyev, Konstantin; gaetan.rivet at 6wind.com; Wu, Jingjing;
> thomas at monjalon.net; motih at mellanox.com; Van Haaren, Harry; Tan,
> Jianfeng
> Cc: jblunck at infradead.org; shreyansh.jain at nxp.com; dev at dpdk.org; Guo,
> Jia; Zhang, Helin
> Subject: [PATCH V16 2/4] eal: add device event monitor framework
>
> This patch aims to add a general device event monitor mechanism at
mechanism -> framework?
Linux uevent is a mechanism.
> EAL device layer, for device hotplug awareness and actions adopted
> accordingly. It could also expand for all other type of device event
> monitor, but not in this scope at the stage.
>
> To get started, users firstly register or unregister callbacks through
> the new added APIs. Callbacks can be some device specific, or for all
> devices.
> -rte_dev_callback_register
> -rte_dev_callback_unregister
>
New APIs shall be added into rte_eal_version.map.
And also, we shall update the release note.
> Then application shall call below new added APIs to enable/disable the
> mechanism:
> - rte_dev_event_monitor_start
> - rte_dev_event_monitor_stop
Do we really have the use case to keep the callbacks, but stop monitoring? I don't think we really need these two APIs to enable/disable.
Instead, if we have a callback registered, then enable it; if we don't have any callbacks, then it's definitely disabled.
>
> Use hotplug case for example, when device hotplug insertion or hotplug
> removal, we will get notified from kernel, then call user's callbacks
> accordingly to handle it, such as detach or attach the device from the
> bus, and could be benifit for futher fail-safe or live-migration.
Typo: "be benifit " -> "benefit"
Typo: " futher" -> "further"
>
> Signed-off-by: Jeff Guo <jia.guo at intel.com>
> ---
> v16->v15:
> 1.remove some linux related code out of eal common layer
> 2.fix some uneasy readble issue.
> ---
> lib/librte_eal/bsdapp/eal/Makefile | 1 +
> lib/librte_eal/bsdapp/eal/eal_dev.c | 19 +++++
> lib/librte_eal/common/eal_common_dev.c | 145
> ++++++++++++++++++++++++++++++++
> lib/librte_eal/common/eal_private.h | 24 ++++++
> lib/librte_eal/common/include/rte_dev.h | 92 ++++++++++++++++++++
> lib/librte_eal/linuxapp/eal/Makefile | 1 +
> lib/librte_eal/linuxapp/eal/eal_dev.c | 20 +++++
> 7 files changed, 302 insertions(+)
> create mode 100644 lib/librte_eal/bsdapp/eal/eal_dev.c
> create mode 100644 lib/librte_eal/linuxapp/eal/eal_dev.c
>
> diff --git a/lib/librte_eal/bsdapp/eal/Makefile
> b/lib/librte_eal/bsdapp/eal/Makefile
> index dd455e6..c0921dd 100644
> --- a/lib/librte_eal/bsdapp/eal/Makefile
> +++ b/lib/librte_eal/bsdapp/eal/Makefile
> @@ -33,6 +33,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=
> eal_lcore.c
> SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_timer.c
> SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_interrupts.c
> SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_alarm.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_dev.c
>
> # from common dir
> SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_lcore.c
> diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c
> b/lib/librte_eal/bsdapp/eal/eal_dev.c
> new file mode 100644
> index 0000000..ad606b3
> --- /dev/null
> +++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#include <rte_log.h>
> +
> +int __rte_experimental
> +rte_dev_event_monitor_start(void)
> +{
> + RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
> + return -1;
> +}
> +
> +int __rte_experimental
> +rte_dev_event_monitor_stop(void)
> +{
> + RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
> + return -1;
> +}
> diff --git a/lib/librte_eal/common/eal_common_dev.c
> b/lib/librte_eal/common/eal_common_dev.c
> index cd07144..3a1bbb6 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -14,9 +14,34 @@
> #include <rte_devargs.h>
> #include <rte_debug.h>
> #include <rte_log.h>
> +#include <rte_spinlock.h>
> +#include <rte_malloc.h>
>
> #include "eal_private.h"
>
> +/* spinlock for device callbacks */
> +static rte_spinlock_t dev_event_lock = RTE_SPINLOCK_INITIALIZER;
> +
> +/**
> + * The device event callback description.
> + *
> + * It contains callback address to be registered by user application,
> + * the pointer to the parameters for callback, and the device name.
> + */
> +struct dev_event_callback {
> + TAILQ_ENTRY(dev_event_callback) next; /**< Callbacks list */
> + rte_dev_event_cb_fn cb_fn; /**< Callback address */
> + void *cb_arg; /**< Callback parameter */
> + char *dev_name; /**< Callback devcie name, NULL is for all
> device */
Typo: " devcie" -> "device"
> + uint32_t active; /**< Callback is executing */
> +};
> +
> +/** @internal Structure to keep track of registered callbacks */
> +TAILQ_HEAD(dev_event_cb_list, dev_event_callback);
> +
> +/* The device event callback list for all registered callbacks. */
> +static struct dev_event_cb_list dev_event_cbs;
> +
> static int cmp_detached_dev_name(const struct rte_device *dev,
> const void *_name)
> {
> @@ -207,3 +232,123 @@ rte_eal_hotplug_remove(const char *busname,
> const char *devname)
> rte_eal_devargs_remove(busname, devname);
> return ret;
> }
> +
> +static struct dev_event_callback * __rte_experimental
We don't have to flag an internal function as " __rte_experimental ".
> +dev_event_cb_find(const char *device_name, rte_dev_event_cb_fn cb_fn,
> + void *cb_arg)
> +{
> + struct dev_event_callback *event_cb = NULL;
> +
> + TAILQ_FOREACH(event_cb, &(dev_event_cbs), next) {
> + if (event_cb->cb_fn == cb_fn && event_cb->cb_arg == cb_arg) {
> + if (device_name == NULL && event_cb->dev_name == NULL)
> + break;
> + if (device_name == NULL || event_cb->dev_name == NULL)
> + continue;
> + if (!strcmp(event_cb->dev_name, device_name))
> + break;
> + }
> + }
> + return event_cb;
> +}
> +
> +int __rte_experimental
> +rte_dev_callback_register(const char *device_name, rte_dev_event_cb_fn cb_fn,
> + void *cb_arg)
"rte_dev_event_callback_register" sounds more reasonable?
> +{
> + struct dev_event_callback *event_cb = NULL;
We don't need to initialize it to NULL.
> +
> + if (!cb_fn)
> + return -EINVAL;
> +
> + rte_spinlock_lock(&dev_event_lock);
> +
> + if (TAILQ_EMPTY(&(dev_event_cbs)))
> + TAILQ_INIT(&(dev_event_cbs));
> +
> + event_cb = dev_event_cb_find(device_name, cb_fn, cb_arg);
> +
> + /* create a new callback. */
> + if (event_cb == NULL) {
> + event_cb = malloc(sizeof(struct dev_event_callback));
> + if (event_cb != NULL) {
> + event_cb->cb_fn = cb_fn;
> + event_cb->cb_arg = cb_arg;
> + event_cb->dev_name = strdup(device_name);
> + if (event_cb->dev_name == NULL)
> + return -EINVAL;
> + TAILQ_INSERT_TAIL(&(dev_event_cbs), event_cb, next);
> + } else {
> + RTE_LOG(ERR, EAL,
> + "Failed to allocate memory for device event callback");
Miss the unlock here.
> + return -ENOMEM;
> + }
> + }
> +
> + rte_spinlock_unlock(&dev_event_lock);
> + return (event_cb == NULL) ? -EEXIST : 0;
> +}
> +
> +int __rte_experimental
> +rte_dev_callback_unregister(const char *device_name,
> rte_dev_event_cb_fn cb_fn,
> + void *cb_arg)
> +{
> + int ret = -1;
> + struct dev_event_callback *event_cb = NULL;
> +
> + if (!cb_fn)
> + return -EINVAL;
> +
> + rte_spinlock_lock(&dev_event_lock);
> +
> + event_cb = dev_event_cb_find(device_name, cb_fn, cb_arg);
> +
> + /*
> + * if this callback is not executing right now,
> + * then remove it.
> + */
This note is not in right place.
> + if (event_cb != NULL) {
> + if (event_cb->active == 0) {
> + TAILQ_REMOVE(&(dev_event_cbs), event_cb, next);
> + rte_free(event_cb);
> + ret = 0;
> + }
> + ret = -EBUSY;
Miss "else" for busy cb.
> + }
Miss "else" for a cb which is not existed. And print an error log here.
> +
> + rte_spinlock_unlock(&dev_event_lock);
> + return ret;
> +}
> +
> +int __rte_experimental
> +dev_callback_process(char *device_name, enum rte_dev_event_type event,
> + void *cb_arg)
>From interrupt thread, there is no cb_arg.
> +{
> + struct dev_event_callback *cb_lst;
> + int rc = 0;
> +
> + rte_spinlock_lock(&dev_event_lock);
> +
> + if (device_name == NULL)
> + return -EINVAL;
Put such check out of lock. Or it's very easy to miss the unlock which is happening now.
> +
> + TAILQ_FOREACH(cb_lst, &dev_event_cbs, next) {
> + if (!cb_lst->dev_name)
> + break;
> + else if (!strcmp(cb_lst->dev_name, device_name))
> + break;
> + }
We invoke only one callback. But for this device, we could have many callback to call.
> + if (cb_lst) {
> + cb_lst->active = 1;
> + if (cb_arg)
> + cb_lst->cb_arg = cb_arg;
What's the reason of overwriting this cb_arg?
> + rte_spinlock_unlock(&dev_event_lock);
> + rc = cb_lst->cb_fn(device_name, event,
> + cb_lst->cb_arg);
> + rte_spinlock_lock(&dev_event_lock);
> + cb_lst->active = 0;
> + }
> +
> + rte_spinlock_unlock(&dev_event_lock);
> + return rc;
What's the reason of returning the ret of a callback? I don't think we need to return anything here.
> +}
> diff --git a/lib/librte_eal/common/eal_private.h
> b/lib/librte_eal/common/eal_private.h
> index 0b28770..d55cd68 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -9,6 +9,8 @@
> #include <stdint.h>
> #include <stdio.h>
>
> +#include <rte_dev.h>
> +
> /**
> * Initialize the memzone subsystem (private to eal).
> *
> @@ -205,4 +207,26 @@ struct rte_bus
> *rte_bus_find_by_device_name(const char *str);
>
> int rte_mp_channel_init(void);
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
It's not an API. We don't need this.
> + *
> + * internal Executes all the user application registered callbacks for
> + * the specific device. It is for DPDK internal user only. User
> + * application should not call it directly.
> + *
> + * @param device_name
> + * The device name.
> + * @param event
> + * the device event type
> + * @param cb_arg
> + * callback parameter.
> + *
> + * @return
> + * - On success, return zero.
> + * - On failure, a negative value.
> + */
> +int __rte_experimental
It's not an API, so don't add "__rte_experimental" flag.
> +dev_callback_process(char *device_name, enum rte_dev_event_type
> event,
> + void *cb_arg);
As mentioned above, we don't have cb_arg from interrupt thread.
> #endif /* _EAL_PRIVATE_H_ */
> diff --git a/lib/librte_eal/common/include/rte_dev.h
> b/lib/librte_eal/common/include/rte_dev.h
> index b688f1e..8867de6 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -24,6 +24,26 @@ extern "C" {
> #include <rte_compat.h>
> #include <rte_log.h>
>
> +/**
> + * The device event type.
> + */
> +enum rte_dev_event_type {
> + RTE_DEV_EVENT_UNKNOWN, /**< unknown event type */
Again, why do we report an "unknown" event to applications?
> + RTE_DEV_EVENT_ADD, /**< device being added */
> + RTE_DEV_EVENT_REMOVE, /**< device being removed */
> + RTE_DEV_EVENT_MAX /**< max value of this enum */
> +};
> +
> +struct rte_dev_event {
> + enum rte_dev_event_type type; /**< device event type */
> + int subsystem; /**< subsystem id */
> + char *devname; /**< device name */
I prefer to remove such note if we can already get the information from the variable name.
> +};
> +
> +typedef int (*rte_dev_event_cb_fn)(char *device_name,
> + enum rte_dev_event_type event,
> + void *cb_arg);
"rte_dev_event_callback" sounds better than "rte_dev_event_cb_fn" to me.
> +
> __attribute__((format(printf, 2, 0)))
> static inline void
> rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> @@ -267,4 +287,76 @@ __attribute__((used)) = str
> }
> #endif
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * It registers the callback for the specific device.
"the" -> "a"
Besides, "It registers a callback for the specific device or all devices"
> + * Multiple callbacks cal be registered at the same time.
"cal" -> "can"
Besides, above sentence sounds like this API can register multiple callbacks. Change to:
"Users can call this API multiple times to register multiple callbacks."
> + *
> + * @param device_name
> + * The device name, that is the param name of the struct rte_device,
> + * null value means for all devices.
> + * @param cb_fn
> + * callback address.
> + * @param cb_arg
> + * address of parameter for callback.
> + *
> + * @return
> + * - On success, zero.
> + * - On failure, a negative value.
> + */
> +int __rte_experimental
> +rte_dev_callback_register(const char *device_name,
> rte_dev_event_cb_fn cb_fn,
> + void *cb_arg);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * It unregisters the callback according to the specified device.
> + *
> + * @param device_name
> + * The device name, that is the param name of the struct rte_device,
> + * null value means for all devices.
> + * @param cb_fn
> + * callback address.
> + * @param cb_arg
> + * address of parameter for callback.
> + *
> + * @return
> + * - On success, return the number of callback entities removed.
> + * - On failure, a negative value.
> + */
> +int __rte_experimental
> +rte_dev_callback_unregister(const char *device_name,
> rte_dev_event_cb_fn cb_fn,
> + void *cb_arg);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Start the device event monitoring.
> + *
> + * @param none
> + * @return
> + * - On success, zero.
> + * - On failure, a negative value.
> + */
> +int __rte_experimental
> +rte_dev_event_monitor_start(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Stop the device event monitoring .
> + *
> + * @param none
> + * @return
> + * - On success, zero.
> + * - On failure, a negative value.
> + */
> +int __rte_experimental
> +rte_dev_event_monitor_stop(void);
> #endif /* _RTE_DEV_H_ */
> diff --git a/lib/librte_eal/linuxapp/eal/Makefile
> b/lib/librte_eal/linuxapp/eal/Makefile
> index 7e5bbe8..8578796 100644
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -41,6 +41,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=
> eal_lcore.c
> SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_timer.c
> SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_interrupts.c
> SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_alarm.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_dev.c
>
> # from common dir
> SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_lcore.c
> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c
> b/lib/librte_eal/linuxapp/eal/eal_dev.c
> new file mode 100644
> index 0000000..5ab5830
> --- /dev/null
> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#include <rte_log.h>
> +#include <rte_dev.h>
> +
> +int __rte_experimental
> +rte_dev_event_monitor_start(void)
> +{
> + /* TODO: start uevent monitor for linux */
> + return 0;
> +}
> +
> +int __rte_experimental
> +rte_dev_event_monitor_stop(void)
> +{
> + /* TODO: stop uevent monitor for linux */
> + return 0;
> +}
> --
> 2.7.4
More information about the dev
mailing list