[v2,1/1] librte_eal: rte_intr_callback_unregister_sync() - wrapper around rte_intr_callback_unregister().
diff mbox series

Message ID 20200817140828.9769-2-Renata.Saiakhova@ekinops.com
State Deferred
Delegated to: David Marchand
Headers show
Series
  • pci_vfio_disable_notifier(): avoid race with unregister
Related show

Checks

Context Check Description
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Renata Saiakhova Aug. 17, 2020, 2:08 p.m. UTC
Avoid race with unregister interrupt hanlder 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>
---
 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/rte_eal_version.map      |  1 +
 5 files changed, 51 insertions(+), 1 deletion(-)

Comments

David Marchand Oct. 8, 2020, 7:47 a.m. UTC | #1
On Mon, Aug 17, 2020 at 4:09 PM Renata Saiakhova
<Renata.Saiakhova@ekinops.com> wrote:
>
> Avoid race with unregister interrupt hanlder 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>

Review please.
David Marchand Oct. 20, 2020, 1:40 p.m. UTC | #2
On Thu, Oct 8, 2020 at 9:47 AM David Marchand <david.marchand@redhat.com> wrote:
>
> On Mon, Aug 17, 2020 at 4:09 PM Renata Saiakhova
> <Renata.Saiakhova@ekinops.com> wrote:
> >
> > Avoid race with unregister interrupt hanlder 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>

Anatoly, Harman, this patch has been waiting for a long time.
Can you review it?


Thanks.
Harman Kalra Oct. 28, 2020, 8:36 p.m. UTC | #3
On Mon, Aug 17, 2020 at 04:08:27PM +0200, Renata Saiakhova wrote:
> External Email
> 
> ----------------------------------------------------------------------
> Avoid race with unregister interrupt hanlder 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,

   Just trying to understand the scenario, as you mentioned "while
   removing the device by rte_dev_remove()" are you calling
   rte_eal_hotplug_remove or kernel has sent an event to remove the
   device. As far as I know vfio notifier mechanism is used by kernel
   vfio driver to notify user to release the resources and as you are
   observing EAGAIN means same callback is executing.
   Regarding the tight polling loop in the patch, I think its good to
   have a fixed retry logic to avoid any unidentified corner case which
   might lead to infinite looping.

Thanks
Harman
   

> Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.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/rte_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 07e072e13..a4bfdf553 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -415,7 +415,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 6d53d33c8..7d99bdaff 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 13db5c4e8..c99d5dbd4 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/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index bf0c17c23..b1d824f59 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -325,6 +325,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
>

Patch
diff mbox series

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 07e072e13..a4bfdf553 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -415,7 +415,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 6d53d33c8..7d99bdaff 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 13db5c4e8..c99d5dbd4 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/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index bf0c17c23..b1d824f59 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -325,6 +325,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