DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] eal/windows: fix deadlock when setting alarm
@ 2020-10-30 18:31 Dmitry Kozlyuk
  2020-10-30 18:42 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
  0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Kozlyuk @ 2020-10-30 18:31 UTC (permalink / raw)
  To: dev
  Cc: Kadam, Pallavi, Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy

Windows alarms are bot harmed and executed from the interrupt thread.
rte_eal_alarm_set() dispatched code to arm an alarm to that thread and
waited for its completion via a spinlock. However, if called from alarm
callback (i.e. from the interrupt thread), this caused a deadlock,
because arming could not be run until its dispatcher exits, but it could
only exit after arming has finished.

Call arming code directly when running in the interrupt thread.

Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")

Reported-by: Pallavi Kadam <pallavi.kadam@intel.com>
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 lib/librte_eal/windows/eal_alarm.c | 68 ++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 22 deletions(-)

diff --git a/lib/librte_eal/windows/eal_alarm.c b/lib/librte_eal/windows/eal_alarm.c
index 3b262793a..f5bf88715 100644
--- a/lib/librte_eal/windows/eal_alarm.c
+++ b/lib/librte_eal/windows/eal_alarm.c
@@ -30,7 +30,7 @@ static LIST_HEAD(alarm_list, alarm_entry) alarm_list = LIST_HEAD_INITIALIZER();
 
 static rte_spinlock_t alarm_lock = RTE_SPINLOCK_INITIALIZER;
 
-static int intr_thread_exec(void (*func)(void *arg), void *arg);
+static int intr_thread_exec_sync(void (*func)(void *arg), void *arg);
 
 static void
 alarm_remove_unsafe(struct alarm_entry *ap)
@@ -57,6 +57,18 @@ alarm_callback(void *arg, DWORD low __rte_unused, DWORD high __rte_unused)
 	rte_spinlock_unlock(&alarm_lock);
 }
 
