Skip to content

dlist API enhancements supporting timer cleanup and scheduling fix #12485

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

Closed
wants to merge 9 commits into from
Closed

dlist API enhancements supporting timer cleanup and scheduling fix #12485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
882fe74
kernel: thread: correct type and member name of dlist node
pabigot Jan 4, 2019
9d57c54
kernel: sched: fix empty list detection
pabigot Jan 4, 2019
32f8c18
sys: dlist: Add sys_dnode_is_linked
pabigot Dec 30, 2018
681da23
kernel: timeout: remove local fix for double-remove
pabigot Dec 30, 2018
2e57052
kernel: poll: fix double-remove of node
pabigot Dec 30, 2018
fb0d742
kernel: timeout: detect inactive timeouts using dnode linked state
pabigot Jan 2, 2019
9416324
sys: dlist: add API to split lists
pabigot Jan 2, 2019
2d971c7
kernel: timeout: avoid premature adjustment of timeout
pabigot Jan 7, 2019
2e0d00b
tests: kernel: timer: test delay for schedule in timer callback
pabigot Jan 9, 2019
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
1 change: 1 addition & 0 deletions kernel/include/timeout_q.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ extern "C" {
static inline void _init_timeout(struct _timeout *t, _timeout_func_t fn)
{
sys_dnode_init(&t->node);
t->dticks = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This... is not the scheme I thought we'd agreed to. I thought this was simply going to change the convention

We've had multiple bugs with this kind of separate list handling in the past, because the timer objects may be modified while in that list, and it's not possible to keep execution locked while the callbacks execute. How does this work if another context aborts a thread while it's queued in the "to be executed" list? What happens if the callback tries to reschedule itself?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is what I said I was going to do: a solution based on the branch I'd referenced in some issue attached to the problem we were discussing. Both abort and self-reschedule should work just fine.

It looks like #12499 causes the test I created to pass, so we'll probably go with that. I'll end up re-opening #12248 though because if you're not going to take the change to managing the timeout list everything else that got added lacks motivation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't dump the whole thing. The cleaned up list removal workaround has obvious value, as does the test case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't add the test case as it will fail until your solution is merged. The rest is back in #12248 where it started out.

}

void _add_timeout(struct _timeout *to, _timeout_func_t fn, s32_t ticks);
Expand Down
89 changes: 50 additions & 39 deletions kernel/timeout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@ static struct k_spinlock timeout_lock;

static bool can_wait_forever;

/* Cycles left to process in the currently-executing z_clock_announce() */
static int announce_remaining;

#if defined(CONFIG_TIMER_READS_ITS_FREQUENCY_AT_RUNTIME)
int z_clock_hw_cycles_per_sec = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
#endif
Expand All @@ -44,33 +41,25 @@ static struct _timeout *next(struct _timeout *t)
return n == NULL ? NULL : CONTAINER_OF(n, struct _timeout, node);
}

static void remove_timeout(struct _timeout *t)
{
if (next(t) != NULL) {
next(t)->dticks += t->dticks;
}

sys_dlist_remove(&t->node);
}

static s32_t elapsed(void)
{
return announce_remaining == 0 ? z_clock_elapsed() : 0;
}

void _add_timeout(struct _timeout *to, _timeout_func_t fn, s32_t ticks)
{
__ASSERT(!sys_dnode_is_linked(&to->node), "");
to->fn = fn;

/* @todo This really ought to be removed to allow scheduling
* with negative delays, since the floor operation fails to
* maintain correct periodicity for timers that are so late
* they missed more than an interval. But without it
* kernel/timer/timer_api:test_timer_periodicity fails. Is
* the test making incorrect assumptions about how to trick
* the system? */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the point where I need insight. Negative delays are important for maintaining periodicity when ticks are not announced in a timely manner.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pabigot: Question is what is reference point of _add_timeout()? As far as I know, the reference is "now()" in real time. So scheduling timeouts in past is not possible. On the other hand we should be able to add timeouts which should fire "now()".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The incoming argument ticks is signed, relative to now = k_uptime_get(). It's negative if we've already missed the deadline. We can't unilaterally discard the only information we have on the alignment of the recurring deadline, so negative delays must be recorded and preserved so subsequent reschedules maintain the correct offsets. In practice the timeout will be near the head of the queue (before anything that isn't late) so it will be invoked in the next tick interrupt. That's the earliest possible unless the z_clock_announce is reworked to loop as long as there's a late timeout at the head of the list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. Now i see it. We have to handle missed periodic timeouts which were missed more than the period.

