* Re: [dpdk-dev] [EXT] [PATCH v4 1/1] eal/interrupts: add synchronous wrapper around unregister
2021-02-18 21:27 ` [dpdk-dev] [PATCH v4 1/1] " Renata Saiakhova
@ 2021-03-11 9:44 ` Harman Kalra
2021-03-25 13:14 ` [dpdk-dev] " David Marchand
1 sibling, 0 replies; 4+ messages in thread
From: Harman Kalra @ 2021-03-11 9:44 UTC (permalink / raw)
To: Renata Saiakhova, Anatoly Burakov, Bruce Richardson,
Ray Kinsella, Neil Horman
Cc: dev
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH v4 1/1] eal/interrupts: add synchronous wrapper around unregister
2021-02-18 21:27 ` [dpdk-dev] [PATCH v4 1/1] " Renata Saiakhova
2021-03-11 9:44 ` [dpdk-dev] [EXT] " Harman Kalra
@ 2021-03-25 13:14 ` David Marchand
1 sibling, 0 replies; 4+ messages in thread
From: David Marchand @ 2021-03-25 13:14 UTC (permalink / raw)
To: Renata Saiakhova
Cc: Anatoly Burakov, Harman Kalra, Bruce Richardson, Ray Kinsella,
Neil Horman, dev
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.
--
David Marchand
^ permalink raw reply [flat|nested] 4+ messages in thread