[dpdk-dev] [PATCH V16 2/4] eal: add device event monitor framework

Guo, Jia jia.guo at intel.com
Wed Mar 28 10:12:47 CEST 2018


jianfeng

will correct every typo, and comment inline.

On 3/28/2018 11:39 AM, Tan, Jianfeng wrote:
>
>> -----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.
you raise a good question, but if it is good readable to enable the 
monitor into the register function or let register into the enable 
function, should let we think about that.
>> 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?
it is no used anymore here.
>> +		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