[dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks

[dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks

[Mit Matelske]

The source callback list grows infinitely when more than alarm
is queued.

This fix recognizes that an alarm interrupt in FreeBSD should never
have more than one callback on its list, so if
rte_intr_callback_register() is called with an interrupt handle type
of RTE_INTR_HANDLE_ALARM, so if such an interrupt type already has a
non-empty list, then a new callback is not created, but the kevent
timer is restarted properly.

Signed-off-by: Mit Matelske <mit@pt.net>

---
 lib/librte_eal/freebsd/eal/eal_interrupts.c | 79 +++++++++++----------
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal/eal_interrupts.c b/lib/librte_eal/freebsd/eal/eal_interrupts.c
index f6831b790..3fee762be 100644
--- a/lib/librte_eal/freebsd/eal/eal_interrupts.c
+++ b/lib/librte_eal/freebsd/eal/eal_interrupts.c
@@ -83,9 +83,9 @@ int
 rte_intr_callback_register(const struct rte_intr_handle *intr_handle,
 		rte_intr_callback_fn cb, void *cb_arg)
 {
-	struct rte_intr_callback *callback = NULL;
-	struct rte_intr_source *src = NULL;
-	int ret, add_event;
+	struct rte_intr_callback *callback;
+	struct rte_intr_source *src;
+	int ret, add_event = 0;
 
 	/* first do parameter checking */
 	if (intr_handle == NULL || intr_handle->fd < 0 || cb == NULL) {
@@ -98,47 +98,53 @@ rte_intr_callback_register(const struct rte_intr_handle *intr_handle,
 		return -ENODEV;
 	}
 
-	/* allocate a new interrupt callback entity */
-	callback = calloc(1, sizeof(*callback));
-	if (callback == NULL) {
-		RTE_LOG(ERR, EAL, "Can not allocate memory\n");
-		return -ENOMEM;
-	}
-	callback->cb_fn = cb;
-	callback->cb_arg = cb_arg;
-	callback->pending_delete = 0;
-	callback->ucb_fn = NULL;
-
 	rte_spinlock_lock(&intr_lock);
 
-	/* check if there is at least one callback registered for the fd */
+	/* find the source for this intr_handle */
 	TAILQ_FOREACH(src, &intr_sources, next) {
-		if (src->intr_handle.fd == intr_handle->fd) {
-			/* we had no interrupts for this */
-			if (TAILQ_EMPTY(&src->callbacks))
-				add_event = 1;
-
-			TAILQ_INSERT_TAIL(&(src->callbacks), callback, next);
-			ret = 0;
+		if (src->intr_handle.fd == intr_handle->fd)
 			break;
-		}
 	}
 
-	/* no existing callbacks for this - add new source */
-	if (src == NULL) {
-		src = calloc(1, sizeof(*src));
-		if (src == NULL) {
+	/* if this is an alarm interrupt and it already has a callback,
+	 * then we don't want to create a new callback because the only
+	 * thing on the list should be eal_alarm_callback() and we may
+	 * be called just to reset the timer.
+	 */
+	if (src != NULL && src->intr_handle.type == RTE_INTR_HANDLE_ALARM &&
+		 !TAILQ_EMPTY(&src->callbacks)) {
+		callback = NULL;
+	} else {
+		/* allocate a new interrupt callback entity */
+		callback = calloc(1, sizeof(*callback));
+		if (callback == NULL) {
 			RTE_LOG(ERR, EAL, "Can not allocate memory\n");
 			ret = -ENOMEM;
 			goto fail;
-		} else {
-			src->intr_handle = *intr_handle;
-			TAILQ_INIT(&src->callbacks);
-			TAILQ_INSERT_TAIL(&(src->callbacks), callback, next);
-			TAILQ_INSERT_TAIL(&intr_sources, src, next);
-			add_event = 1;
-			ret = 0;
 		}
+		callback->cb_fn = cb;
+		callback->cb_arg = cb_arg;
+		callback->pending_delete = 0;
+		callback->ucb_fn = NULL;
+
+		if (src == NULL) {
+			src = calloc(1, sizeof(*src));
+			if (src == NULL) {
+				RTE_LOG(ERR, EAL, "Can not allocate memory\n");
+				ret = -ENOMEM;
+				goto fail;
+			} else {
+				src->intr_handle = *intr_handle;
+				TAILQ_INIT(&src->callbacks);
+				TAILQ_INSERT_TAIL(&intr_sources, src, next);
+			}
+		}
+
+		/* we had no interrupts for this */
+		if (TAILQ_EMPTY(&src->callbacks))
+			add_event = 1;
+
+		TAILQ_INSERT_TAIL(&(src->callbacks), callback, next);
 	}
 
 	/* add events to the queue. timer events are special as we need to
@@ -178,11 +184,12 @@ rte_intr_callback_register(const struct rte_intr_handle *intr_handle,
 	}
 	rte_spinlock_unlock(&intr_lock);
 
-	return ret;
+	return 0;
 fail:
 	/* clean up */
 	if (src != NULL) {
-		TAILQ_REMOVE(&(src->callbacks), callback, next);
+		if (callback != NULL)
+			TAILQ_REMOVE(&(src->callbacks), callback, next);
 		if (TAILQ_EMPTY(&(src->callbacks))) {
 			TAILQ_REMOVE(&intr_sources, src, next);
 			free(src);
-- 
2.23.0

Re: [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks

[Bruce Richardson]

On Wed, Nov 20, 2019 at 02:10:56PM -0600, Mit Matelske wrote:
> The source callback list grows infinitely when more than alarm
> is queued.
> 
> This fix recognizes that an alarm interrupt in FreeBSD should never
> have more than one callback on its list, so if
> rte_intr_callback_register() is called with an interrupt handle type
> of RTE_INTR_HANDLE_ALARM, so if such an interrupt type already has a
> non-empty list, then a new callback is not created, but the kevent
> timer is restarted properly.
> 
> Signed-off-by: Mit Matelske <mit@pt.net>
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks

[David Marchand]

On Mon, Mar 16, 2020 at 6:36 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Nov 20, 2019 at 02:10:56PM -0600, Mit Matelske wrote:
> > The source callback list grows infinitely when more than alarm
> > is queued.
> >
> > This fix recognizes that an alarm interrupt in FreeBSD should never
> > have more than one callback on its list, so if
> > rte_intr_callback_register() is called with an interrupt handle type
> > of RTE_INTR_HANDLE_ALARM, so if such an interrupt type already has a
> > non-empty list, then a new callback is not created, but the kevent
> > timer is restarted properly.
> >
> > Signed-off-by: Mit Matelske <mit@pt.net>
> >
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>

Missing a Fixes: line.

And probably worth backporting, so
Cc: stable@dpdk.org

-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks

[David Marchand]

On Thu, Mar 19, 2020 at 9:48 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Mon, Mar 16, 2020 at 6:36 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Wed, Nov 20, 2019 at 02:10:56PM -0600, Mit Matelske wrote:
> > > The source callback list grows infinitely when more than alarm
> > > is queued.
> > >
> > > This fix recognizes that an alarm interrupt in FreeBSD should never
> > > have more than one callback on its list, so if
> > > rte_intr_callback_register() is called with an interrupt handle type
> > > of RTE_INTR_HANDLE_ALARM, so if such an interrupt type already has a
> > > non-empty list, then a new callback is not created, but the kevent
> > > timer is restarted properly.
> > >

Fixes: 23150bd8d8a8 ("eal/bsd: add interrupt thread")
Cc: stable@dpdk.org

> > > Signed-off-by: Mit Matelske <mit@pt.net>
> > >
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> >

Applied, thanks.

-- 
David Marchand