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

["Guo, Jia" <jia.guo@intel.com>]

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!