Re: [dpdk-dev] [PATCH v7 1/2] eal: add uevent monitor for hot plug

[Thomas Monjalon <thomas@monjalon.net>]

09/01/2018 09:25, Guo, Jia:
> On 1/9/2018 8:39 AM, Thomas Monjalon wrote:
> >> +int
> >> +_rte_dev_callback_process(struct rte_device *device,
> >> +			enum rte_eal_dev_event_type event,
> >> +			void *cb_arg, void *ret_param)
> > 
> > cb_arg must be an opaque parameter which is registered with the
> > callback and passed later. No need as parameter of this function.
> >
> > ret_param is not needed at all. The kernel event will be just
> > translated as rte_eal_dev_event_type (rte_dev_event after rename).
> 
> suggest hold one to let new param, such as device info, add by 
> ret_param, so cb_arg have set when register and no use anymore, delete it.

Sorry I don't understand. Please rephrase.

> >> --- a/lib/librte_eal/common/include/rte_bus.h
> >> +++ b/lib/librte_eal/common/include/rte_bus.h
> >>   /**
> >> + * Device iterator to find a device on a bus.
> >> + *
> >> + * This function returns an rte_device if one of those held by the bus
> >> + * matches the data passed as parameter.
> >> + *
> >> + * If the comparison function returns zero this function should stop iterating
> >> + * over any more devices. To continue a search the device of a previous search
> >> + * can be passed via the start parameter.
> >> + *
> >> + * @param cmp
> >> + *	the device name comparison function.
> >> + *
> >> + * @param data
> >> + *	Data to compare each device against.
> >> + *
> >> + * @param start
> >> + *	starting point for the iteration
> >> + *
> >> + * @return
> >> + *	The first device matching the data, NULL if none exists.
> >> + */
> >> +typedef struct rte_device *
> >> +(*rte_bus_find_device_by_name_t)(const struct rte_device *start,
> >> +			 rte_dev_cmp_name_t cmp,
> >> +			 const void *data);
> > 
> > Why is it needed? There is already rte_bus_find_device_t.
> 
> because the rte_bus_find_device_t just find a device structure in the 
> device list, but here need to find a device structure by device name 
> which come from uevent info.

I don't understand how it is different?
Looking at the code, it is a copy/paste except it is dedicated
to name comparison.
You can remove rte_bus_find_device_by_name_t and provide a
comparison function which looks at name.

> >> +int
> >> +rte_eal_dev_monitor_enable(void);
> > 
> > I suggest to drop this function which is just calling rte_dev_monitor_start.
> 
> more discuss, i suggest keep on it , let rte_dev_monitor_start 
> separately stay on the platform code and let user commonly call 
> rte_eal_dev_monitor_enable.

Then you may need a disable function.
It will end up to be like start/stop.
I think it is just redundant.

If kept, please rename it to rte_dev_event_monitor_enable.

> >> +	/* create the host thread to wait/handle the uevent from kernel */
> >> +	ret = pthread_create(&uev_monitor_thread, NULL,
> >> +		dev_uev_monitoring, NULL);
> >> +	return ret;
> >> +}
> > 
> > I think you should use rte_service for thread management.
> 
> thanks for your info, such a good mechanism to use that  i even not know 
> that before. i will study and use it.

OK, good.