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" <[email protected]> wrote: > 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: > > [email protected]: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. */ > -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