+static int
+alarm_set(struct alarm_entry *entry, LARGE_INTEGER deadline)
+{
+	BOOL ret = SetWaitableTimer(
+		entry->timer, &deadline, 0, alarm_callback, entry, FALSE);
+	if (!ret) {
+		RTE_LOG_WIN32_ERR("SetWaitableTimer");
+		return -1;
+	}
+	return 0;
+}
+
 struct alarm_task {
 	struct alarm_entry *entry;
 	LARGE_INTEGER deadline;
@@ -64,14 +76,10 @@ struct alarm_task {
 };
 
 static void
-alarm_set(void *arg)
+alarm_task_exec(void *arg)
 {
 	struct alarm_task *task = arg;
-
-	BOOL ret = SetWaitableTimer(
-		task->entry->timer, &task->deadline,
-		0, alarm_callback, task->entry, FALSE);
-	task->ret = ret ? 0 : (-1);
+	task->ret = alarm_set(task->entry, task->deadline);
 }
 
 int
@@ -80,14 +88,14 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 	struct alarm_entry *ap;
 	HANDLE timer;
 	FILETIME ft;
-	struct alarm_task task;
+	LARGE_INTEGER deadline;
 	int ret;
 
 	/* Calculate deadline ASAP, unit of measure = 100ns. */
 	GetSystemTimePreciseAsFileTime(&ft);
-	task.deadline.LowPart = ft.dwLowDateTime;
-	task.deadline.HighPart = ft.dwHighDateTime;
-	task.deadline.QuadPart += 10 * us;
+	deadline.LowPart = ft.dwLowDateTime;
+	deadline.HighPart = ft.dwHighDateTime;
+	deadline.QuadPart += 10 * us;
 
 	ap = calloc(1, sizeof(*ap));
 	if (ap == NULL) {
@@ -106,21 +114,37 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 	ap->timer = timer;
 	ap->cb_fn = cb_fn;
 	ap->cb_arg = cb_arg;
-	task.entry = ap;
 
 	/* Waitable timer must be set in the same thread that will
 	 * do an alertable wait for the alarm to trigger, that is,
-	 * in the interrupt thread. Setting can fail, so do it synchronously.
+	 * in the interrupt thread.
 	 */
-	ret = intr_thread_exec(alarm_set, &task);
-	if (ret < 0) {
-		RTE_LOG(ERR, EAL, "Cannot setup alarm in interrupt thread\n");
-		goto fail;
-	}
+	if (rte_thread_is_intr()) {
+		/* Directly schedule callback execution. */
+		ret = alarm_set(ap, deadline);
+		if (ret < 0) {
+			RTE_LOG(ERR, EAL, "Cannot setup alarm\n");
+			goto fail;
+		}
+	} else {
+		/* Dispatch a task to set alarm into the interrupt thread.
+		 * Execute it synchronously, because it can fail.
+		 */
+		struct alarm_task task = {
+			.entry = ap,
+			.deadline = deadline,
+		};
+
+		ret = intr_thread_exec_sync(alarm_task_exec, &task);
+		if (ret < 0) {
+			RTE_LOG(ERR, EAL, "Cannot setup alarm in interrupt thread\n");
+			goto fail;
+		}
 
-	ret = task.ret;
-	if (ret < 0)
-		goto fail;
+		ret = task.ret;
+		if (ret < 0)
+			goto fail;
+	}
 
 	rte_spinlock_lock(&alarm_lock);
 	LIST_INSERT_HEAD(&alarm_list, ap, next);
@@ -196,7 +220,7 @@ intr_thread_entry(void *arg)
 }
 
 static int
-intr_thread_exec(void (*func)(void *arg), void *arg)
+intr_thread_exec_sync(void (*func)(void *arg), void *arg)
 {
 	struct intr_task task;
 	int ret;
-- 
2.28.0


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

* [dpdk-dev] [PATCH v2] eal/windows: fix deadlock when setting alarm
  2020-10-30 18:31 [dpdk-dev] [PATCH] eal/windows: fix deadlock when setting alarm Dmitry Kozlyuk
@ 2020-10-30 18:42 ` Dmitry Kozlyuk
  2020-10-30 19:25   ` Kadam, Pallavi
  0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Kozlyuk @ 2020-10-30 18:42 UTC (permalink / raw)
  To: dev
  Cc: Kadam, Pallavi, Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy

Windows alarms are both armed and executed from the interrupt thread.
rte_eal_alarm_set() dispatched alarm-arming code to that thread and
waited for its completion via a spinlock. However, if called from alarm
callback (i.e. from the interrupt thread), this caused a deadlock,
because arming could not be run until its dispatcher exits, but it could
only exit after it finished waiting for arming to complete.

Call arming code directly when running in the interrupt thread.

Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")

Reported-by: Pallavi Kadam <pallavi.kadam@intel.com>
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---

v2: fix typo in description

 lib/librte_eal/windows/eal_alarm.c | 68 ++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 22 deletions(-)

diff --git a/lib/librte_eal/windows/eal_alarm.c b/lib/librte_eal/windows/eal_alarm.c
index 3b262793a..f5bf88715 100644
--- a/lib/librte_eal/windows/eal_alarm.c
+++ b/lib/librte_eal/windows/eal_alarm.c
@@ -30,7 +30,7 @@ static LIST_HEAD(alarm_list, alarm_entry) alarm_list = LIST_HEAD_INITIALIZER();
 
 static rte_spinlock_t alarm_lock = RTE_SPINLOCK_INITIALIZER;
 
-static int intr_thread_exec(void (*func)(void *arg), void *arg);
+static int intr_thread_exec_sync(void (*func)(void *arg), void *arg);
 
 static void
 alarm_remove_unsafe(struct alarm_entry *ap)
@@ -57,6 +57,18 @@ alarm_callback(void *arg, DWORD low __rte_unused, DWORD high __rte_unused)
 	rte_spinlock_unlock(&alarm_lock);
 }
 
+static int
+alarm_set(struct alarm_entry *entry, LARGE_INTEGER deadline)
+{
+	BOOL ret = SetWaitableTimer(
+		entry->timer, &deadline, 0, alarm_callback, entry, FALSE);
+	if (!ret) {
+		RTE_LOG_WIN32_ERR("SetWaitableTimer");
+		return -1;
+	}
+	return 0;
+}
+
 struct alarm_task {
 	struct alarm_entry *entry;
 	LARGE_INTEGER deadline;
@@ -64,14 +76,10 @@ struct alarm_task {
 };
 
 static void
-alarm_set(void *arg)
+alarm_task_exec(void *arg)
 {
 	struct alarm_task *task = arg;
-
-	BOOL ret = SetWaitableTimer(
-		task->entry->timer, &task->deadline,
-		0, alarm_callback, task->entry, FALSE);
-	task->ret = ret ? 0 : (-1);
+	task->ret = alarm_set(task->entry, task->deadline);
 }
 
 int
@@ -80,14 +88,14 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 	struct alarm_entry *ap;
 	HANDLE timer;
 	FILETIME ft;
-	struct alarm_task task;
+	LARGE_INTEGER deadline;
 	int ret;
 
 	/* Calculate deadline ASAP, unit of measure = 100ns. */
 	GetSystemTimePreciseAsFileTime(&ft);
-	task.deadline.LowPart = ft.dwLowDateTime;
-	task.deadline.HighPart = ft.dwHighDateTime;
-	task.deadline.QuadPart += 10 * us;
+	deadline.LowPart = ft.dwLowDateTime;
+	deadline.HighPart = ft.dwHighDateTime;
+	deadline.QuadPart += 10 * us;
 
 	ap = calloc(1, sizeof(*ap));
 	if (ap == NULL) {
@@ -106,21 +114,37 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 	ap->timer = timer;
 	ap->cb_fn = cb_fn;
 	ap->cb_arg = cb_arg;
-	task.entry = ap;
 
 	/* Waitable timer must be set in the same thread that will
 	 * do an alertable wait for the alarm to trigger, that is,
-	 * in the interrupt thread. Setting can fail, so do it synchronously.
+	 * in the interrupt thread.
 	 */
-	ret = intr_thread_exec(alarm_set, &task);
-	if (ret < 0) {
-		RTE_LOG(ERR, EAL, "Cannot setup alarm in interrupt thread\n");
-		goto fail;
-	}
+	if (rte_thread_is_intr()) {
+		/* Directly schedule callback execution. */
+		ret = alarm_set(ap, deadline);
+		if (ret < 0) {
+			RTE_LOG(ERR, EAL, "Cannot setup alarm\n");
+			goto fail;
+		}
+	} else {
+		/* Dispatch a task to set alarm into the interrupt thread.
+		 * Execute it synchronously, because it can fail.
+		 */
+		struct alarm_task task = {
+			.entry = ap,
+			.deadline = deadline,
+		};
+
+		ret = intr_thread_exec_sync(alarm_task_exec, &task);
+		if (ret < 0) {
+			RTE_LOG(ERR, EAL, "Cannot setup alarm in interrupt thread\n");
+			goto fail;
+		}
 
-	ret = task.ret;
-	if (ret < 0)
-		goto fail;
+		ret = task.ret;
+		if (ret < 0)
+			goto fail;
+	}
 
 	rte_spinlock_lock(&alarm_lock);
 	LIST_INSERT_HEAD(&alarm_list, ap, next);
@@ -196,7 +220,7 @@ intr_thread_entry(void *arg)
 }
 
 static int
-intr_thread_exec(void (*func)(void *arg), void *arg)
+intr_thread_exec_sync(void (*func)(void *arg), void *arg)
 {
 	struct intr_task task;
 	int ret;
-- 
2.28.0


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

* Re: [dpdk-dev] [PATCH v2] eal/windows: fix deadlock when setting alarm
  2020-10-30 18:42 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
@ 2020-10-30 19:25   ` Kadam, Pallavi
  2020-11-03 20:32     ` Thomas Monjalon
  0 siblings, 1 reply; 4+ messages in thread
From: Kadam, Pallavi @ 2020-10-30 19:25 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev; +Cc: Narcisa Ana Maria Vasile, Dmitry Malloy


On 10/30/2020 11:42 AM, Dmitry Kozlyuk wrote:
> Windows alarms are both armed and executed from the interrupt thread.
> rte_eal_alarm_set() dispatched alarm-arming code to that thread and
> waited for its completion via a spinlock. However, if called from alarm
> callback (i.e. from the interrupt thread), this caused a deadlock,
> because arming could not be run until its dispatcher exits, but it could
> only exit after it finished waiting for arming to complete.
>
> Call arming code directly when running in the interrupt thread.
>
> Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")
>
> Reported-by: Pallavi Kadam <pallavi.kadam@intel.com>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---

Tested-by: Pallavi Kadam <pallavi.kadam@intel.com>

Acked-by: Pallavi Kadam <pallavi.kadam@intel.com>

>

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

* Re: [dpdk-dev] [PATCH v2] eal/windows: fix deadlock when setting alarm
  2020-10-30 19:25   ` Kadam, Pallavi
@ 2020-11-03 20:32     ` Thomas Monjalon
  0 siblings, 0 replies; 4+ messages in thread
From: Thomas Monjalon @ 2020-11-03 20:32 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Narcisa Ana Maria Vasile, Dmitry Malloy, Kadam, Pallavi

30/10/2020 20:25, Kadam, Pallavi:
> On 10/30/2020 11:42 AM, Dmitry Kozlyuk wrote:
> > Windows alarms are both armed and executed from the interrupt thread.
> > rte_eal_alarm_set() dispatched alarm-arming code to that thread and
> > waited for its completion via a spinlock. However, if called from alarm
> > callback (i.e. from the interrupt thread), this caused a deadlock,
> > because arming could not be run until its dispatcher exits, but it could
> > only exit after it finished waiting for arming to complete.
> >
> > Call arming code directly when running in the interrupt thread.
> >
> > Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")
> >
> > Reported-by: Pallavi Kadam <pallavi.kadam@intel.com>
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> 
> Tested-by: Pallavi Kadam <pallavi.kadam@intel.com>
> 
> Acked-by: Pallavi Kadam <pallavi.kadam@intel.com>

Applied, thanks



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

end of thread, other threads:[~2020-11-03 20:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-30 18:31 [dpdk-dev] [PATCH] eal/windows: fix deadlock when setting alarm Dmitry Kozlyuk
2020-10-30 18:42 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
2020-10-30 19:25   ` Kadam, Pallavi
2020-11-03 20:32     ` Thomas Monjalon

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	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

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


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