DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v5 0/1] eal/interrupts: add synchronous wrapper around unregister
@ 2021-04-06 14:46 Renata Saiakhova
  2021-04-06 14:46 ` [dpdk-dev] [PATCH v5 1/1] " Renata Saiakhova
  0 siblings, 1 reply; 3+ messages in thread
From: Renata Saiakhova @ 2021-04-06 14:46 UTC (permalink / raw)
  Cc: dev, Renata Saiakhova

For pci_vfio, while removing the device by rte_dev_remove(),
pci_vfio_disable_notifier() will call rte_intr_callback_unregister(),
which may return -EAGAIN, if an interrupt source (kernel) has some active
callbacks right now. As a result, the callback (req notifier) can be never unregistered,
and the corresponding descriptor (vfio_req_intr_handle.fd) can be never closed.
The kernel will continuously try to notify the user space using req notifier, but as
the device is already removed, in this case it even cannot find a bus for that
device, the log is full of messages "EAL: Cannot find bus for device (XX:XX.X)"

v5:
  Re-spin with windows update and 21.05 edition
v4:
* Typo spelling in commit message
v3:
* Subject line reworded
v2:
* Use only for pci-vfio in pci_vfio_disable_notifier()

Renata Saiakhova (1):
  eal/interrupts: add synchronous wrapper around unregister

 drivers/bus/pci/linux/pci_vfio.c        |  2 +-
 lib/librte_eal/freebsd/eal_interrupts.c | 12 ++++++++++++
 lib/librte_eal/include/rte_interrupts.h | 25 +++++++++++++++++++++++++
 lib/librte_eal/linux/eal_interrupts.c   | 12 ++++++++++++
 lib/librte_eal/version.map              |  1 +
 lib/librte_eal/windows/eal_interrupts.c |  8 ++++++++
 6 files changed, 59 insertions(+), 1 deletion(-)

-- 
2.17.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [dpdk-dev] [PATCH v5 1/1] eal/interrupts: add synchronous wrapper around unregister
  2021-04-06 14:46 [dpdk-dev] [PATCH v5 0/1] eal/interrupts: add synchronous wrapper around unregister Renata Saiakhova
@ 2021-04-06 14:46 ` Renata Saiakhova
  2021-04-07  9:12   ` David Marchand
  0 siblings, 1 reply; 3+ messages in thread
From: Renata Saiakhova @ 2021-04-06 14:46 UTC (permalink / raw)
  To: Anatoly Burakov, Harman Kalra, Bruce Richardson, Ray Kinsella,
	Neil Horman, Dmitry Kozlyuk, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam
  Cc: dev, Renata Saiakhova

Avoid race with unregister interrupt handler if interrupt
source has some active callbacks at the moment, use wrapper
around rte_intr_callback_unregister() to check for -EAGAIN
return value and to loop until rte_intr_callback_unregister()
succeeds.

Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Harman Kalra <hkalra@marvell.com>
---
 drivers/bus/pci/linux/pci_vfio.c        |  2 +-
 lib/librte_eal/freebsd/eal_interrupts.c | 12 ++++++++++++
 lib/librte_eal/include/rte_interrupts.h | 25 +++++++++++++++++++++++++
 lib/librte_eal/linux/eal_interrupts.c   | 12 ++++++++++++
 lib/librte_eal/version.map              |  1 +
 lib/librte_eal/windows/eal_interrupts.c |  8 ++++++++
 6 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 9d689d6fd..d3a59a117 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -409,7 +409,7 @@ pci_vfio_disable_notifier(struct rte_pci_device *dev)
 		return -1;
 	}
 
-	ret = rte_intr_callback_unregister(&dev->vfio_req_intr_handle,
+	ret = rte_intr_callback_unregister_sync(&dev->vfio_req_intr_handle,
 					   pci_vfio_req_handler,
 					   (void *)&dev->device);
 	if (ret < 0) {
diff --git a/lib/librte_eal/freebsd/eal_interrupts.c b/lib/librte_eal/freebsd/eal_interrupts.c
index 72eeacbc1..86810845f 100644
--- a/lib/librte_eal/freebsd/eal_interrupts.c
+++ b/lib/librte_eal/freebsd/eal_interrupts.c
@@ -345,6 +345,18 @@ rte_intr_callback_unregister(const struct rte_intr_handle *intr_handle,
 	return ret;
 }
 
+int
+rte_intr_callback_unregister_sync(const struct rte_intr_handle *intr_handle,
+		rte_intr_callback_fn cb_fn, void *cb_arg)
+{
+	int ret = 0;
+
+	while ((ret = rte_intr_callback_unregister(intr_handle, cb_fn, cb_arg)) == -EAGAIN)
+		rte_pause();
+
+	return ret;
+}
+
 int
 rte_intr_enable(const struct rte_intr_handle *intr_handle)
 {
diff --git a/lib/librte_eal/include/rte_interrupts.h b/lib/librte_eal/include/rte_interrupts.h
index e3b406abc..cc3bf45d8 100644
--- a/lib/librte_eal/include/rte_interrupts.h
+++ b/lib/librte_eal/include/rte_interrupts.h
@@ -94,6 +94,31 @@ rte_intr_callback_unregister_pending(const struct rte_intr_handle *intr_handle,
 				rte_intr_callback_fn cb_fn, void *cb_arg,
 				rte_intr_unregister_callback_fn ucb_fn);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Loop until rte_intr_callback_unregister() succeeds.
+ * After a call to this function,
+ * the callback provided by the specified interrupt handle is unregistered.
+ *
+ * @param intr_handle
+ *  pointer to the interrupt handle.
+ * @param cb
+ *  callback address.
+ * @param cb_arg
+ *  address of parameter for callback, (void *)-1 means to remove all
+ *  registered which has the same callback address.
+ *
+ * @return
+ *  - On success, return the number of callback entities removed.
+ *  - On failure, a negative value.
+ */
+__rte_experimental
+int
+rte_intr_callback_unregister_sync(const struct rte_intr_handle *intr_handle,
+				rte_intr_callback_fn cb, void *cb_arg);
+
 /**
  * It enables the interrupt for the specified handle.
  *
diff --git a/lib/librte_eal/linux/eal_interrupts.c b/lib/librte_eal/linux/eal_interrupts.c
index 1dd994bd1..22b3b7bcd 100644
--- a/lib/librte_eal/linux/eal_interrupts.c
+++ b/lib/librte_eal/linux/eal_interrupts.c
@@ -662,6 +662,18 @@ rte_intr_callback_unregister(const struct rte_intr_handle *intr_handle,
 	return ret;
 }
 
+int
+rte_intr_callback_unregister_sync(const struct rte_intr_handle *intr_handle,
+			rte_intr_callback_fn cb_fn, void *cb_arg)
+{
+	int ret = 0;
+
+	while ((ret = rte_intr_callback_unregister(intr_handle, cb_fn, cb_arg)) == -EAGAIN)
+		rte_pause();
+
+	return ret;
+}
+
 int
 rte_intr_enable(const struct rte_intr_handle *intr_handle)
 {
diff --git a/lib/librte_eal/version.map b/lib/librte_eal/version.map
index e23745ae6..88d88aaed 100644
--- a/lib/librte_eal/version.map
+++ b/lib/librte_eal/version.map
@@ -421,6 +421,7 @@ EXPERIMENTAL {
 	rte_version_release;
 	rte_version_suffix;
 	rte_version_year;
+	rte_intr_callback_unregister_sync;
 };
 
 INTERNAL {
diff --git a/lib/librte_eal/windows/eal_interrupts.c b/lib/librte_eal/windows/eal_interrupts.c
index 3b8266d38..1d4cf794d 100644
--- a/lib/librte_eal/windows/eal_interrupts.c
+++ b/lib/librte_eal/windows/eal_interrupts.c
@@ -131,6 +131,14 @@ rte_intr_callback_unregister(
 	return 0;
 }
 
+int
+rte_intr_callback_unregister_sync(
+	__rte_unused const struct rte_intr_handle *intr_handle,
+	__rte_unused rte_intr_callback_fn cb_fn, __rte_unused void *cb_arg)
+{
+	return 0;
+}
+
 int
 rte_intr_enable(__rte_unused const struct rte_intr_handle *intr_handle)
 {
-- 
2.17.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/1] eal/interrupts: add synchronous wrapper around unregister
  2021-04-06 14:46 ` [dpdk-dev] [PATCH v5 1/1] " Renata Saiakhova
