kern/timer: improve access synchronization

3 files changed, 24 insertions, 12 deletions

diff --git a/kern/timer.c b/kern/timer.c
index 365b2fa4..ba148d5f 100644
--- a/kern/timer.c
+++ b/kern/timer.c
@@ -76,9 +76,6 @@ struct timer_bucket {
  * The hash table bucket matching the last time member has already been
  * processed, and the next periodic event resumes from the next bucket.
  *
- * The cpu member is used to determine which lock serializes access to
- * the structure. It must be accessed atomically.
- *
  * Locking order: interrupts -> timer_cpu_data.
  */
 struct timer_cpu_data {
@@ -242,13 +239,13 @@ timer_set_canceled(struct timer *timer)
 static void
 timer_set_time(struct timer *timer, uint64_t ticks)
 {
-    timer->ticks = ticks;
+    atomic_store(&timer->ticks, ticks, ATOMIC_RELAXED);
 }
 
 static bool
 timer_occurred(const struct timer *timer, uint64_t ref)
 {
-    return clock_time_occurred(timer->ticks, ref);
+    return clock_time_occurred(timer_get_time(timer), ref);
 }
 
 static uintptr_t
diff --git a/kern/timer.h b/kern/timer.h
index 46616686..a082c538 100644
--- a/kern/timer.h
+++ b/kern/timer.h
@@ -23,6 +23,7 @@
 
 #include <stdint.h>
 
+#include <kern/atomic.h>
 #include <kern/init.h>
 
 /*
@@ -47,7 +48,7 @@ typedef void (*timer_fn_t)(struct timer *);
 static inline uint64_t
 timer_get_time(const struct timer *timer)
 {
-    return timer->ticks; /* TODO atomic */
+    return atomic_load(&timer->ticks, ATOMIC_RELAXED);
 }
 
 /*
diff --git a/kern/timer_i.h b/kern/timer_i.h
index 4ed01f22..40e97f1b 100644
--- a/kern/timer_i.h
+++ b/kern/timer_i.h
@@ -24,18 +24,32 @@
 #include <kern/hlist.h>
 #include <kern/work.h>
 
+/*
+ * Locking keys :
+ * (c) cpu_data
+ * (a) atomic
+ *
+ * (*) The ticks member represents the expiration date. It may be read without
+ * locking the timer, in which case it must be accessed atomically. It
+ * may only be updated when the timer is locked though, so reads at such
+ * times don't need to be atomic.
+ *
+ * (**) The cpu member is used to determine which lock serializes access to
+ * the structure. It must be accessed atomically, but updated while the
+ * timer is locked.
+ */
 struct timer {
     union {
-        struct hlist_node node;
+        struct hlist_node node; /* (c)      */
         struct work work;
     };
 
-    uint64_t ticks;
+    uint64_t ticks;             /* (c,a,*)  */
     timer_fn_t fn;
-    unsigned int cpu;
-    unsigned short state;
-    unsigned short flags;
-    struct thread *joiner;
+    unsigned int cpu;           /* (c,a,**) */
+    unsigned short state;       /* (c)      */
+    unsigned short flags;       /* (c)      */
+    struct thread *joiner;      /* (c)      */
 };
 
 #endif /* _KERN_TIMER_I_H */