From 8dad54c914453957e78db4c4f42785aa1a22f0ab Mon Sep 17 00:00:00 2001 From: Martin Stein Date: Fri, 20 Jun 2014 17:44:09 +0200 Subject: [PATCH] hw: fix scheduler timing on prio preemption Previously, the timer was used to remember the state of the time slices. This was sufficient before priorities entered the scene as a thread always received a fresh time slice when he was scheduled away. However, with priorities this isn't always the case. A thread can be preempted by another thread due to a higher priority. In this case the low-priority thread must remember how much time he has consumed from its current time slice because the timer gets re-programmed. Otherwise, if we have high-priority threads that block and unblock with high frequency, the head of the next lower priority would start with a fresh time slice all the time and is never superseded. fix #1287 --- .../src/core/include/kernel/processor.h | 68 ++++++++++--------- .../src/core/include/kernel/scheduler.h | 29 +++++--- .../src/core/include/spec/cortex_a9/timer.h | 16 +++-- .../src/core/include/spec/exynos5/timer.h | 27 +------- .../base-hw/src/core/include/spec/rpi/timer.h | 35 ++++------ repos/base-hw/src/core/kernel/processor.cc | 61 +++++++++++++++++ repos/base/include/drivers/timer/epit_base.h | 35 +++++++--- 7 files changed, 166 insertions(+), 105 deletions(-) diff --git a/repos/base-hw/src/core/include/kernel/processor.h b/repos/base-hw/src/core/include/kernel/processor.h index d99adfd79..8af8def8f 100644 --- a/repos/base-hw/src/core/include/kernel/processor.h +++ b/repos/base-hw/src/core/include/kernel/processor.h @@ -96,6 +96,8 @@ class Kernel::Processor_client : public Processor_scheduler::Item Processor * _processor; Cpu_lazy_state _lazy_state; + unsigned _tics_consumed; + /** * Handle an interrupt exception that occured during execution * @@ -143,7 +145,8 @@ class Kernel::Processor_client : public Processor_scheduler::Item Processor_client(Processor * const processor, Priority const priority) : Processor_scheduler::Item(priority), - _processor(processor) + _processor(processor), + _tics_consumed(0) { } /** @@ -155,12 +158,30 @@ class Kernel::Processor_client : public Processor_scheduler::Item _unschedule(); } + /** + * Update how many tics the client consumed from its current time slice + * + * \param tics_left tics that aren't consumed yet at the slice + * \param tics_per_slice tics that the slice provides + */ + void update_tics_consumed(unsigned const tics_left, + unsigned const tics_per_slice) + { + unsigned const old_tics_left = tics_per_slice - _tics_consumed; + _tics_consumed += old_tics_left - tics_left; + } + + /** + * Reset how many tics the client consumed from its current time slice + */ + void reset_tics_consumed() { _tics_consumed = 0; } /*************** ** Accessors ** ***************/ Cpu_lazy_state * lazy_state() { return &_lazy_state; } + unsigned tics_consumed() { return _tics_consumed; } }; class Kernel::Processor : public Cpu @@ -172,13 +193,18 @@ class Kernel::Processor : public Cpu bool _ip_interrupt_pending; Timer * const _timer; - /** - * Reset the scheduling timer for a new scheduling interval - */ - void _reset_timer() + void _start_timer(unsigned const tics) { + _timer->start_one_shot(tics, _id); } + + unsigned _tics_per_slice() { + return _timer->ms_to_tics(USER_LAP_TIME_MS); } + + void _update_timer(unsigned const tics_consumed, + unsigned const tics_per_slice) { - unsigned const tics = _timer->ms_to_tics(USER_LAP_TIME_MS); - _timer->start_one_shot(tics, _id); + assert(tics_consumed <= tics_per_slice); + if (tics_consumed >= tics_per_slice) { _start_timer(1); } + else { _start_timer(tics_per_slice - tics_consumed); } } public: @@ -200,7 +226,7 @@ class Kernel::Processor : public Cpu /** * Initializate on the processor that this object corresponds to */ - void init_processor_local() { _reset_timer(); } + void init_processor_local() { _update_timer(0, _tics_per_slice()); } /** * Check for a scheduling timeout and handle it in case @@ -213,7 +239,6 @@ class Kernel::Processor : public Cpu { if (_timer->interrupt_id(_id) != interrupt_id) { return false; } _scheduler.yield_occupation(); - _timer->clear_interrupt(_id); return true; } @@ -237,30 +262,7 @@ class Kernel::Processor : public Cpu /** * Handle exception of the processor and proceed its user execution */ - void exception() - { - /* - * Request the current occupant without any update. While the - * processor was outside the kernel, another processor may have changed the - * scheduling of the local activities in a way that an update would return - * an occupant other than that whose exception caused the kernel entry. - */ - Processor_client * const old_client = _scheduler.occupant(); - Cpu_lazy_state * const old_state = old_client->lazy_state(); - old_client->exception(_id); - - /* - * The processor local as well as remote exception-handling may have - * changed the scheduling of the local activities. Hence we must update the - * occupant. - */ - bool update; - Processor_client * const new_client = _scheduler.update_occupant(update); - if (update) { _reset_timer(); } - Cpu_lazy_state * const new_state = new_client->lazy_state(); - prepare_proceeding(old_state, new_state); - new_client->proceed(_id); - } + void exception(); /*************** ** Accessors ** diff --git a/repos/base-hw/src/core/include/kernel/scheduler.h b/repos/base-hw/src/core/include/kernel/scheduler.h index 56e63fd84..75cfdc2a4 100644 --- a/repos/base-hw/src/core/include/kernel/scheduler.h +++ b/repos/base-hw/src/core/include/kernel/scheduler.h @@ -116,7 +116,7 @@ class Kernel::Scheduler Double_list _items[Priority::MAX + 1]; bool _yield; - bool _check_update(T * const occupant) + bool _does_update(T * const occupant) { if (_yield) { _yield = false; @@ -138,18 +138,29 @@ class Kernel::Scheduler /** * Adjust occupant reference to the current scheduling plan * - * \return updated occupant reference + * \param updated true on return if the occupant has changed/yielded + * \param refreshed true on return if the occupant got a new timeslice + * + * \return updated occupant */ - T * update_occupant(bool & update) + T * update_occupant(bool & updated, bool & refreshed) { for (int i = Priority::MAX; i >= 0 ; i--) { - T * const head = _items[i].head(); - if (!head) { continue; } - update = _check_update(head); - _occupant = head; - return head; + T * const new_occupant = _items[i].head(); + if (!new_occupant) { continue; } + updated = _does_update(new_occupant); + T * const old_occupant = _occupant; + if (!old_occupant) { refreshed = true; } + else { + unsigned const new_prio = new_occupant->priority(); + unsigned const old_prio = old_occupant->priority(); + refreshed = new_prio <= old_prio; + } + _occupant = new_occupant; + return new_occupant; } - update = _check_update(_idle); + updated = _does_update(_idle); + refreshed = true; _occupant = 0; return _idle; } diff --git a/repos/base-hw/src/core/include/spec/cortex_a9/timer.h b/repos/base-hw/src/core/include/spec/cortex_a9/timer.h index 1ad399824..22a02e4a3 100644 --- a/repos/base-hw/src/core/include/spec/cortex_a9/timer.h +++ b/repos/base-hw/src/core/include/spec/cortex_a9/timer.h @@ -34,6 +34,11 @@ namespace Genode */ struct Load : Register<0x0, 32> { }; + /** + * Counter value register + */ + struct Counter : Register<0x4, 32> { }; + /** * Timer control register */ @@ -51,17 +56,14 @@ namespace Genode struct Event : Bitfield<0,1> { }; /* if counter hit zero */ }; - void _clear_interrupt() { write(1); } - public: /** - * Constructor, clears the interrupt output + * Constructor */ Timer() : Mmio(Cpu::PRIVATE_TIMER_MMIO_BASE) { write(0); - _clear_interrupt(); } /** @@ -80,7 +82,7 @@ namespace Genode inline void start_one_shot(unsigned const tics, unsigned) { /* reset timer */ - _clear_interrupt(); + write(1); Control::access_t control = 0; Control::Irq_enable::set(control, 1); write(control); @@ -99,9 +101,9 @@ namespace Genode } /** - * Clear interrupt output line + * Return current native timer value */ - void clear_interrupt(unsigned) { _clear_interrupt(); } + unsigned value(unsigned const) { return read(); } }; } diff --git a/repos/base-hw/src/core/include/spec/exynos5/timer.h b/repos/base-hw/src/core/include/spec/exynos5/timer.h index 597872756..78e9aa114 100644 --- a/repos/base-hw/src/core/include/spec/exynos5/timer.h +++ b/repos/base-hw/src/core/include/spec/exynos5/timer.h @@ -228,11 +228,13 @@ class Genode::Timer : public Mmio { switch (processor_id) { case 0: + write(1); _run_0(0); _acked_write(tics); _run_0(1); return; case 1: + write(1); _run_1(0); _acked_write(tics); _run_1(1); @@ -246,22 +248,6 @@ class Genode::Timer : public Mmio */ unsigned ms_to_tics(unsigned const ms) { return ms * _tics_per_ms; } - /** - * Clear interrupt output line - */ - void clear_interrupt(unsigned const processor_id) - { - switch (processor_id) { - case 0: - write(1); - return; - case 1: - write(1); - return; - default: return; - } - } - unsigned value(unsigned const processor_id) { switch (processor_id) { @@ -270,15 +256,6 @@ class Genode::Timer : public Mmio default: return 0; } } - - unsigned irq_state(unsigned const processor_id) - { - switch (processor_id) { - case 0: return read(); - case 1: return read(); - default: return 0; - } - } }; namespace Kernel { class Timer : public Genode::Timer { }; } diff --git a/repos/base-hw/src/core/include/spec/rpi/timer.h b/repos/base-hw/src/core/include/spec/rpi/timer.h index 4374d22bf..955269df5 100644 --- a/repos/base-hw/src/core/include/spec/rpi/timer.h +++ b/repos/base-hw/src/core/include/spec/rpi/timer.h @@ -24,6 +24,8 @@ namespace Genode { /** * Timer driver for core + * + * Timer channel 0 apparently doesn't work on the RPI, so we use channel 1 */ class Timer; } @@ -32,16 +34,7 @@ class Genode::Timer : public Mmio { private: - /* - * The timer channel 0 apparently does not work on the Raspberry Pi. - * So we use channel 1. - */ - - struct Cs : Register<0x0, 32> - { - struct Status : Bitfield<1, 1> { }; - }; - + struct Cs : Register<0x0, 32> { struct M1 : Bitfield<1, 1> { }; }; struct Clo : Register<0x4, 32> { }; struct Cmp : Register<0x10, 32> { }; @@ -49,28 +42,30 @@ class Genode::Timer : public Mmio Timer() : Mmio(Board::SYSTEM_TIMER_MMIO_BASE) { } - static unsigned interrupt_id(unsigned) - { - return Board::SYSTEM_TIMER_IRQ; - } + static unsigned interrupt_id(int) { return Board::SYSTEM_TIMER_IRQ; } inline void start_one_shot(uint32_t const tics, unsigned) { write(0); write(read() + tics); - write(1); + write(1); } - static uint32_t ms_to_tics(unsigned const ms) - { - return (Board::SYSTEM_TIMER_CLOCK / 1000) * ms; - } + static uint32_t ms_to_tics(unsigned const ms) { + return (Board::SYSTEM_TIMER_CLOCK / 1000) * ms; } void clear_interrupt(unsigned) { - write(1); + write(1); read(); } + + unsigned value(unsigned) + { + Cmp::access_t const cmp = read(); + Clo::access_t const clo = read(); + return cmp > clo ? cmp - clo : 0; + } }; namespace Kernel { class Timer : public Genode::Timer { }; } diff --git a/repos/base-hw/src/core/kernel/processor.cc b/repos/base-hw/src/core/kernel/processor.cc index 49ba2460f..72afa12a0 100644 --- a/repos/base-hw/src/core/kernel/processor.cc +++ b/repos/base-hw/src/core/kernel/processor.cc @@ -190,3 +190,64 @@ bool Kernel::Processor_domain_update::_perform(unsigned const domain_id) } return true; } + + +void Kernel::Processor::exception() +{ + /* + * Request the current occupant without any update. While the + * processor was outside the kernel, another processor may have changed the + * scheduling of the local activities in a way that an update would return + * an occupant other than that whose exception caused the kernel entry. + */ + Processor_client * const old_client = _scheduler.occupant(); + Cpu_lazy_state * const old_state = old_client->lazy_state(); + old_client->exception(_id); + + /* + * The processor local as well as remote exception-handling may have + * changed the scheduling of the local activities. Hence we must update the + * occupant. + */ + bool updated, refreshed; + Processor_client * const new_client = + _scheduler.update_occupant(updated, refreshed); + + /** + * There are three scheduling situations we have to deal with: + * + * The client has not changed and didn't yield: + * + * The client must not update its time-slice state as the timer + * can continue as is and hence keeps providing the information. + * + * The client has changed or did yield and the previous client + * received a fresh timeslice: + * + * The previous client can reset his time-slice state. + * The timer must be re-programmed according to the time-slice + * state of the new client. + * + * The client has changed and the previous client did not receive + * a fresh timeslice: + * + * The previous client must update its time-slice state. The timer + * must be re-programmed according to the time-slice state of the + * new client. + */ + if (updated) { + unsigned const tics_per_slice = _tics_per_slice(); + if (refreshed) { old_client->reset_tics_consumed(); } + else { + unsigned const tics_left = _timer->value(_id); + old_client->update_tics_consumed(tics_left, tics_per_slice); + } + _update_timer(new_client->tics_consumed(), tics_per_slice); + } + /** + * Apply the CPU state of the new client and continue his execution + */ + Cpu_lazy_state * const new_state = new_client->lazy_state(); + prepare_proceeding(old_state, new_state); + new_client->proceed(_id); +} diff --git a/repos/base/include/drivers/timer/epit_base.h b/repos/base/include/drivers/timer/epit_base.h index bc5120edb..a3e73664f 100644 --- a/repos/base/include/drivers/timer/epit_base.h +++ b/repos/base/include/drivers/timer/epit_base.h @@ -114,7 +114,9 @@ namespace Genode /* disable timer */ write(0); - clear_interrupt(0); + + /* clear interrupt */ + write(1); } void _start_one_shot(unsigned const tics) @@ -157,21 +159,32 @@ namespace Genode { /* disable timer */ write(0); - - /* if the timer has hit zero already return 0 */ - return read() ? 0 : read(); + return value(0); } - /** - * Clear interrupt output line - */ - void clear_interrupt(unsigned) { write(1); } - /** * Translate milliseconds to a native timer value */ - static unsigned ms_to_tics(unsigned const ms) { - return TICS_PER_MS * ms; } + unsigned ms_to_tics(unsigned const ms) + { + return TICS_PER_MS * ms; + } + + /** + * Translate native timer value to milliseconds + */ + unsigned tics_to_ms(unsigned const tics) + { + return tics / TICS_PER_MS; + } + + /** + * Return current native timer value + */ + unsigned value(unsigned const) + { + return read() ? 0 : read(); + } }; }