diff mbox series

[v4,1/4] ethdev: Add API to enable device event handler

Message ID 1531227098-29564-2-git-send-email-jia.guo@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series Enable eal hotplug event handler in ethdev | expand

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Guo, Jia July 10, 2018, 12:51 p.m. UTC
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

Zhang, Qi Z July 10, 2018, 1:24 p.m. UTC | #1
> -----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>
Andrew Rybchenko July 11, 2018, 8:49 a.m. UTC | #2
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)

<...>
Guo, Jia July 11, 2018, 11:17 a.m. UTC | #3
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)
>
> <...>
diff mbox series

Patch

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",
+			    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)
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index c9c825e..3de1cd4 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -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.