Code Review - Make set timer interrupt callback consistent

[Davide Libenzi]

While looking around at the timer code, I noticed that the set_interrupt callback does not

do what the caller would expect, when called with a remote timer chain.

In the local CPU timer chain mode, it actually set the timer interrupt to the time parameter passed to it, while when called remotely, it triggers an IPI, which triggers a call to a function (in the remote CPU) which ends up setting up the timer in that CPU, according to the current status of that timer chain.

It works fine today, because we happen to call that function always with the next time in the chain, so the behavior is consistent among local and remote.

IMO if the only consistently working mode to call such API is with the current next timer on the chain, it would be better to make that explicit and remove the "time" parameter (which is ignored in the remote case) altogether.

Here is a small CL for that.

https://github.com/brho/akaros/compare/staging...dlibenzi:set_timer_int_fix

The following changes since commit 2a9b3cdc47dbde55f9d125fde3e11832ca4c0b91:

  Avoid double declarations, integer overflow, and use branch hints (2015-10-30 16:02:29 -0400)

are available in the git repository at:

  g...@github.com:dlibenzi/akaros set_timer_int_fix

for you to fetch changes up to e675df195938da0d759cb7bfbcdd2ea2b5015dd4:

  Made the timer interrupt setup callback consistent in behavior (2015-10-31 16:02:18 -0700)

----------------------------------------------------------------
Davide Libenzi (1):
      Made the timer interrupt setup callback consistent in behavior

 kern/include/alarm.h |  8 ++++----
 kern/src/alarm.c     | 23 +++++++++++++----------
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/kern/include/alarm.h b/kern/include/alarm.h
index f65f7bc..04752c3 100644
--- a/kern/include/alarm.h
+++ b/kern/include/alarm.h
@@ -106,12 +106,12 @@ struct timer_chain {
 	struct awaiters_tailq		waiters;
 	uint64_t					earliest_time;
 	uint64_t					latest_time;
-	void (*set_interrupt) (uint64_t time, struct timer_chain *);
+	void (*set_interrupt) (struct timer_chain *);
 };
 
 /* Called once per timer chain, currently in per_cpu_init() */
 void init_timer_chain(struct timer_chain *tchain,
-                      void (*set_interrupt) (uint64_t, struct timer_chain *));
+                      void (*set_interrupt) (struct timer_chain *));
 /* For fresh alarm waiters.  func == 0 for kthreads */
 void init_awaiter(struct alarm_waiter *waiter,
                   void (*func) (struct alarm_waiter *));
@@ -134,8 +134,8 @@ bool reset_alarm_rel(struct timer_chain *tchain, struct alarm_waiter *waiter,
 int sleep_on_awaiter(struct alarm_waiter *waiter);
 /* Interrupt handlers need to call this.  Don't call it directly. */
 void __trigger_tchain(struct timer_chain *tchain, struct hw_trapframe *hw_tf);
-/* How to set a specific alarm: the per-cpu timer interrupt */
-void set_pcpu_alarm_interrupt(uint64_t time, struct timer_chain *tchain);
+/* Sets the timer chain interrupt according to the next timer in the chain. */
+void set_pcpu_alarm_interrupt(struct timer_chain *tchain);
 
 /* Debugging */
 #define ALARM_POISON_TIME 12345				/* could use some work */
diff --git a/kern/src/alarm.c b/kern/src/alarm.c
index a10ebfc..55f1209 100644
--- a/kern/src/alarm.c
+++ b/kern/src/alarm.c
@@ -38,7 +38,7 @@ static void reset_tchain_times(struct timer_chain *tchain)
 
 /* One time set up of a tchain, currently called in per_cpu_init() */
 void init_timer_chain(struct timer_chain *tchain,
-                      void (*set_interrupt) (uint64_t, struct timer_chain *))
+                      void (*set_interrupt) (struct timer_chain *))
 {
 	spinlock_init_irqsave(&tchain->lock);
 	TAILQ_INIT(&tchain->waiters);
@@ -113,13 +113,13 @@ static void reset_tchain_interrupt(struct timer_chain *tchain)
 	if (TAILQ_EMPTY(&tchain->waiters)) {
 		/* Turn it off */
 		printd("Turning alarm off\n");
-		tchain->set_interrupt(0, tchain);
+		tchain->set_interrupt(tchain);
 	} else {
 		/* Make sure it is on and set to the earliest time */
 		assert(tchain->earliest_time != ALARM_POISON_TIME);
 		/* TODO: check for times in the past or very close to now */
 		printd("Turning alarm on for %llu\n", tchain->earliest_time);
-		tchain->set_interrupt(tchain->earliest_time, tchain);
+		tchain->set_interrupt(tchain);
 	}
 }
 
@@ -364,11 +364,13 @@ int sleep_on_awaiter(struct alarm_waiter *waiter)
 	return 0;
 }
 
-/* Sets the Alarm interrupt, per-core style.  Also is an example of what any
- * similar function needs to do (this is the func ptr in the tchain). 
- * Note the tchain is our per-core one, and we don't need tchain passed to us to
- * figure that out.  It's kept around in case other tchain-usage wants it -
- * might not be necessary in the future.
+/* Sets the timer interrupt for the timer chain passed as parameter.
+ * The next interrupt will be scheduled at the nearest timer available in the
+ * chain.
+ * This function can be called either for the local CPU, or for a remote CPU.
+ * If called for the local CPU, it proceeds in setting up the local timer,
+ * otherwise it will trigger an IPI, and will let the remote CPU IRQ handler
+ * to setup the timer according to the active information on its timer chain.
  *
  * Needs to set the interrupt to trigger tchain at the given time, or disarm it
  * if time is 0.   Any function like this needs to do a few things:
@@ -381,9 +383,9 @@ int sleep_on_awaiter(struct alarm_waiter *waiter)
  * Called with the tchain lock held, and IRQs disabled.  However, we could be
  * calling this cross-core, and we cannot disable those IRQs (hence the
  * locking). */
-void set_pcpu_alarm_interrupt(uint64_t time, struct timer_chain *tchain)
+void set_pcpu_alarm_interrupt(struct timer_chain *tchain)
 {
-	uint64_t rel_usec, now;
+	uint64_t time, rel_usec, now;
 	int pcoreid = core_id();
 	struct per_cpu_info *rem_pcpui, *pcpui = &per_cpu_info[pcoreid];
 	struct timer_chain *pcpui_tchain = &pcpui->tchain;
@@ -400,6 +402,7 @@ void set_pcpu_alarm_interrupt(uint64_t time, struct timer_chain *tchain)
 		send_ipi(rem_pcpui - &per_cpu_info[0], IdtLAPIC_TIMER);
 		return;
 	}
+	time = TAILQ_EMPTY(&tchain->waiters) ? 0: tchain->earliest_time;
 	if (time) {
 		/* Arm the alarm.  For times in the past, we just need to make sure it
 		 * goes off. */

[Barret Rhoden]

Thanks, merged to staging at 1165c2bda44b..df2c47c3a7f3 (from, to]

You can see the entire diff with 'git diff' or at
https://github.com/brho/akaros/compare/1165c2bda44b...df2c47c3a7f3

Barret

On 2015-10-31 at 16:33 "'Davide Libenzi' via Akaros"