Re: [dpdk-dev] [PATCH V9 1/5] eal: add uevent monitor api and callback func

[Thomas Monjalon <thomas@monjalon.net>]

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.