On 1/11/2018 9:43 AM, Thomas Monjalon wrote:
Hi,
Thanks for splitting the patches.
I will review the first one today. Please see below.
10/01/2018 10:12, Jeff Guo:
--- /dev/null
+++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
+int
+rte_dev_monitor_start(void)
+{
+ return -1;
+}
+
+int
+rte_dev_monitor_stop(void)
+{
+ return -1;
+}
You should add a log to show it is not supported.
ok.
--- /dev/null
+++ b/lib/librte_eal/bsdapp/eal/include/exec-env/rte_dev.h
+#ifndef _RTE_DEV_H_
+#error "don't include this file directly, please include generic <rte_dev.h>"
+#endif
Why creating different rte_dev.h for BSD and Linux?
This is an API, it should be the same.
if no need at this time, combine it to a file.
+/**
+ * Start the device uevent monitoring.
+ *
+ * @param none
+ * @return
+ * - On success, zero.
+ * - On failure, a negative value.
+ */
+int
+rte_dev_monitor_start(void);
+
+/**
+ * Stop the device uevent monitoring .
+ *
+ * @param none
+ * @return
+ * - On success, zero.
+ * - On failure, a negative value.
+ */
+
+int
+rte_dev_monitor_stop(void);
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -42,9 +42,32 @@
#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 rte_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
Please rename to rte_dev_event_lock.
Let's use rte_dev_event_ prefix consistently.
make consistently, agree.
+ * The user application callback description.
+ *
+ * It contains callback address to be registered by user application,
+ * the pointer to the parameters for callback, and the event type.
+ */
+struct rte_eal_dev_callback {
Rename to rte_dev_event?
+ TAILQ_ENTRY(rte_eal_dev_callback) next; /**< Callbacks list */
+ rte_eal_dev_cb_fn cb_fn; /**< Callback address */
Rename to rte_dev_event_callback?
+ void *cb_arg; /**< Parameter for callback */
Comment should be about opaque context.
+ void *ret_param; /**< Return parameter */
+ enum rte_dev_event_type event; /**< device event type */
+ uint32_t active; /**< Callback is executing */
Why active is needed?
avoid the lock when unregistered callback.
+};
+
+/* A genaral callback for all new devices be added onto the bus */
+static struct rte_eal_dev_callback *dev_add_cb;
It should not be a different callback for new devices.
You must allow registering the callback for all and new devices.
Please look how it's done for ethdev:
https://dpdk.org/patch/32900/
the aim to use this special callback is because when new device add onto
the bus, no device instance to store the callback. i saw ethdev
solution, that is base on port but that would not make sense in rte
device layer. so
i try to abandon add callback in rte device, replace of add device name
into callback , please see my v10 patch.
+int
+rte_dev_callback_register(struct rte_device *device,
+ enum rte_dev_event_type event,
+ rte_eal_dev_cb_fn cb_fn, void *cb_arg)
+{
Why passing an event type at registration?
I think the event processing dispatch must be done in the callback,
not at registration.
make sense, just register all type for device ,and let eal to pass the
event.
+ /* allocate a new interrupt callback entity */
+ user_cb = rte_zmalloc("eal device event",
+ sizeof(*user_cb), 0);
No need to use rte_malloc here.
Please check this callback API patch:
https://dpdk.org/patch/33144/
could be better to concentration the code. but if you could tell me why
not use rte_zmalloc.
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
+enum uev_monitor_netlink_group {
+ UEV_MONITOR_KERNEL,
+ UEV_MONITOR_UDEV,
+};
Please keep a namespace prefix like RTE_DEV_EVENT_ (same for enum name).
Some comments are missing for these constants.
+/**
+ * The device event type.
+ */
+enum rte_dev_event_type {
+ RTE_DEV_EVENT_UNKNOWN, /**< unknown event type */
+ RTE_DEV_EVENT_ADD, /**< device being added */
+ RTE_DEV_EVENT_REMOVE,
+ /**< device being removed */
+ RTE_DEV_EVENT_CHANGE,
+ /**< device status being changed,
+ * etc charger percent
+ */
What means status changed?
What means charger percent?
status changed means that object path change or other more, charger
percent just a example for some kobject status. so i don't think we
should explicit identify all , i will delete it until we want to use it.
+ RTE_DEV_EVENT_MOVE, /**< device sysfs path being moved */
sysfs is Linux specific
+ RTE_DEV_EVENT_ONLINE, /**< device being enable */
You mean a device can be added but not enabled?
So enabling is switching it on by a register? or something else?
+ RTE_DEV_EVENT_OFFLINE, /**< device being disable */
+ RTE_DEV_EVENT_MAX /**< max value of this enum */
+};
+
+struct rte_eal_uevent {
+ enum rte_dev_event_type type; /**< device event type */
+ int subsystem; /**< subsystem id */
+ char *devname; /**< device name */
+ enum uev_monitor_netlink_group group; /**< device netlink group */
+};
I don't understand why this struct is exposed in the public API.
Please rename from rte_eal_ to rte_dev_.
will modify the uevent to event.
@@ -166,6 +204,8 @@ struct rte_device {
const struct rte_driver *driver;/**< Associated driver */
int numa_node; /**< NUMA node connection */
struct rte_devargs *devargs; /**< Device user arguments */
+ /** User application callbacks for device event */
+ struct rte_eal_dev_cb_list uev_cbs;
Do not use uev word in API, it refers to uevent which is implementation
specific. You can name it event_callbacks.
I am afraid this change is breaking the ABI.
For the first time, 18.02 will be ABI stable.
will not modify rte device struct in v10.
+/**
+ * It registers the callback for the specific event. Multiple
+ * callbacks cal be registered at the same time.
+ * @param event
+ * The device event type.
+ * @param cb_fn
+ * callback address.
+ * @param cb_arg
+ * address of parameter for callback.
+ *
+ * @return
+ * - On success, zero.
+ * - On failure, a negative value.
+ */
+int rte_dev_callback_register(struct rte_device *device,
+ enum rte_dev_event_type event,
+ rte_eal_dev_cb_fn cb_fn, void *cb_arg);
+
+/**
+ * It unregisters the callback according to the specified event.
+ *
+ * @param event
+ * The event type which corresponding to the callback.
+ * @param cb_fn
+ * callback address.
+ * address of parameter for callback, (void *)-1 means to remove all
+ * registered which has the same callback address.
+ *
+ * @return
+ * - On success, return the number of callback entities removed.
+ * - On failure, a negative value.
+ */
+int rte_dev_callback_unregister(struct rte_device *device,
+ enum rte_dev_event_type event,
+ rte_eal_dev_cb_fn cb_fn, void *cb_arg);
Such new functions should be added as experimental.
There will be probably more to review in this patch.
Let's progress on these comments please.
thanks for your review!
