Re: [dpdk-dev] [PATCH v3 1/4] ethdev: Add eal device event callback

[Jeff Guo <jia.guo@intel.com>]

hi, andrew


On 7/9/2018 9:14 PM, Andrew Rybchenko wrote:
> On 09.07.2018 14:46, Jeff Guo wrote:
>> Implement a eal device event callback "rte_eth_dev_event_callback"
>> in ethdev, it could let pmd driver have chance to manage the eal
>> device event, such as process hotplug event.
>  >
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v3->v2:
>> add new callback in ethdev
>> ---
>>   doc/guides/rel_notes/release_18_08.rst |  8 ++++++++
>>   lib/librte_ethdev/rte_ethdev.c         | 37 
>> ++++++++++++++++++++++++++++++++++
>>   lib/librte_ethdev/rte_ethdev_driver.h  | 20 ++++++++++++++++++
>>   3 files changed, 65 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/release_18_08.rst 
>> b/doc/guides/rel_notes/release_18_08.rst
>> index bc01242..2326058 100644
>> --- a/doc/guides/rel_notes/release_18_08.rst
>> +++ b/doc/guides/rel_notes/release_18_08.rst
>> @@ -46,6 +46,14 @@ New Features
>>     Flow API support has been added to CXGBE Poll Mode Driver to offload
>>     flows to Chelsio T5/T6 NICs.
>>   +* **Added eal device event callback in ethdev for hotplug.**
>> +
>> +  Implement a eal device event callback in ethdev, it could let pmd 
>> driver
>
> "pmd driver" sounds strange since PMD stands for poll-mode driver.
>

ok and will modify it. thanks.

>> +  have chance to manage the eal device event, such as process 
>> hotplug event.
>> +
>> +  * ``rte_eth_dev_event_callback`` for driver use to register it and 
>> process
>> +    eal device event.
>> +
>>     API Changes
>>   -----------
>> diff --git a/lib/librte_ethdev/rte_ethdev.c 
>> b/lib/librte_ethdev/rte_ethdev.c
>> index a9977df..36f218a 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -4518,6 +4518,43 @@ rte_eth_devargs_parse(const char *dargs, 
>> struct rte_eth_devargs *eth_da)
>>       return result;
>>   }
>>   +void __rte_experimental
>> +rte_eth_dev_event_callback(char *device_name, enum 
>> rte_dev_event_type type,
>> +                 void *arg)
>> +{
>> +    struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)arg;
>> +
>> +    if (type >= RTE_DEV_EVENT_MAX) {
>> +        fprintf(stderr, "%s called upon invalid event %d\n",
>> +            __func__, type);
>> +        fflush(stderr);
>
> I'd like to understand why fprintf() is used here for logging instead 
> of rte_log
> mechanisms.
> Also if we really want the log, may be it make sense to move the if to 
> default
> case below.
>

ok.

>> +    }
>> +
>> +    switch (type) {
>> +    case RTE_DEV_EVENT_REMOVE:
>> +        ethdev_log(INFO, "The device: %s has been removed!\n",
>> +                device_name);
>> +
>> +        if (!device_name || !eth_dev)
>> +            return;
>> +
>> +        if (!(eth_dev->data->dev_flags & RTE_ETH_EVENT_INTR_RMV))
>> +            return;
>> +
>> +        if (!strcmp(device_name, eth_dev->device->name))
>
> Do we really need to check it? The callback is registered for devices
> with such name, so it should be always true. May be it is OK to 
> double-check
> I just want to be sure that I understand it properly.
>

i think it should be check here, since the eth_dev is being an pointer 
of arg to transfer into the eal event callback, and the arg is no 
default relation with the device name,
and we could not require user to always set the valid value when they 
use the callback.

>> + _rte_eth_dev_callback_process(eth_dev,
>> +                              RTE_ETH_EVENT_INTR_RMV,
>> +                              NULL);
>> +        break;
>> +    case RTE_DEV_EVENT_ADD:
>> +        ethdev_log(INFO, "The device: %s has been added!\n",
>> +            device_name);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +}
>> +
>>   RTE_INIT(ethdev_init_log);
>>   static void
>>   ethdev_init_log(void)
>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h 
>> b/lib/librte_ethdev/rte_ethdev_driver.h
>> index c9c825e..fed5afa 100644
>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>> @@ -82,6 +82,26 @@ int rte_eth_dev_release_port(struct rte_eth_dev 
>> *eth_dev);
>>   void _rte_eth_dev_reset(struct rte_eth_dev *dev);
>>     /**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Implement a rte eth eal device event callbacks for the specific 
>> device.
>> + *
>> + * @param device_name
>> + *  Pointer to the name of the rte device.
>
> Is it name of the device which generates the event? If so, it should 
> be highlighted.
>

yes, should be.

>> + * @param event
>> + *  Eal device event type.
>> + * @param ret_param
>> + *  To pass data back to user application.
>> + *
>> + * @return
>> + *  void
>> + */
>> +void __rte_experimental
>> +rte_eth_dev_event_callback(char *device_name,
>> +        enum rte_dev_event_type event, void *cb_arg);
>> +
>> +/**
>>    * @internal Executes all the user application registered callbacks 
>> for
>>    * the specific device. It is for DPDK internal user only. User
>>    * application should not call it directly.
>