[v4,1/4] ethdev: Add API to enable device event handler
Checks
Commit Message
Implement a couple of eal device event handler install/uninstall APIs
in ethdev, it could let PMDs have chance to manage the eal device event,
such as register device event callback, then monitor and process the
hotplug event.
Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v4->v3:
change to use eal device event handler install api
---
doc/guides/rel_notes/release_18_08.rst | 12 +++++++
lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++
lib/librte_ethdev/rte_ethdev_driver.h | 32 ++++++++++++++++++
3 files changed, 103 insertions(+)
Comments
> -----Original Message-----
> From: Guo, Jia
> Sent: Tuesday, July 10, 2018 8:52 PM
> To: stephen@networkplumber.org; Richardson, Bruce
> <bruce.richardson@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> gaetan.rivet@6wind.com; Wu, Jingjing <jingjing.wu@intel.com>;
> thomas@monjalon.net; motih@mellanox.com; matan@mellanox.com; Van
> Haaren, Harry <harry.van.haaren@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; He, Shaopeng <shaopeng.he@intel.com>; Iremonger,
> Bernard <bernard.iremonger@intel.com>; arybchenko@solarflare.com
> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org; Guo, Jia
> <jia.guo@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> Subject: [PATCH v4 1/4] ethdev: Add API to enable device event handler
>
> Implement a couple of eal device event handler install/uninstall APIs in ethdev,
> it could let PMDs have chance to manage the eal device event, such as register
> device event callback, then monitor and process the hotplug event.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
On 10.07.2018 15:51, Jeff Guo wrote:
> Implement a couple of eal device event handler install/uninstall APIs
> in ethdev, it could let PMDs have chance to manage the eal device event,
> such as register device event callback, then monitor and process the
> hotplug event.
I think it is moving in right direction, but my problem with the patch is
that I don't understand what prevents us to make it even more generic.
Right now it looks like that PCI driver flag RTE_PCI_DRV_INTR_RMV is
sufficient and everything else could be done on ethdev layer.
RTE_PCI_DRV_INTR_RMV is mapped to RTE_ETH_DEV_INTR_RMV eth
device flag. The flag may be used as an indication in rte_eth_dev_create()
to register the callback.
May be I'm completely wrong above, but if so, I'd like to understand why
and prefer to see explanations in the patch description.
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v4->v3:
> change to use eal device event handler install api
> ---
> doc/guides/rel_notes/release_18_08.rst | 12 +++++++
> lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++
> lib/librte_ethdev/rte_ethdev_driver.h | 32 ++++++++++++++++++
> 3 files changed, 103 insertions(+)
>
> diff --git a/doc/guides/rel_notes/release_18_08.rst b/doc/guides/rel_notes/release_18_08.rst
> index bc01242..b6482ce 100644
> --- a/doc/guides/rel_notes/release_18_08.rst
> +++ b/doc/guides/rel_notes/release_18_08.rst
> @@ -46,6 +46,18 @@ 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 process helper in ethdev.**
> +
> + Implement a couple of eal device event handler install/uninstall APIs in
> + ethdev, these helper could let PMDs have chance to manage the eal device
> + event, such as register device event callback, then monitor and process
> + hotplug event.
> +
> + * ``rte_eth_dev_event_handler_install`` for PMDs use to install the device
> + event handler.
> + * ``rte_eth_dev_event_handler_uninstall`` for PMDs use to uninstall the device
> + event handler.
> +
>
> API Changes
> -----------
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index a9977df..09ea02d 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4518,6 +4518,65 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
> return result;
> }
>
> +static void __rte_experimental
> +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;
> +
> + switch (type) {
> + case RTE_DEV_EVENT_REMOVE:
> + ethdev_log(INFO, "The device: %s has been removed!\n",
Colon after 'device' above looks strange for me. I'd suggest to remove it.
If so, similar below for ADD.
> + device_name);
> +
> + if (!device_name || !eth_dev)
> + return;
> +
> + if (!(eth_dev->data->dev_flags & RTE_ETH_EVENT_INTR_RMV))
It looks like a typo above. Shouldn't it be RTE_ETH_DEV_INTR_RMV?
> + return;
> +
> + if (!strcmp(device_name, eth_dev->device->name))
> + _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;
> + }
> +}
> +
> +int __rte_experimental
> +rte_eth_dev_event_handler_install(struct rte_eth_dev *eth_dev)
> +{
> + int result = 0;
> +
> + result = rte_dev_event_callback_register(eth_dev->device->name,
> + eth_dev_event_callback, eth_dev);
> + if (result)
> + RTE_LOG(ERR, EAL, "device event callback register failed for "
> + "device:%s!\n", eth_dev->device->name);
Why ethdev_log() is used above, but here is RTE_LOG with EAL?
Similar question below.
> +
> + return result;
> +}
> +
> +int __rte_experimental
> +rte_eth_dev_event_handler_uninstall(struct rte_eth_dev *eth_dev)
> +{
> + int result = 0;
> +
> + result = rte_dev_event_callback_unregister(eth_dev->device->name,
> + eth_dev_event_callback, eth_dev);
> + if (result)
> + RTE_LOG(ERR, EAL, "device event callback unregister failed for"
> + " device:%s!\n", eth_dev->device->name);
> +
> + return result;
> +}
> +
> RTE_INIT(ethdev_init_log);
> static void
> ethdev_init_log(void)
<...>
On 7/11/2018 4:49 PM, Andrew Rybchenko wrote:
> On 10.07.2018 15:51, Jeff Guo wrote:
>> Implement a couple of eal device event handler install/uninstall APIs
>> in ethdev, it could let PMDs have chance to manage the eal device event,
>> such as register device event callback, then monitor and process the
>> hotplug event.
>
> I think it is moving in right direction, but my problem with the patch is
> that I don't understand what prevents us to make it even more generic.
> Right now it looks like that PCI driver flag RTE_PCI_DRV_INTR_RMV is
> sufficient and everything else could be done on ethdev layer.
> RTE_PCI_DRV_INTR_RMV is mapped to RTE_ETH_DEV_INTR_RMV eth
> device flag. The flag may be used as an indication in
> rte_eth_dev_create()
> to register the callback.
> May be I'm completely wrong above, but if so, I'd like to understand why
> and prefer to see explanations in the patch description.
>
Your acknowledgement is right, and it is make sense to check the reason
why is the most generic but not other.
i think that let driver to decide if it support the RTE_PCI_DRV_INTR_RMV
should be fine, that is first.
And second, the place of installer in driver is also fine, each ethdev
driver install specific callback for each port, and could let
driver have change to manage the status of hotplug for further. So if
there are no better place in ethdev here for more generic to install it,
that should be fine.
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v4->v3:
>> change to use eal device event handler install api
>> ---
>> doc/guides/rel_notes/release_18_08.rst | 12 +++++++
>> lib/librte_ethdev/rte_ethdev.c | 59
>> ++++++++++++++++++++++++++++++++++
>> lib/librte_ethdev/rte_ethdev_driver.h | 32 ++++++++++++++++++
>> 3 files changed, 103 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/release_18_08.rst
>> b/doc/guides/rel_notes/release_18_08.rst
>> index bc01242..b6482ce 100644
>> --- a/doc/guides/rel_notes/release_18_08.rst
>> +++ b/doc/guides/rel_notes/release_18_08.rst
>> @@ -46,6 +46,18 @@ 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 process helper in ethdev.**
>> +
>> + Implement a couple of eal device event handler install/uninstall
>> APIs in
>> + ethdev, these helper could let PMDs have chance to manage the eal
>> device
>> + event, such as register device event callback, then monitor and
>> process
>> + hotplug event.
>> +
>> + * ``rte_eth_dev_event_handler_install`` for PMDs use to install
>> the device
>> + event handler.
>> + * ``rte_eth_dev_event_handler_uninstall`` for PMDs use to
>> uninstall the device
>> + event handler.
>> +
>> API Changes
>> -----------
>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>> b/lib/librte_ethdev/rte_ethdev.c
>> index a9977df..09ea02d 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -4518,6 +4518,65 @@ rte_eth_devargs_parse(const char *dargs,
>> struct rte_eth_devargs *eth_da)
>> return result;
>> }
>> +static void __rte_experimental
>> +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;
>> +
>> + switch (type) {
>> + case RTE_DEV_EVENT_REMOVE:
>> + ethdev_log(INFO, "The device: %s has been removed!\n",
>
> Colon after 'device' above looks strange for me. I'd suggest to remove
> it.
> If so, similar below for ADD.
>
ok, i am fine.
>> + device_name);
>> +
>> + if (!device_name || !eth_dev)
>> + return;
>> +
>> + if (!(eth_dev->data->dev_flags & RTE_ETH_EVENT_INTR_RMV))
>
> It looks like a typo above. Shouldn't it be RTE_ETH_DEV_INTR_RMV?
>
you are right here, it is a typo.
>> + return;
>> +
>> + if (!strcmp(device_name, eth_dev->device->name))
>> + _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;
>> + }
>> +}
>> +
>> +int __rte_experimental
>> +rte_eth_dev_event_handler_install(struct rte_eth_dev *eth_dev)
>> +{
>> + int result = 0;
>> +
>> + result = rte_dev_event_callback_register(eth_dev->device->name,
>> + eth_dev_event_callback, eth_dev);
>> + if (result)
>> + RTE_LOG(ERR, EAL, "device event callback register failed for "
>> + "device:%s!\n", eth_dev->device->name);
>
> Why ethdev_log() is used above, but here is RTE_LOG with EAL?
> Similar question below.
>
should be align to use ethdev_log.
>> +
>> + return result;
>> +}
>> +
>> +int __rte_experimental
>> +rte_eth_dev_event_handler_uninstall(struct rte_eth_dev *eth_dev)
>> +{
>> + int result = 0;
>> +
>> + result = rte_dev_event_callback_unregister(eth_dev->device->name,
>> + eth_dev_event_callback, eth_dev);
>> + if (result)
>> + RTE_LOG(ERR, EAL, "device event callback unregister failed for"
>> + " device:%s!\n", eth_dev->device->name);
>> +
>> + return result;
>> +}
>> +
>> RTE_INIT(ethdev_init_log);
>> static void
>> ethdev_init_log(void)
>
> <...>
@@ -46,6 +46,18 @@ 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 process helper in ethdev.**
+
+ Implement a couple of eal device event handler install/uninstall APIs in
+ ethdev, these helper could let PMDs have chance to manage the eal device
+ event, such as register device event callback, then monitor and process
+ hotplug event.
+
+ * ``rte_eth_dev_event_handler_install`` for PMDs use to install the device
+ event handler.
+ * ``rte_eth_dev_event_handler_uninstall`` for PMDs use to uninstall the device
+ event handler.
+
API Changes
-----------
@@ -4518,6 +4518,65 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
return result;
}
+static void __rte_experimental
+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;
+
+ 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))
+ _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;
+ }
+}
+
+int __rte_experimental
+rte_eth_dev_event_handler_install(struct rte_eth_dev *eth_dev)
+{
+ int result = 0;
+
+ result = rte_dev_event_callback_register(eth_dev->device->name,
+ eth_dev_event_callback, eth_dev);
+ if (result)
+ RTE_LOG(ERR, EAL, "device event callback register failed for "
+ "device:%s!\n", eth_dev->device->name);
+
+ return result;
+}
+
+int __rte_experimental
+rte_eth_dev_event_handler_uninstall(struct rte_eth_dev *eth_dev)
+{
+ int result = 0;
+
+ result = rte_dev_event_callback_unregister(eth_dev->device->name,
+ eth_dev_event_callback, eth_dev);
+ if (result)
+ RTE_LOG(ERR, EAL, "device event callback unregister failed for"
+ " device:%s!\n", eth_dev->device->name);
+
+ return result;
+}
+
RTE_INIT(ethdev_init_log);
static void
ethdev_init_log(void)
@@ -82,6 +82,38 @@ 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 helper to install the device event handler for the specific
+ * ether device.
+ *
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ *
+ * @return
+ * - 0 on success, negative on error
+ */
+int __rte_experimental
+rte_eth_dev_event_handler_install(struct rte_eth_dev *dev);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Implement a helper to uninstall the device event handler for the specific
+ * ether device.
+ *
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ *
+ * @return
+ * - 0 on success, negative on error
+ */
+int __rte_experimental
+rte_eth_dev_event_handler_uninstall(struct rte_eth_dev *dev);
+
+/**
* @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.