@ 2021-04-07  9:12   ` David Marchand
  0 siblings, 0 replies; 3+ messages in thread
From: David Marchand @ 2021-04-07  9:12 UTC (permalink / raw)
  To: Renata Saiakhova
  Cc: Anatoly Burakov, Harman Kalra, Bruce Richardson, Ray Kinsella,
	Neil Horman, Dmitry Kozlyuk, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam, dev

On Tue, Apr 6, 2021 at 4:50 PM Renata Saiakhova
<Renata.Saiakhova@ekinops.com> wrote:
>
> Avoid race with unregister interrupt handler if interrupt
> source has some active callbacks at the moment, use wrapper
> around rte_intr_callback_unregister() to check for -EAGAIN
> return value and to loop until rte_intr_callback_unregister()
> succeeds.
>
> Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Harman Kalra <hkalra@marvell.com>
> ---

[snip]

> diff --git a/lib/librte_eal/version.map b/lib/librte_eal/version.map
> index e23745ae6..88d88aaed 100644
> --- a/lib/librte_eal/version.map
> +++ b/lib/librte_eal/version.map
> @@ -421,6 +421,7 @@ EXPERIMENTAL {
>         rte_version_release;
>         rte_version_suffix;
>         rte_version_year;
> +       rte_intr_callback_unregister_sync;

Sorted this alphabetically + added symbol in rte_eal_exports.def.

Applied, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-07  9:12 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 14:46 [dpdk-dev] [PATCH v5 0/1] eal/interrupts: add synchronous wrapper around unregister Renata Saiakhova
2021-04-06 14:46 ` [dpdk-dev] [PATCH v5 1/1] " Renata Saiakhova
2021-04-07  9:12   ` David Marchand

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git