[v4,1/1] eal/interrupts: add synchronous wrapper around unregister
Checks
Commit Message
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>
---
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 +
5 files changed, 51 insertions(+), 1 deletion(-)
Comments
> 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.
>
Hi Renata,
Thanks for the clarification, changes looks good to me.
Acked-by: Harman Kalra <hkalra@marvell.com>
> Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.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 +
> 5 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c
> b/drivers/bus/pci/linux/pci_vfio.c
> index e3f7b6abe..c14f7f8c4 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -414,7 +414,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
> fce90a112..56caa9cc9 100644
> --- a/lib/librte_eal/version.map
> +++ b/lib/librte_eal/version.map
> @@ -318,6 +318,7 @@ EXPERIMENTAL {
> rte_fbarray_find_rev_biggest_free;
> rte_fbarray_find_rev_biggest_used;
> rte_intr_callback_unregister_pending;
> + rte_intr_callback_unregister_sync;
> rte_realloc_socket;
>
> # added in 19.08
> --
> 2.17.2
On Thu, Feb 18, 2021 at 10:28 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>
> ---
> 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 +
> 5 files changed, 51 insertions(+), 1 deletion(-)
Seeing the description of this function, I'd expect it to be the same
on all OS implementations.
Please, could you respin with Windows update?
[snip]
> diff --git a/lib/librte_eal/version.map b/lib/librte_eal/version.map
> index fce90a112..56caa9cc9 100644
> --- a/lib/librte_eal/version.map
> +++ b/lib/librte_eal/version.map
> @@ -318,6 +318,7 @@ EXPERIMENTAL {
> rte_fbarray_find_rev_biggest_free;
> rte_fbarray_find_rev_biggest_used;
> rte_intr_callback_unregister_pending;
> + rte_intr_callback_unregister_sync;
> rte_realloc_socket;
>
> # added in 19.08
> --
> 2.17.2
>
The new symbol should be with other 21.05 additions.
Thanks.
@@ -414,7 +414,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) {
@@ -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)
{
@@ -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.
*
@@ -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)
{
@@ -318,6 +318,7 @@ EXPERIMENTAL {
rte_fbarray_find_rev_biggest_free;
rte_fbarray_find_rev_biggest_used;
rte_intr_callback_unregister_pending;
+ rte_intr_callback_unregister_sync;
rte_realloc_socket;
# added in 19.08