Re: [dpdk-dev] [EXTERNAL] [PATCH 2/2] eal/windows: implement alarm API

[Khoa To <khot@microsoft.com>]

Hi Dmitry,

Since all alarm callbacks are scheduled on the interrupt thread, looks like this implementation of rte_eal_alarm_set() would create a head-of-line blocking issue, as the callbacks are serialized one after the other.
Is that correct?  If so, while this does not violate the API contract, it seems undesirable.

I looked at the Linux implementation of alarm, and although I am not very familiar with Linux system, it seems to have similar implementation, with the same head-of-line blocking issue.

I'm wondering why in both Linux and this Windows implementation, we don't spawn a new thread to service each alarm independently.
And, regardless of the Linux implementation, should we consider another implementation that avoids the head-of-line blocking issue?

Khoa.

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Dmitry Kozlyuk
> Sent: Thursday, September 10, 2020 5:22 PM
> To: dev@dpdk.org
> Cc: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>; Dmitry Malloy
> (MESHCHANINOV) <dmitrym@microsoft.com>; Pallavi Kadam <pallavi.kadam@intel.com>
> Subject: [EXTERNAL] [dpdk-dev] [PATCH 2/2] eal/windows: implement alarm API
> 
> Implementation is based on waitable timers Win32 API. When timer is set,
> a callback and its argument are supplied to the OS, while timer handle
> is stored in EAL alarm list. When timer expires, OS wakes up the
> interrupt thread and runs the callback. Upon completion it removes the
> alarm.
> 
> Waitable timers must be set from the thread their callback will run in,
> eal_intr_thread_schedule() provides a way to schedule asyncronuous code
> execution in the interrupt thread. Alarm module builds synchronous timer
> setup on top of it.
> 
> Windows alarms are not a type of DPDK interrupt handle and do not
> interact with interrupt module beyond executing in the same thread.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  lib/librte_eal/rte_eal_exports.def |   2 +
>  lib/librte_eal/windows/eal_alarm.c | 219 +++++++++++++++++++++++++++++
>  lib/librte_eal/windows/meson.build |   1 +
>  3 files changed, 222 insertions(+)
>  create mode 100644 lib/librte_eal/windows/eal_alarm.c
> 
> diff --git a/lib/librte_eal/rte_eal_exports.def b/lib/librte_eal/rte_eal_exports.def
> index 9baca0110..b6abb5ae1 100644
> --- a/lib/librte_eal/rte_eal_exports.def
> +++ b/lib/librte_eal/rte_eal_exports.def
> @@ -14,6 +14,8 @@ EXPORTS
>  	rte_devargs_insert
>  	rte_devargs_next
>  	rte_devargs_remove
> +	rte_eal_alarm_set
> +	rte_eal_alarm_cancel
>  	rte_eal_get_configuration
>  	rte_eal_has_hugepages
>  	rte_eal_has_pci
> diff --git a/lib/librte_eal/windows/eal_alarm.c b/lib/librte_eal/windows/eal_alarm.c
> new file mode 100644
> index 000000000..3b262793a
> --- /dev/null
> +++ b/lib/librte_eal/windows/eal_alarm.c
> @@ -0,0 +1,219 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2020 Dmitry Kozlyuk
> + */
> +
> +#include <stdatomic.h>
> +#include <stdbool.h>
> +
> +#include <rte_alarm.h>
> +#include <rte_spinlock.h>
> +
> +#include <rte_eal_trace.h>
> +
> +#include "eal_windows.h"
> +
> +enum alarm_state {
> +	ALARM_ARMED,
> +	ALARM_TRIGGERED,
> +	ALARM_CANCELLED
> +};
> +
> +struct alarm_entry {
> +	LIST_ENTRY(alarm_entry) next;
> +	rte_eal_alarm_callback cb_fn;
> +	void *cb_arg;
> +	HANDLE timer;
> +	atomic_uint state;
> +};
> +
> +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 void
> +alarm_remove_unsafe(struct alarm_entry *ap)
> +{
> +	LIST_REMOVE(ap, next);
> +	CloseHandle(ap->timer);
> +	free(ap);
> +}
> +
> +static void
> +alarm_callback(void *arg, DWORD low __rte_unused, DWORD high __rte_unused)
> +{
> +	struct alarm_entry *ap = arg;
> +	unsigned int state = ALARM_ARMED;
> +
> +	if (!atomic_compare_exchange_strong(
> +			&ap->state, &state, ALARM_TRIGGERED))
> +		return;
> +
> +	ap->cb_fn(ap->cb_arg);
> +
> +	rte_spinlock_lock(&alarm_lock);
> +	alarm_remove_unsafe(ap);
> +	rte_spinlock_unlock(&alarm_lock);
> +}
> +
> +struct alarm_task {
> +	struct alarm_entry *entry;
> +	LARGE_INTEGER deadline;
> +	int ret;
> +};
> +
> +static void
> +alarm_set(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);
> +}
> +
> +int
> +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;
> +	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;
> +
> +	ap = calloc(1, sizeof(*ap));
> +	if (ap == NULL) {
> +		RTE_LOG(ERR, EAL, "Cannot allocate alarm entry\n");
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	timer = CreateWaitableTimer(NULL, FALSE, NULL);
> +	if (timer == NULL) {
> +		RTE_LOG_WIN32_ERR("CreateWaitableTimer()");
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +
> +	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.
> +	 */
> +	ret = intr_thread_exec(alarm_set, &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;
> +
> +	rte_spinlock_lock(&alarm_lock);
> +	LIST_INSERT_HEAD(&alarm_list, ap, next);
> +	rte_spinlock_unlock(&alarm_lock);
> +
> +	goto exit;
> +
> +fail:
> +	if (timer != NULL)
> +		CloseHandle(timer);
> +	if (ap != NULL)
> +		free(ap);
> +
> +exit:
> +	rte_eal_trace_alarm_set(us, cb_fn, cb_arg, ret);
> +	return ret;
> +}
> +
> +static bool
> +alarm_matches(const struct alarm_entry *ap,
> +	rte_eal_alarm_callback cb_fn, void *cb_arg)
> +{
> +	bool any_arg = cb_arg == (void *)(-1);
> +	return (ap->cb_fn == cb_fn) && (any_arg || ap->cb_arg == cb_arg);
> +}
> +
> +int
> +rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
> +{
> +	struct alarm_entry *ap;
> +	unsigned int state;
> +	int removed;
> +	bool executing;
> +
> +	removed = 0;
> +	do {
> +		executing = false;
> +
> +		rte_spinlock_lock(&alarm_lock);
> +
> +		LIST_FOREACH(ap, &alarm_list, next) {
> +			if (!alarm_matches(ap, cb_fn, cb_arg))
> +				continue;
> +
> +			state = ALARM_ARMED;
> +			if (atomic_compare_exchange_strong(
> +					&ap->state, &state, ALARM_CANCELLED)) {
> +				alarm_remove_unsafe(ap);
> +				removed++;
> +			} else if (state == ALARM_TRIGGERED)
> +				executing = true;
> +		}
> +
> +		rte_spinlock_unlock(&alarm_lock);
> +	} while (executing);
> +
> +	rte_eal_trace_alarm_cancel(cb_fn, cb_arg, removed);
> +	return removed;
> +}
> +
> +struct intr_task {
> +	void (*func)(void *arg);
> +	void *arg;
> +	rte_spinlock_t lock; /* unlocked at task completion */
> +};
> +
> +static void
> +intr_thread_entry(void *arg)
> +{
> +	struct intr_task *task = arg;
> +	task->func(task->arg);
> +	rte_spinlock_unlock(&task->lock);
> +}
> +
> +static int
> +intr_thread_exec(void (*func)(void *arg), void *arg)
> +{
> +	struct intr_task task;
> +	int ret;
> +
> +	task.func = func;
> +	task.arg = arg;
> +	rte_spinlock_init(&task.lock);
> +
> +	/* Make timers more precise by synchronizing in userspace. */
> +	rte_spinlock_lock(&task.lock);
> +	ret = eal_intr_thread_schedule(intr_thread_entry, &task);
> +	if (ret < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot schedule task to interrupt thread\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Wait for the task to complete. */
> +	rte_spinlock_lock(&task.lock);
> +	return 0;
> +}
> diff --git a/lib/librte_eal/windows/meson.build b/lib/librte_eal/windows/meson.build
> index b690bc6b0..3b2faf29e 100644
> --- a/lib/librte_eal/windows/meson.build
> +++ b/lib/librte_eal/windows/meson.build
> @@ -5,6 +5,7 @@ subdir('include')
> 
>  sources += files(
>  	'eal.c',
> +	'eal_alarm.c',
>  	'eal_debug.c',
>  	'eal_file.c',
>  	'eal_hugepages.c',
> --
> 2.25.4