From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Tan, Jianfeng" Subject: Re: [PATCH V16 2/4] eal: add device event monitor framework Date: Wed, 28 Mar 2018 03:39:19 +0000 Message-ID: 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "Zhang, Helin" To: "Guo, Jia" , "stephen@networkplumber.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "Ananyev, Konstantin" , "gaetan.rivet@6wind.com" , "Wu, Jingjing" , "thomas@monjalon.net" , "motih@mellanox.com" , "Van Haaren, Harry" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 453497CD8 for ; Wed, 28 Mar 2018 05:39:24 +0200 (CEST) In-Reply-To: <1522063256-3997-3-git-send-email-jia.guo@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----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 >=20 > 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. >=20 > 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 >=20 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.=20 >=20 > 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" >=20 > 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 >=20 > 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) +=3D > eal_lcore.c > SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D eal_timer.c > SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D eal_interrupts.c > SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D eal_alarm.c > +SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D eal_dev.c >=20 > # from common dir > SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D 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 >=20 > #include "eal_private.h" >=20 > +/* spinlock for device callbacks */ > +static rte_spinlock_t dev_event_lock =3D 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"=20 > + 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 =3D NULL; > + > + TAILQ_FOREACH(event_cb, &(dev_event_cbs), next) { > + if (event_cb->cb_fn =3D=3D cb_fn && event_cb->cb_arg =3D=3D cb_arg) { > + if (device_name =3D=3D NULL && event_cb->dev_name =3D=3D NULL) > + break; > + if (device_name =3D=3D NULL || event_cb->dev_name =3D=3D 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 c= b_fn, > + void *cb_arg) "rte_dev_event_callback_register" sounds more reasonable? > +{ > + struct dev_event_callback *event_cb =3D 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 =3D dev_event_cb_find(device_name, cb_fn, cb_arg); > + > + /* create a new callback. */ > + if (event_cb =3D=3D NULL) { > + event_cb =3D malloc(sizeof(struct dev_event_callback)); > + if (event_cb !=3D NULL) { > + event_cb->cb_fn =3D cb_fn; > + event_cb->cb_arg =3D cb_arg; > + event_cb->dev_name =3D strdup(device_name); > + if (event_cb->dev_name =3D=3D 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 =3D=3D 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 =3D -1; > + struct dev_event_callback *event_cb =3D NULL; > + > + if (!cb_fn) > + return -EINVAL; > + > + rte_spinlock_lock(&dev_event_lock); > + > + event_cb =3D 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 !=3D NULL) { > + if (event_cb->active =3D=3D 0) { > + TAILQ_REMOVE(&(dev_event_cbs), event_cb, next); > + rte_free(event_cb); > + ret =3D 0; > + } > + ret =3D -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 =3D 0; > + > + rte_spinlock_lock(&dev_event_lock); > + > + if (device_name =3D=3D NULL) > + return -EINVAL; Put such check out of lock. Or it's very easy to miss the unlock which is h= appening 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 callba= ck to call. > + if (cb_lst) { > + cb_lst->active =3D 1; > + if (cb_arg) > + cb_lst->cb_arg =3D cb_arg; What's the reason of overwriting this cb_arg? > + rte_spinlock_unlock(&dev_event_lock); > + rc =3D cb_lst->cb_fn(device_name, event, > + cb_lst->cb_arg); > + rte_spinlock_lock(&dev_event_lock); > + cb_lst->active =3D 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 >=20 > +#include > + > /** > * Initialize the memzone subsystem (private to eal). > * > @@ -205,4 +207,26 @@ struct rte_bus > *rte_bus_find_by_device_name(const char *str); >=20 > int rte_mp_channel_init(void); >=20 > +/** > + * @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 >=20 > +/** > + * 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)) =3D str > } > #endif >=20 > +/** > + * @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 callback= s. 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) +=3D > eal_lcore.c > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D eal_timer.c > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D eal_interrupts.c > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D eal_alarm.c > +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D eal_dev.c >=20 > # from common dir > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D 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