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

[Guo, Jia]

On 1/9/2018 7:38 PM, Thomas Monjalon wrote:
> 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.
please see v8 part of it. i need ret_param to pass the device name by 
the call back to the user.
>>>> --- 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.
i mean that if the device have been remove and then insertion, the 
device have not construct when just got the device name from the uevent 
massage,  so this case could i use the original find device function?
>>>> +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.
>