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.
> --- /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.
> +/**
> + * 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.
> + * 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?
> +};
> +
> +/* 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/
> +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.
> + /* 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/
> --- 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?
> + 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_.
> @@ -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.
> +/**
> + * 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.
