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

Thomas Monjalon thomas at monjalon.net
Thu Jan 11 02:43:08 CET 2018


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.


More information about the dev mailing list