Skip to content

Update idle loop to reduce calls to suspend #6534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 25, 2018
Merged

Update idle loop to reduce calls to suspend #6534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
953bbeb
Update idle loop to reduce calls to suspend
c1728p9 Mar 27, 2018
044eb1e
Update the SysTimer test
c1728p9 Apr 10, 2018
20013d7
RTOS: SysTimer: Update tests
fkjagodzinski Apr 12, 2018
ec59cbb
RTOS: SysTimer: Fix test compilation error on ARM & IAR
fkjagodzinski Apr 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 84 additions & 13 deletions TESTS/mbedmicro-rtos-mbed/systimer/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "greentea-client/test_env.h"
#include "unity.h"
#include "utest.h"
#include "ticker_api.h"

extern "C" {
#include "rtx_lib.h"
Expand All @@ -44,7 +45,9 @@ class SysTimerTest: public rtos::internal::SysTimer {
Semaphore _sem;
virtual void handler()
{
increment_tick();
core_util_critical_section_enter();
_increment_tick();
core_util_critical_section_exit();
_sem.release();
}

Expand All @@ -54,6 +57,11 @@ class SysTimerTest: public rtos::internal::SysTimer {
{
}

SysTimerTest(const ticker_data_t *data) :
SysTimer(data), _sem(0, 1)
{
}

virtual ~SysTimerTest()
{
}
Expand All @@ -64,6 +72,65 @@ class SysTimerTest: public rtos::internal::SysTimer {
}
};

timestamp_t mock_ticker_timestamp;

void mock_ticker_init()
{
}

uint32_t mock_ticker_read()
{
return mock_ticker_timestamp;
}

void mock_ticker_disable_interrupt()
{
}

void mock_ticker_clear_interrupt()
{
}

void mock_ticker_set_interrupt(timestamp_t timestamp)
{
}

void mock_ticker_fire_interrupt()
{
}

const ticker_info_t *mock_ticker_get_info()
{
static const ticker_info_t mock_ticker_info = {
.frequency = 1000000,
.bits = 32
};
return &mock_ticker_info;
}

ticker_interface_t mock_ticker_interface = {
.init = mock_ticker_init,
.read = mock_ticker_read,
.disable_interrupt = mock_ticker_disable_interrupt,
.clear_interrupt = mock_ticker_clear_interrupt,
.set_interrupt = mock_ticker_set_interrupt,
.fire_interrupt = mock_ticker_fire_interrupt,
.get_info = mock_ticker_get_info,
};

ticker_event_queue_t mock_ticker_event_queue;

const ticker_data_t mock_ticker_data = {
.interface = &mock_ticker_interface,
.queue = &mock_ticker_event_queue
};

void mock_ticker_reset()
{
mock_ticker_timestamp = 0;
memset(&mock_ticker_event_queue, 0, sizeof mock_ticker_event_queue);
}

/** Test tick count is zero upon creation
*
* Given a SysTimer
Expand All @@ -79,26 +146,29 @@ void test_created_with_zero_tick_count(void)
/** Test tick count is updated correctly
*
* Given a SysTimer
* When @a update_tick method is called immediately after creation
* When the @a suspend and @a resume methods are called immediately after creation
* Then the tick count is not updated
* When @a update_tick is called again after a delay
* When @a suspend and @a resume methods are called again after a delay
* Then the tick count is updated
* and the number of ticks incremented is equal TEST_TICKS - 1
* When @a update_tick is called again without a delay
* When @a suspend and @a resume methods are called again without a delay
* Then the tick count is not updated
*/
void test_update_tick(void)
{
SysTimerTest st;
TEST_ASSERT_EQUAL_UINT32(0, st.update_tick());
mock_ticker_reset();
SysTimerTest st(&mock_ticker_data);
st.suspend(TEST_TICKS * 2);
TEST_ASSERT_EQUAL_UINT32(0, st.resume());
TEST_ASSERT_EQUAL_UINT32(0, st.get_tick());
us_timestamp_t test_ticks_elapsed_ts = st.get_time() + DELAY_US;

while (st.get_time() <= test_ticks_elapsed_ts) {}
TEST_ASSERT_EQUAL_UINT32(TEST_TICKS - 1, st.update_tick());
st.suspend(TEST_TICKS * 2);
mock_ticker_timestamp = DELAY_US;
TEST_ASSERT_EQUAL_UINT32(TEST_TICKS - 1, st.resume());
TEST_ASSERT_EQUAL_UINT32(TEST_TICKS - 1, st.get_tick());

TEST_ASSERT_EQUAL_UINT32(0, st.update_tick());
st.suspend(TEST_TICKS * 2);
TEST_ASSERT_EQUAL_UINT32(0, st.resume());
TEST_ASSERT_EQUAL_UINT32(TEST_TICKS - 1, st.get_tick());
}

Expand All @@ -110,12 +180,13 @@ void test_update_tick(void)
*/
void test_get_time(void)
{
SysTimerTest st;
mock_ticker_reset();
SysTimerTest st(&mock_ticker_data);
us_timestamp_t t1 = st.get_time();

wait_us(DELAY_US);
mock_ticker_timestamp = DELAY_US;
us_timestamp_t t2 = st.get_time();
TEST_ASSERT_UINT64_WITHIN(DELAY_DELTA_US, DELAY_US, t2 - t1);
TEST_ASSERT_EQUAL_UINT64(DELAY_US, t2 - t1);
}

/** Test cancel_tick
Expand Down
79 changes: 66 additions & 13 deletions rtos/TARGET_CORTEX/SysTimer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#if DEVICE_LOWPOWERTIMER

#include "hal/lp_ticker_api.h"
#include "mbed_critical.h"
#include "rtx_core_cm.h"
extern "C" {
#include "rtx_lib.h"
Expand All @@ -43,6 +44,14 @@ namespace internal {

SysTimer::SysTimer() :
TimerEvent(get_lp_ticker_data()), _start_time(0), _tick(0)
{
_start_time = ticker_read_us(_ticker_data);
_suspend_time_passed = true;
_suspended = false;
}

SysTimer::SysTimer(const ticker_data_t *data) :
TimerEvent(data), _start_time(0), _tick(0)
{
_start_time = ticker_read_us(_ticker_data);
}
Expand All @@ -61,23 +70,30 @@ void SysTimer::setup_irq()
#endif
}

void SysTimer::schedule_tick(uint32_t delta)
void SysTimer::suspend(uint32_t ticks)
{
insert_absolute(_start_time + (_tick + delta) * 1000000ULL / OS_TICK_FREQ);
}
core_util_critical_section_enter();

void SysTimer::cancel_tick()
{
remove();
schedule_tick(ticks);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@c1728p9 Why is there no remove() call here? I'm seeing issues on my system because of this.

The SysTimer is based on a TimerEvent class, which has a static member for the timer's event data struct. Additionally, the call to suspend may happen before the previously set timer expires (for example when all threads go idle and the idle handler gets activated). At that point, the timer event data is still present in the timer queue. Calling schedule_tick here will insert the same event structure again in the queue. And since the queue is by reference, that'll lead to a recursive timer that breaks the system.

Copy link
Contributor Author

@c1728p9 c1728p9 May 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @stevew817 I gave this a try but was unable to reproduce the failure. Do you have example code which reproduces the issue?

From both code analysis and running the code can I cant find a sequence which causes this. The basic flow of the idle loop should unconditionally call remove before each insert as indicated below:

while (true) {
    osKernelSuspend();      // removes event
    os_timer->suspend();    // inserts event
    sleep();
    os_timer->resume();     // removes event
    osKernelResume();       // inserts event
}

With a debugger I can confirm the following sequence is being met by setting a breakpoint in TimerEvent::remove, TimerEvent::insert and TimerEvent::insert_absolute. Below is the callstack from each:

osKernelSuspend()
    svcRtxKernelSuspend()
        KernelBlock()
            OS_Tick_Disable()
                os_timer->cancel_tick()
                    remove()

os_timer->suspend()
    schedule_tick()
        insert_absolute()

os_timer->resume()
    remove()

osKernelResume()
    svcRtxKernelResume()
        KernelUnblock()
            OS_Tick_Enable()
                schedule_tick()
                    insert_absolute();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran across this when running cloud-client-example using Thread as a sleepy node in tickless mode. The code would hang, and attaching a debugger confirmed it was stuck in the tick handler IRQ because somehow the tick event had managed to add itself recursively (the next pointer of the tick event data was pointing to the same event data, causing the remove call to not do anything since the head just always ands up being the tick event data).

I fixed the issue by adding a cancel_tick() call before the schedule_ticks call in SysTimer::suspend. I didn't investigate further after solving my issue, so I'm afraid I don't have a reproducer ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevew817 I'm still unable to reproduce this problem, but I created #7027 just to be safe. Could you provide further details on how to replicate this failure (such as git shas used in cloud-client-example and mbed-os along with config, any local mods you made and the hardware you were using) or an example application?

_suspend_time_passed = false;
_suspended = true;

core_util_critical_section_exit();
}

uint32_t SysTimer::get_tick()
bool SysTimer::suspend_time_passed()
{
return _tick & 0xFFFFFFFF;
return _suspend_time_passed;
}

uint32_t SysTimer::update_tick()
uint32_t SysTimer::resume()
{
core_util_critical_section_enter();

_suspended = false;
_suspend_time_passed = true;
remove();

uint64_t new_tick = (ticker_read_us(_ticker_data) - _start_time) * OS_TICK_FREQ / 1000000;
if (new_tick > _tick) {
// Don't update to the current tick. Instead, update to the
Expand All @@ -88,9 +104,34 @@ uint32_t SysTimer::update_tick()
}
uint32_t elapsed_ticks = new_tick - _tick;
_tick = new_tick;

core_util_critical_section_exit();
return elapsed_ticks;
}

void SysTimer::schedule_tick(uint32_t delta)
{
core_util_critical_section_enter();

insert_absolute(_start_time + (_tick + delta) * 1000000ULL / OS_TICK_FREQ);

core_util_critical_section_exit();
}

void SysTimer::cancel_tick()
{
core_util_critical_section_enter();

remove();

core_util_critical_section_exit();
}

uint32_t SysTimer::get_tick()
{
return _tick & 0xFFFFFFFF;
}

us_timestamp_t SysTimer::get_time()
{
return ticker_read_us(_ticker_data);
Expand All @@ -100,24 +141,36 @@ SysTimer::~SysTimer()
{
}

void SysTimer::set_irq_pending()
void SysTimer::_set_irq_pending()
{
// Protected function synchronized externally

#if (defined(NO_SYSTICK))
NVIC_SetPendingIRQ(mbed_get_m0_tick_irqn());
#else
SCB->ICSR = SCB_ICSR_PENDSTSET_Msk;
#endif
}

void SysTimer::increment_tick()
void SysTimer::_increment_tick()
{
// Protected function synchronized externally

_tick++;
}

void SysTimer::handler()
{
set_irq_pending();
increment_tick();
core_util_critical_section_enter();

if (_suspended) {
_suspend_time_passed = true;
} else {
_set_irq_pending();
_increment_tick();
}

core_util_critical_section_exit();
}

}
Expand Down
46 changes: 33 additions & 13 deletions rtos/TARGET_CORTEX/SysTimer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,42 @@ class SysTimer: private mbed::TimerEvent, private mbed::NonCopyable<SysTimer> {
public:

SysTimer();
SysTimer(const ticker_data_t *data);
virtual ~SysTimer();

/**
* Enable an IRQ/SysTick with the correct priority.
*/
static void setup_irq();

/**
* Set wakeup time and schedule a wakeup event after delta ticks
*
* After suspend has been called the function suspend_time_passed
* can be used to determine if the suspend time has passed.
*
* @param delta Ticks to remain suspended
*/
void suspend(uint32_t delta);

/**
* Check if the suspend time has passed
*
* @return true if the specified number of ticks has passed otherwise false
*/
bool suspend_time_passed();

/**
* Exit suspend mode and return elapsed ticks
*
* Due to a scheduling issue, the number of ticks returned is decremented
* by 1 so that a handler can be called and update to the current value.
* This allows scheduling restart successfully after the OS is resumed.
*
* @return the number of elapsed ticks minus 1
*/
uint32_t resume();

/**
* Schedule an os tick to fire
*
Expand All @@ -77,17 +106,6 @@ class SysTimer: private mbed::TimerEvent, private mbed::NonCopyable<SysTimer> {
*/
uint32_t get_tick();

/**
* Update the internal tick count
*
* @return The number of ticks incremented
*
* @note Due to a scheduling issue, the number of ticks returned is decremented
* by 1 so that a handler can be called and update to the current value.
* This allows scheduling restart successfully after the OS is resumed.
*/
uint32_t update_tick();

/**
* Get the time
*
Expand All @@ -97,10 +115,12 @@ class SysTimer: private mbed::TimerEvent, private mbed::NonCopyable<SysTimer> {

protected:
virtual void handler();
void increment_tick();
static void set_irq_pending();
void _increment_tick();
static void _set_irq_pending();
us_timestamp_t _start_time;
uint64_t _tick;
bool _suspend_time_passed;
bool _suspended;
};

/**
Expand Down
Loading