ticks = max(1, ticks);

LOCKED(&timeout_lock) {
struct _timeout *t;

to->dticks = ticks + elapsed();
to->dticks = ticks + z_clock_elapsed();
for (t = first(); t != NULL; t = next(t)) {
__ASSERT(t->dticks >= 0, "");

if (t->dticks > to->dticks) {
t->dticks -= to->dticks;
sys_dlist_insert_before(&timeout_list,
Expand All @@ -96,10 +85,14 @@ int _abort_timeout(struct _timeout *to)

LOCKED(&timeout_lock) {
if (sys_dnode_is_linked(&to->node)) {
remove_timeout(to);
if (next(to) != NULL) {
next(to)->dticks += to->dticks;
}
sys_dlist_remove(&to->node);
ret = 0;
}
}
to->dticks = 0;

return ret;
}
Expand Down Expand Up @@ -132,7 +125,7 @@ s32_t _get_next_timeout_expiry(void)
LOCKED(&timeout_lock) {
struct _timeout *to = first();

ret = to == NULL ? maxw : max(0, to->dticks - elapsed());
ret = to == NULL ? maxw : max(0, to->dticks - z_clock_elapsed());
}

#ifdef CONFIG_TIMESLICING
Expand Down Expand Up @@ -168,33 +161,51 @@ void z_clock_announce(s32_t ticks)
z_time_slice(ticks);
#endif

sys_dlist_t ready;
sys_dnode_t *node;
s32_t remaining_ticks = ticks;
k_spinlock_key_t key = k_spin_lock(&timeout_lock);
struct _timeout *t = first();

announce_remaining = ticks;

while (first() != NULL && first()->dticks <= announce_remaining) {
struct _timeout *t = first();
int dt = t->dticks;
curr_tick += ticks;

curr_tick += dt;
announce_remaining -= dt;
t->dticks = 0;
remove_timeout(t);
if (!t) {
/* Fast exit, no timeouts */
goto out;
}

k_spin_unlock(&timeout_lock, key);
t->fn(t);
key = k_spin_lock(&timeout_lock);
/* Find the first timeout that isn't at/past its deadline */
while ((t != NULL) && (t->dticks <= remaining_ticks)) {
remaining_ticks -= t->dticks;
t = next(t);
}

if (first() != NULL) {
first()->dticks -= announce_remaining;
sys_dlist_init(&ready);
if (t == NULL) {
sys_dlist_join(&ready, &timeout_list);
} else {
sys_dlist_split(&ready, &timeout_list, &t->node);
t->dticks -= remaining_ticks;
}

curr_tick += announce_remaining;
announce_remaining = 0;
/* Invoke the callback of each expired timeout */
node = sys_dlist_peek_head(&ready);
if (node) {
k_spin_unlock(&timeout_lock, key);
do {
sys_dlist_remove(node);
t = CONTAINER_OF(node, struct _timeout, node);
t->dticks -= ticks;

z_clock_set_timeout(_get_next_timeout_expiry(), false);
t->fn(t);

node = sys_dlist_peek_head(&ready);
} while (node != NULL);
key = k_spin_lock(&timeout_lock);
}

out:
z_clock_set_timeout(_get_next_timeout_expiry(), false);
k_spin_unlock(&timeout_lock, key);
}

Expand Down
2 changes: 1 addition & 1 deletion kernel/timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ void _timer_expiration_handler(struct _timeout *t)
if (timer->period > 0) {
key = irq_lock();
_add_timeout(&timer->timeout, _timer_expiration_handler,
timer->period);
timer->timeout.dticks + timer->period);
irq_unlock(key);
}

Expand Down