Re: [dpdk-dev] [PATCH v4 1/4] ethdev: Add API to enable device event handler
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
---
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->na
Re: [dpdk-dev] [PATCH v4 1/4] ethdev: Add API to enable device event handler
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
---
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)
<...>
Re: [dpdk-dev] [PATCH v4 1/4] ethdev: Add API to enable device event handler
> -Original Message- > From: Guo, Jia > Sent: Tuesday, July 10, 2018 8:52 PM > To: step...@networkplumber.org; Richardson, Bruce > ; Yigit, Ferruh ; > Ananyev, Konstantin ; > gaetan.ri...@6wind.com; Wu, Jingjing ; > tho...@monjalon.net; mo...@mellanox.com; ma...@mellanox.com; Van > Haaren, Harry ; Zhang, Qi Z > ; He, Shaopeng ; Iremonger, > Bernard ; arybche...@solarflare.com > Cc: jblu...@infradead.org; shreyansh.j...@nxp.com; dev@dpdk.org; Guo, Jia > ; Zhang, Helin > 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 Reviewed-by: Qi Zhang
