From 2633ff8661c8b710f6f9369c1075910a2c77332c Mon Sep 17 00:00:00 2001
From: Martin Stein <martin.stein@genode-labs.com>
Date: Tue, 12 Sep 2017 13:15:01 +0200
Subject: [PATCH] alarm: fix information loss due to int-cast
When we have two time values of an unsigned integer type and we create
the difference and want to know wether it is positive or negative within
the same value we loose at least one half of the value range for casting
to signed integers. This was the case in the alarm scheduler when
checking wether an alarm already triggered. Even worse, we casted from
'unsigned long' to 'signed int' which caused further loss on at least
x86_64. Thus, big timeouts like ~0UL falsely triggered directly.
Now, we use an extra boolean value to remember in which period of the
time counter we are and to which period of the time counter the deadline
of an alarm belongs. This boolean switches its value each time the time
counter wraps. This way, we can avoid any casting by checking wether the
current time is of the same period as the deadline of the alarm that we
inspect. If so, the alarm is pending if "current time >= alarm
deadline", otherwise it is pending if "current time < alarm deadline".
Ref #2490
---
repos/os/include/os/alarm.h | 18 +++++++++--
repos/os/src/lib/alarm/alarm.cc | 57 ++++++++++++++++++++++++++-------
2 files changed, 60 insertions(+), 15 deletions(-)
diff --git a/repos/os/include/os/alarm.h b/repos/os/include/os/alarm.h
index 43c2bcd836..d009c93bf9 100644
--- a/repos/os/include/os/alarm.h
+++ b/repos/os/include/os/alarm.h
@@ -34,16 +34,27 @@ class Genode::Alarm
Lock _dispatch_lock; /* taken during handle method */
Time _deadline; /* next deadline */
+ bool _deadline_period;
Time _period; /* duration between alarms */
int _active; /* set to one when active */
Alarm *_next; /* next alarm in alarm list */
Alarm_scheduler *_scheduler; /* currently assigned scheduler */
- void _assign(Time period, Time deadline, Alarm_scheduler *scheduler) {
- _period = period, _deadline = deadline, _scheduler = scheduler; }
+ void _assign(Time period,
+ Time deadline,
+ bool deadline_period,
+ Alarm_scheduler *scheduler)
+ {
+ _period = period;
+ _deadline_period = deadline_period;
+ _deadline = deadline;
+ _scheduler = scheduler;
+ }
void _reset() {
- _assign(0, 0, 0), _active = 0, _next = 0; }
+ _assign(0, 0, false, 0), _active = 0, _next = 0; }
+
+ bool _is_pending_at(unsigned long time, bool time_period) const;
protected:
@@ -71,6 +82,7 @@ class Genode::Alarm_scheduler
Lock _lock; /* protect alarm list */
Alarm *_head; /* head of alarm list */
Alarm::Time _now; /* recent time (updated by handle method) */
+ bool _now_period { false };
/**
* Enqueue alarm into alarm queue
diff --git a/repos/os/src/lib/alarm/alarm.cc b/repos/os/src/lib/alarm/alarm.cc
index e48a87d073..c50cc440c6 100644
--- a/repos/os/src/lib/alarm/alarm.cc
+++ b/repos/os/src/lib/alarm/alarm.cc
@@ -34,7 +34,7 @@ void Alarm_scheduler::_unsynchronized_enqueue(Alarm *alarm)
}
/* if deadline is smaller than any other deadline, put it on the head */
- if ((int)alarm->_deadline - (int)_now < (int)_head->_deadline - (int)_now) {
+ if (alarm->_is_pending_at(_head->_deadline, _head->_deadline_period)) {
alarm->_next = _head;
_head = alarm;
return;
@@ -43,8 +43,10 @@ void Alarm_scheduler::_unsynchronized_enqueue(Alarm *alarm)
/* find list element with a higher deadline */
Alarm *curr = _head;
while (curr->_next &&
- (int)curr->_next->_deadline - (int)_now < (int)alarm->_deadline - (int)_now)
+ curr->_next->_is_pending_at(alarm->_deadline, alarm->_deadline_period))
+ {
curr = curr->_next;
+ }
/* if end of list is reached, append new element */
if (curr->_next == 0) {
@@ -81,12 +83,21 @@ void Alarm_scheduler::_unsynchronized_dequeue(Alarm *alarm)
}
+bool Alarm::_is_pending_at(unsigned long time, bool time_period) const
+{
+ return (time_period == _deadline_period &&
+ time >= _deadline) ||
+ (time_period != _deadline_period &&
+ time < _deadline);
+}
+
+
Alarm *Alarm_scheduler::_get_pending_alarm()
{
Lock::Guard lock_guard(_lock);
- if (!_head || ((int)_head->_deadline - (int)_now >= 0))
- return 0;
+ if (!_head || !_head->_is_pending_at(_now, _now_period)) {
+ return nullptr; }
/* remove alarm from head of the list */
Alarm *pending_alarm = _head;
@@ -99,7 +110,7 @@ Alarm *Alarm_scheduler::_get_pending_alarm()
pending_alarm->_dispatch_lock.lock();
/* reset alarm object */
- pending_alarm->_next = 0;
+ pending_alarm->_next = nullptr;
pending_alarm->_active--;
return pending_alarm;
@@ -108,9 +119,16 @@ Alarm *Alarm_scheduler::_get_pending_alarm()
void Alarm_scheduler::handle(Alarm::Time curr_time)
{
- Alarm *curr;
+ /*
+ * Raise the time counter and if it wraps, update also in which
+ * period of the time counter we are.
+ */
+ if (_now > curr_time) {
+ _now_period = !_now_period;
+ }
_now = curr_time;
+ Alarm *curr;
while ((curr = _get_pending_alarm())) {
unsigned long triggered = 1;
@@ -130,11 +148,26 @@ void Alarm_scheduler::handle(Alarm::Time curr_time)
if (reschedule) {
- /* schedule next event */
- if (curr->_deadline == 0)
- curr->_deadline = _now;
-
- curr->_deadline += triggered * curr->_period;
+ /*
+ * At this point, the alarm deadline normally is somewhere near
+ * the current time but If the alarm had no deadline by now,
+ * initialize it with the current time.
+ */
+ if (curr->_deadline == 0) {
+ curr->_deadline = _now;
+ curr->_deadline_period = _now_period;
+ }
+ /*
+ * Raise the deadline value by one period of the alarm and
+ * if the deadline value wraps thereby, update also in which
+ * period it is located.
+ */
+ Alarm::Time const deadline = curr->_deadline +
+ triggered * curr->_period;
+ if (curr->_deadline > deadline) {
+ curr->_deadline_period = !curr->_deadline_period;
+ }
+ curr->_deadline = deadline;
/* synchronize enqueue operation */
Lock::Guard lock_guard(_lock);
@@ -157,7 +190,7 @@ void Alarm_scheduler::_setup_alarm(Alarm &alarm, Alarm::Time period, Alarm::Time
if (alarm._active)
_unsynchronized_dequeue(&alarm);
- alarm._assign(period, deadline, this);
+ alarm._assign(period, deadline, _now > deadline, this);
_unsynchronized_enqueue(&alarm);
}