From ad58fd2c61f108df5c75951d8f2ba2d015952c9e Mon Sep 17 00:00:00 2001
From: =?utf8?q?Ji=C5=99=C3=AD=20Z=C3=A1rev=C3=BAcky?=
 <zarevucky.jiri@gmail.com>
Date: Mon, 15 Aug 2022 18:26:16 +0200
Subject: [PATCH] Make timeout->cpu immutable

We ensure timeout->cpu is only changed in timeout_register(),
which by its nature is externally synchronized with timeout_unregister(),
and internally synchronized with clock(). Thus in both those contexts,
timeout->cpu is always a valid constant pointing to the CPU of last
call to timeout_register().

Doing so removes the need for synchronization using timeout->lock.
Instead, timeout->link is synchronized by timeout->cpu->timeoutlock,
and all other fields of timeout_t are also immutable outside
timeout_register(), which means they are safely synchronized by
a combination of timeout->cpu->timoutlock and external sychronization
of timeout_register/unregister.
---
 kernel/generic/include/time/timeout.h |  3 +-
 kernel/generic/src/time/clock.c       |  1 -
 kernel/generic/src/time/timeout.c     | 60 ++++++-----------------------------
 3 files changed, 10 insertions(+), 54 deletions(-)

diff --git a/kernel/generic/include/time/timeout.h b/kernel/generic/include/time/timeout.h
index 96c66d379..c8d0483ea 100644
--- a/kernel/generic/include/time/timeout.h
+++ b/kernel/generic/include/time/timeout.h
@@ -44,7 +44,7 @@ typedef void (*timeout_handler_t)(void *arg);
 typedef struct {
 	IRQ_SPINLOCK_DECLARE(lock);
 
-	/** Link to the list of active timeouts on CURRENT->cpu */
+	/** Link to the list of active timeouts on timeout->cpu */
 	link_t link;
 	/** Timeout will be activated when current clock tick reaches this value. */
 	uint64_t deadline;
@@ -60,7 +60,6 @@ typedef struct {
 
 extern void timeout_init(void);
 extern void timeout_initialize(timeout_t *);
-extern void timeout_reinitialize(timeout_t *);
 extern void timeout_register(timeout_t *, uint64_t, timeout_handler_t, void *);
 extern bool timeout_unregister(timeout_t *);
 
diff --git a/kernel/generic/src/time/clock.c b/kernel/generic/src/time/clock.c
index d26492b52..a6ffe6e15 100644
--- a/kernel/generic/src/time/clock.c
+++ b/kernel/generic/src/time/clock.c
@@ -171,7 +171,6 @@ void clock(void)
 			list_remove(cur);
 			timeout_handler_t handler = timeout->handler;
 			void *arg = timeout->arg;
-			timeout_reinitialize(timeout);
 
 			irq_spinlock_unlock(&timeout->lock, false);
 			irq_spinlock_unlock(&CPU->timeoutlock, false);
diff --git a/kernel/generic/src/time/timeout.c b/kernel/generic/src/time/timeout.c
index 7f5af0ee2..7e1e507ba 100644
--- a/kernel/generic/src/time/timeout.c
+++ b/kernel/generic/src/time/timeout.c
@@ -56,22 +56,6 @@ void timeout_init(void)
 	list_initialize(&CPU->timeout_active_list);
 }
 
-/** Reinitialize timeout
- *
- * Initialize all members except the lock.
- *
- * @param timeout Timeout to be initialized.
- *
- */
-void timeout_reinitialize(timeout_t *timeout)
-{
-	timeout->cpu = NULL;
-	timeout->deadline = 0;
-	timeout->handler = NULL;
-	timeout->arg = NULL;
-	link_initialize(&timeout->link);
-}
-
 /** Initialize timeout
  *
  * Initialize all members including the lock.
@@ -82,7 +66,8 @@ void timeout_reinitialize(timeout_t *timeout)
 void timeout_initialize(timeout_t *timeout)
 {
 	irq_spinlock_initialize(&timeout->lock, "timeout_t_lock");
-	timeout_reinitialize(timeout);
+	link_initialize(&timeout->link);
+	timeout->cpu = NULL;
 }
 
 /** Register timeout
@@ -103,9 +88,6 @@ void timeout_register(timeout_t *timeout, uint64_t time,
 	irq_spinlock_lock(&CPU->timeoutlock, true);
 	irq_spinlock_lock(&timeout->lock, false);
 
-	if (timeout->cpu)
-		panic("Unexpected: timeout->cpu != 0.");
-
 	timeout->cpu = CPU;
 	timeout->deadline = CPU->current_clock_tick + us2ticks(time);
 	timeout->handler = handler;
@@ -151,41 +133,17 @@ void timeout_register(timeout_t *timeout, uint64_t time,
  */
 bool timeout_unregister(timeout_t *timeout)
 {
-	DEADLOCK_PROBE_INIT(p_tolock);
-
-grab_locks:
-	irq_spinlock_lock(&timeout->lock, true);
-	if (!timeout->cpu) {
-		irq_spinlock_unlock(&timeout->lock, true);
-		return false;
-	}
-
-	if (!irq_spinlock_trylock(&timeout->cpu->timeoutlock)) {
-		irq_spinlock_unlock(&timeout->lock, true);
-		DEADLOCK_PROBE(p_tolock, DEADLOCK_THRESHOLD);
-		goto grab_locks;
-	}
+	assert(timeout->cpu);
 
-	/*
-	 * Now we know for sure that timeout hasn't been activated yet
-	 * and is lurking in timeout->cpu->timeout_active_list.
-	 */
+	irq_spinlock_lock(&timeout->cpu->timeoutlock, true);
 
-	link_t *cur = list_next(&timeout->link,
-	    &timeout->cpu->timeout_active_list);
-	if (cur != NULL) {
-		timeout_t *tmp = list_get_instance(cur, timeout_t, link);
-		irq_spinlock_lock(&tmp->lock, false);
-		irq_spinlock_unlock(&tmp->lock, false);
+	bool success = link_in_use(&timeout->link);
+	if (success) {
+		list_remove(&timeout->link);
 	}
 
-	list_remove(&timeout->link);
-	irq_spinlock_unlock(&timeout->cpu->timeoutlock, false);
-
-	timeout_reinitialize(timeout);
-	irq_spinlock_unlock(&timeout->lock, true);
-
-	return true;
+	irq_spinlock_unlock(&timeout->cpu->timeoutlock, true);
+	return success;
 }
 
 /** @}
-- 
2.11.4.GIT