From 5e04cdf350f9cede59263b52c2271f7c066439e3 Mon Sep 17 00:00:00 2001 From: "Luke T. Shumaker" Date: Fri, 15 Nov 2024 00:46:44 -0700 Subject: libcr: Begone with PRE_RUNNABLE --- libcr/coroutine.c | 75 ++++++++++++++++++-------------------- libcr/include/libcr/coroutine.h | 35 ++++++++---------- libcr_ipc/include/libcr_ipc/sema.h | 9 ++--- libhw/host_alarmclock.c | 8 ++-- libhw/rp2040_hwtimer.c | 17 +++++---- libhw/w5500.c | 4 +- libhw_generic/alarmclock.c | 3 ++ 7 files changed, 73 insertions(+), 78 deletions(-) diff --git a/libcr/coroutine.c b/libcr/coroutine.c index 6549070..a468df0 100644 --- a/libcr/coroutine.c +++ b/libcr/coroutine.c @@ -155,7 +155,7 @@ /*==================================================================== * Interrupt management routines. */ #if __unix__ - #include /* for sig*, SIG_* */ + #include /* for sig*, SIG* */ /* For a signal to be *in* the mask means that the signal is * *blocked*. */ @@ -166,10 +166,11 @@ sigfillset(&set); sigprocmask(SIG_SETMASK, &set, NULL); } - void _cr_plat_disable_interrupts(void) { - sigset_t all; + bool _cr_plat_save_and_disable_interrupts(void) { + sigset_t all, old; sigfillset(&all); - sigprocmask(SIG_SETMASK, &all, NULL); + sigprocmask(SIG_SETMASK, &all, &old); + return !sigismember(&old, SIGRTMIN); } void _cr_plat_enable_interrupts(void) { sigset_t zero; @@ -181,8 +182,13 @@ static ALWAYS_INLINE void cr_plat_wait_for_interrupt(void) { asm volatile ("wfi":::"memory"); } - void _cr_plat_disable_interrupts(void) { - asm volatile ("cpsid i":::"memory"); + bool _cr_plat_save_and_disable_interrupts(void) { + uint32_t primask; + asm volatile ("mrs %0, PRIMASK\n" + "cpsid i" + : /* %0 */"=r"(primask) + ); + return primask == 0; } void _cr_plat_enable_interrupts(void) { asm volatile ("cpsie i":::"memory"); @@ -273,8 +279,6 @@ enum coroutine_state { CR_NONE = 0, /* this slot in the table is empty */ CR_INITIALIZING, /* running, before cr_begin() */ CR_RUNNING, /* running, after cr_begin() */ - CR_PRE_RUNNABLE, /* running, after cr_unpause_from_intrhandler() - * but before cr_pause() */ CR_RUNNABLE, /* not running, but runnable */ CR_PAUSED, /* not running, and not runnable */ }; @@ -301,7 +305,6 @@ const char *coroutine_state_strs[] = { [CR_NONE] = "CR_NONE", [CR_INITIALIZING] = "CR_INITIALIZING", [CR_RUNNING] = "CR_RUNNING", - [CR_PRE_RUNNABLE] = "CR_PRE_RUNNABLE", [CR_RUNNABLE] = "CR_RUNNABLE", [CR_PAUSED] = "CR_PAUSED", }; @@ -472,6 +475,11 @@ cid_t coroutine_add_with_stack_size(size_t stack_size, assert_cid_state(child, state == CR_RUNNABLE); if (parent) assert_cid_state(parent, state == CR_RUNNING); + /* Restore interrupts because cr_begin() disables interrupts + * before the context switch. XXX: This assumes that + * interrupts were enabled when _add() was called, which we + * didn't actually check. */ + cr_restore_interrupts(true); coroutine_running = parent; return child; @@ -484,9 +492,10 @@ cid_t coroutine_add(const char *name, cr_fn_t fn, void *args) { /* coroutine_main() ***********************************************************/ -void coroutine_main(void) { +__attribute__ ((noreturn)) void coroutine_main(void) { debugf("coroutine_main()\n"); - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); + assert(saved); coroutine_running = 0; for (;;) { cid_t next; @@ -518,12 +527,13 @@ void cr_begin(void) { debugf("cid=%zu: cr_begin()\n", coroutine_running); assert_cid_state(coroutine_running, state == CR_INITIALIZING); + bool saved = cr_save_and_disable_interrupts(); coroutine_table[coroutine_running-1].state = CR_RUNNABLE; coroutine_ringbuf_push(coroutine_running); coroutine_table[coroutine_running-1].sp = cr_plat_get_sp(); if (!cr_plat_setjmp(coroutine_table[coroutine_running-1].env)) /* point=c1 */ cr_plat_longjmp(coroutine_add_env, 1); /* jump to point=a */ - cr_enable_interrupts(); + cr_restore_interrupts(saved); } static inline void _cr_yield() { @@ -551,37 +561,33 @@ void cr_yield(void) { debugf("cid=%zu: cr_yield()\n", coroutine_running); assert_cid_state(coroutine_running, state == CR_RUNNING); - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); coroutine_table[coroutine_running-1].state = CR_RUNNABLE; coroutine_ringbuf_push(coroutine_running); _cr_yield(); - cr_enable_interrupts(); + cr_restore_interrupts(saved); } void cr_pause_and_yield(void) { debugf("cid=%zu: cr_pause_and_yield()\n", coroutine_running); - assert_cid_state(coroutine_running, state == CR_RUNNING || state == CR_PRE_RUNNABLE); - - cr_disable_interrupts(); - if (coroutine_table[coroutine_running-1].state == CR_PRE_RUNNABLE) { - coroutine_table[coroutine_running-1].state = CR_RUNNABLE; - coroutine_ringbuf_push(coroutine_running); - } else - coroutine_table[coroutine_running-1].state = CR_PAUSED; + assert_cid_state(coroutine_running, state == CR_RUNNING); + + bool saved = cr_save_and_disable_interrupts(); + coroutine_table[coroutine_running-1].state = CR_PAUSED; _cr_yield(); - cr_enable_interrupts(); + cr_restore_interrupts(saved); } -void cr_exit(void) { +__attribute__ ((noreturn)) void cr_exit(void) { debugf("cid=%zu: cr_exit()\n", coroutine_running); assert_cid_state(coroutine_running, state == CR_RUNNING); - cr_disable_interrupts(); + (void)cr_save_and_disable_interrupts(); coroutine_table[coroutine_running-1].state = CR_NONE; cr_plat_longjmp(coroutine_main_env, 1); /* jump to point=b */ } -void cr_unpause(cid_t cid) { +void cr_unpause_from_intrhandler(cid_t cid) { debugf("cr_unpause(%zu)\n", cid); assert_cid_state(cid, state == CR_PAUSED); @@ -589,19 +595,10 @@ void cr_unpause(cid_t cid) { coroutine_ringbuf_push(cid); } -void cr_unpause_from_intrhandler(cid_t cid) { - debugf("cr_unpause_from_intrhandler(%zu)\n", cid); - assert_cid_state(cid, state == CR_RUNNING || state == CR_PAUSED); - - if (coroutine_table[cid-1].state == CR_RUNNING) { - assert(cid == coroutine_running); - debugf("... raced, deferring unpause\n"); - coroutine_table[cid-1].state = CR_PRE_RUNNABLE; - } else { - debugf("... actual unpause\n"); - coroutine_table[cid-1].state = CR_RUNNABLE; - coroutine_ringbuf_push(cid); - } +void cr_unpause(cid_t cid) { + bool saved = cr_save_and_disable_interrupts(); + cr_unpause_from_intrhandler(cid); + cr_restore_interrupts(saved); } cid_t cr_getcid(void) { diff --git a/libcr/include/libcr/coroutine.h b/libcr/include/libcr/coroutine.h index 95f9ba0..8aeab7e 100644 --- a/libcr/include/libcr/coroutine.h +++ b/libcr/include/libcr/coroutine.h @@ -94,12 +94,8 @@ cid_t coroutine_add(const char *name, cr_fn_t fn, void *args); /** * The main scheduler loop. - * - * "Should" never return, but will print a message and return if there - * are no coroutines (there were no calls to coroutine_add(), or all - * coroutines cr_exit()). */ -void coroutine_main(void); +__attribute__ ((noreturn)) void coroutine_main(void); /* inside of coroutines *******************************************************/ @@ -132,34 +128,33 @@ cid_t cr_getcid(void); * * This is fast on bare-metal, but slow on an OS (because on an OS it * uses a syscall). + * + * Returns whether interrupts were enabled before the call. */ -#define cr_disable_interrupts() do { \ - _cr_plat_disable_interrupts(); \ - asm volatile ("":::"memory"); \ - } while (0) -void _cr_plat_disable_interrupts(void); +#define cr_save_and_disable_interrupts() ({ \ + bool was_enabled = _cr_plat_save_and_disable_interrupts(); \ + asm volatile ("":::"memory"); \ + was_enabled; \ + }) +bool _cr_plat_save_and_disable_interrupts(void); /** - * Enable interrupts. Any "pending" interrupts that came in while + * Re-enable interrupts. Any "pending" interrupts that came in while * interrupts were disabled will have their handlers called. * * This is fast on bare-metal, but slow on an OS (because on an OS it * uses a syscall). */ -#define cr_enable_interrupts() do { \ - asm volatile ("":::"memory"); \ - _cr_plat_enable_interrupts(); \ +#define cr_restore_interrupts(enable) do { \ + asm volatile ("":::"memory"); \ + if (enable) \ + _cr_plat_enable_interrupts(); \ } while (0) void _cr_plat_enable_interrupts(void); /** * cr_unpause_from_intrhandler() is like cr_unpause(), but safe to - * call from a interrupt handler that might race with the coroutine - * actually pausing itself. - * - * It is also safe to call from a regular coroutine, but compared to - * regular cr_unpause() it is less capable of detecting programming - * errors. So don't do that? + * call from a interrupt handler. */ void cr_unpause_from_intrhandler(cid_t); diff --git a/libcr_ipc/include/libcr_ipc/sema.h b/libcr_ipc/include/libcr_ipc/sema.h index 9958364..0895a22 100644 --- a/libcr_ipc/include/libcr_ipc/sema.h +++ b/libcr_ipc/include/libcr_ipc/sema.h @@ -38,14 +38,14 @@ typedef struct { static inline void cr_sema_signal(cr_sema_t *sema) { assert(sema); - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); sema->cnt++; if (sema->waiters.front) { sema->cnt--; cr_unpause(sema->waiters.front->cid); _cr_ipc_sll_pop_from_front(&sema->waiters); } - cr_enable_interrupts(); + cr_restore_interrupts(saved); } /** @@ -74,18 +74,17 @@ static inline void cr_sema_signal_from_intrhandler(cr_sema_t *sema) { static inline void cr_sema_wait(cr_sema_t *sema) { assert(sema); - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); if (sema->cnt) { /* non-blocking */ sema->cnt--; - cr_enable_interrupts(); } else { /* blocking */ struct _cr_sema_waiter self = { .cid = cr_getcid(), }; _cr_ipc_sll_push_to_rear(&sema->waiters, &self); - cr_enable_interrupts(); cr_pause_and_yield(); } + cr_restore_interrupts(saved); } #endif /* _LIBCR_IPC_SEMA_H_ */ diff --git a/libhw/host_alarmclock.c b/libhw/host_alarmclock.c index 4f43fc7..1d26f1a 100644 --- a/libhw/host_alarmclock.c +++ b/libhw/host_alarmclock.c @@ -110,7 +110,7 @@ static bool hostclock_add_trigger(implements_alarmclock *_alarmclock, trigger->cb = cb; trigger->cb_arg = cb_arg; - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); struct alarmclock_trigger **dst = &alarmclock->queue; while (*dst && fire_at_ns >= (*dst)->fire_at_ns) dst = &(*dst)->next; @@ -145,7 +145,7 @@ static bool hostclock_add_trigger(implements_alarmclock *_alarmclock, if (timer_settime(alarmclock->timer_id, TIMER_ABSTIME, &alarmspec, NULL) != 0) error(1, errno, "timer_settime"); } - cr_enable_interrupts(); + cr_restore_interrupts(saved); return false; } @@ -158,7 +158,7 @@ static void hostclock_del_trigger(implements_alarmclock *_alarmclock, assert(alarmclock); assert(trigger); - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); if (trigger->alarmclock == alarmclock) { if (!trigger->prev) alarmclock->queue = trigger->next; @@ -170,5 +170,5 @@ static void hostclock_del_trigger(implements_alarmclock *_alarmclock, trigger->prev = NULL; trigger->next = NULL; } - cr_enable_interrupts(); + cr_restore_interrupts(saved); } diff --git a/libhw/rp2040_hwtimer.c b/libhw/rp2040_hwtimer.c index 6f37d85..9f0d901 100644 --- a/libhw/rp2040_hwtimer.c +++ b/libhw/rp2040_hwtimer.c @@ -115,7 +115,7 @@ static bool rp2040_hwtimer_add_trigger(implements_alarmclock *_alarmclock, trigger->cb = cb; trigger->cb_arg = cb_arg; - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); struct alarmclock_trigger **dst = &alarmclock->queue; while (*dst && fire_at_ns >= (*dst)->fire_at_ns) dst = &(*dst)->next; @@ -135,14 +135,15 @@ static bool rp2040_hwtimer_add_trigger(implements_alarmclock *_alarmclock, /* Force the interrupt handler to trigger as soon as * we enable interrupts. This handles the case of * when fire_at_ns is before when we called - * cr_disable_interrupts(). We could check + * cr_save_and_disable_interrupts(). We could check * timer_time_us_64() again after calling - * cr_disable_interrupts() and do this conditionally, - * but I don't think that would be any more efficient - * than just letting the interrupt fire. */ + * cr_save_and_disable_interrupts() and do this + * conditionally, but I don't think that would be any + * more efficient than just letting the interrupt + * fire. */ hw_set_bits(&timer_hw->intf, 1 << alarmclock->alarm_num); } - cr_enable_interrupts(); + cr_restore_interrupts(saved); return false; } @@ -154,7 +155,7 @@ static void rp2040_hwtimer_del_trigger(implements_alarmclock *_alarmclock, assert(alarmclock); assert(trigger); - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); if (trigger->alarmclock == alarmclock) { if (!trigger->prev) alarmclock->queue = trigger->next; @@ -166,5 +167,5 @@ static void rp2040_hwtimer_del_trigger(implements_alarmclock *_alarmclock, trigger->prev = NULL; trigger->next = NULL; } - cr_enable_interrupts(); + cr_restore_interrupts(saved); } diff --git a/libhw/w5500.c b/libhw/w5500.c index 08486d4..5678ffd 100644 --- a/libhw/w5500.c +++ b/libhw/w5500.c @@ -340,14 +340,14 @@ void _w5500_init(struct w5500 *chip, w5500_hard_reset(chip); /* Finally, wire in the interrupt handler. */ - cr_disable_interrupts(); + bool saved = cr_save_and_disable_interrupts(); for (size_t i = 0; i < ARRAY_LEN(w5500_chips); i++) { if (w5500_chips[i] == NULL) { w5500_chips[i] = chip; break; } } - cr_enable_interrupts(); + cr_restore_interrupts(saved); coroutine_add("w5500_irq", w5500_irq_cr, chip); } diff --git a/libhw_generic/alarmclock.c b/libhw_generic/alarmclock.c index 3364f72..a16f2f6 100644 --- a/libhw_generic/alarmclock.c +++ b/libhw_generic/alarmclock.c @@ -15,7 +15,10 @@ static void alarmclock_sleep_intrhandler(void *_arg) { } void alarmclock_sleep_until_ns(implements_alarmclock *clock, uint64_t abstime_ns) { + bool saved = cr_save_and_disable_interrupts(); cid_t cid = cr_getcid(); struct alarmclock_trigger trigger; VCALL(clock, add_trigger, &trigger, abstime_ns, alarmclock_sleep_intrhandler, &cid); + cr_pause_and_yield(); + cr_restore_interrupts(saved); } -- cgit v1.2.3-2-g168b