DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks
@ 2019-11-20 20:10 Mit Matelske
  2020-03-16 17:35 ` Bruce Richardson
  0 siblings, 1 reply; 4+ messages in thread
From: Mit Matelske @ 2019-11-20 20:10 UTC (permalink / raw)
  To: bruce.richardson; +Cc: dev, 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


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

* Re: [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks
  2019-11-20 20:10 [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks Mit Matelske
@ 2020-03-16 17:35 ` Bruce Richardson
  2020-03-19  8:48   ` David Marchand
  0 siblings, 1 reply; 4+ messages in thread
From: Bruce Richardson @ 2020-03-16 17:35 UTC (permalink / raw)
  To: Mit Matelske; +Cc: dev

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>

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

* Re: [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks
  2020-03-16 17:35 ` Bruce Richardson
@ 2020-03-19  8:48   ` David Marchand
  2020-03-25 12:53     ` David Marchand
  0 siblings, 1 reply; 4+ messages in thread
From: David Marchand @ 2020-03-19  8:48 UTC (permalink / raw)
  To: Bruce Richardson, Mit Matelske; +Cc: dev

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


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

* Re: [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks
  2020-03-19  8:48   ` David Marchand
@ 2020-03-25 12:53     ` David Marchand
  0 siblings, 0 replies; 4+ messages in thread
From: David Marchand @ 2020-03-25 12:53 UTC (permalink / raw)
  To: Mit Matelske; +Cc: dev, Bruce Richardson

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


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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 20:10 [dpdk-dev] [PATCH] eal/freebsd: fix queuing duplicate eal_alarm_callbacks Mit Matelske
2020-03-16 17:35 ` Bruce Richardson
2020-03-19  8:48   ` David Marchand
2020-03-25 12:53     ` David Marchand

DPDK patches and discussions

Archives are clonable:
	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


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


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