From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 1D0DC5F1D for ; Wed, 28 Mar 2018 10:12:57 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Mar 2018 01:12:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,370,1517904000"; d="scan'208";a="41916162" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.67.68.74]) ([10.67.68.74]) by fmsmga001.fm.intel.com with ESMTP; 28 Mar 2018 01:12:47 -0700 To: "Tan, Jianfeng" , "stephen@networkplumber.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "Ananyev, Konstantin" , "gaetan.rivet@6wind.com" , "Wu, Jingjing" , "thomas@monjalon.net" , "motih@mellanox.com" , "Van Haaren, Harry" References: <1521610066-12966-3-git-send-email-jia.guo@intel.com> <1522063256-3997-1-git-send-email-jia.guo@intel.com> <1522063256-3997-3-git-send-email-jia.guo@intel.com> Cc: "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "Zhang, Helin" From: "Guo, Jia" Message-ID: <1a51d6af-541a-2a46-6626-95bfc24afa97@intel.com> Date: Wed, 28 Mar 2018 16:12:47 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V16 2/4] eal: add device event monitor framework X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Mar 2018 08:12:58 -0000 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@networkplumber.org; Richardson, Bruce; Yigit, Ferruh; >> Ananyev, Konstantin; gaetan.rivet@6wind.com; Wu, Jingjing; >> thomas@monjalon.net; motih@mellanox.com; Van Haaren, Harry; Tan, >> Jianfeng >> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@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 >> --- >> 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 >> + >> +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 >> #include >> #include >> +#include >> +#include >> >> #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 >> #include >> >> +#include >> + >> /** >> * 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 >> #include >> >> +/** >> + * 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 >> +#include >> + >> +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