First implementation of kernel level timers #591
Conversation
… able to do sorted inserts.
…ixed indentation.
Kernel/Scheduler.cpp
Outdated
| struct Timer { | ||
| u64 expires; | ||
| void (*callback)(); | ||
| inline bool operator<(const Timer& rhs) |
There was a problem hiding this comment.
inline is meaningless in this context. Also please mark it const:
| inline bool operator<(const Timer& rhs) | |
| bool operator<(const Timer& rhs) const |
Kernel/Scheduler.cpp
Outdated
| { | ||
| return expires < rhs.expires; | ||
| } | ||
| inline bool operator>( const Timer& rhs) |
There was a problem hiding this comment.
Same comment as operator<
Kernel/Scheduler.h
Outdated
| @@ -32,6 +33,7 @@ class Scheduler { | |||
| static void beep(); | |||
| static void idle_loop(); | |||
| static void stop_idling(); | |||
| static void add_timer(Timer& timer); | |||
There was a problem hiding this comment.
No need to name the argument, since the name is embedded in the type:
| static void add_timer(Timer& timer); | |
| static void add_timer(Timer&); |
AK/SinglyLinkedList.h
Outdated
| @@ -128,6 +128,60 @@ class SinglyLinkedList { | |||
| m_tail->next = node; | |||
| m_tail = node; | |||
| } | |||
|
|
|||
| void sorted_insert_slow(const T& value) | |||
There was a problem hiding this comment.
This version can be implemented by simply calling the T&& one:
void sorted_insert_slow(const T& value)
{
sorted_insert_slow(T(value));
}
AK/SinglyLinkedList.h
Outdated
|
|
||
| } | ||
|
|
||
| void sorted_insert_slow(T&& value) |
There was a problem hiding this comment.
It really seems like this should be a separate data structure. It could be a wrapper around a SinglyLinkedList at first, if you like.
That way the new class can make sure to only provide API's that keep the list sorted, while the old class can stay unsorted. We should not force clients of the class to know whether an instance maintains sort order or not.
For example, it could be a new SortedList template class in AK, or a kernel-specific SortedTimerList for example. (The names are just ideas.)
There was a problem hiding this comment.
That sounds reasonable. If we'd go the kernel-specific route, wouldn't it be better to have a static TimerQueue class? This could do the sorting of a SinglyLinkedList and also provide methods for adding and deleting timers.
There was a problem hiding this comment.
Sure, that sounds nice!
Kernel/Scheduler.cpp
Outdated
| timer.callback = []() { | ||
| dbgprintf("10 secs\n"); | ||
| }; | ||
| add_timer(timer); |
There was a problem hiding this comment.
I would suggest adding some convenience helpers that would allow us to write code like:
add_timer(10, [] {
dbg() << "10 secs";
}):Note that we'll need to use the TICKS_PER_SECOND mutliplier rather than hard-coding 1000, or we'll break the ability to change the kernel's tick frequency.
Kernel/Scheduler.cpp
Outdated
|
|
||
| struct TaskRedirectionData { | ||
| u16 selector; | ||
| TSS32 tss; | ||
| }; | ||
|
|
||
| struct Timer { |
There was a problem hiding this comment.
Timer objects should have some kind of identity so we can find them. It's okay for their address to be the identity, but then we should make sure their address is unique. The easiest way to do that is always allocating them on the heap.
We could also give them a timer ID, e.g an int or whatever seems best. :)
|
I should mention that the general idea is very nice, and will be a huge improvement over using Also, check out https://github.com/SerenityOS/serenity/blob/master/Contributing.md |
|
Please squash these into a single commit and keep the commit messages < 72 chars per line (see Contributing.md) |
| }; | ||
|
|
||
|
|
||
| class TimerQueue { |
There was a problem hiding this comment.
This is a pretty roundabout way to basically add a lot of global functions.
Instead of having a TimerQueue class with nothing but public static functions, let's use a singleton object:
// In header file:
class TimerQueue {
public:
static TimerQueue& the();
u64 add_timer(Timer&);
u64 add_timer(u64 duration, TimeUnit, void (callback)());
...
private:
SinglyLinkedList<Timer> m_timers;
};
In cpp file:
TimerQueue& TimerQueue::the()
{
if (!queue)
queue = new TimerQueue;
return s_the;
}Then you can use the single global instance of TimerQueue like so:
TimerQueue::the().add_timer(...);|
|
||
| void TimerQueue::sorted_insert_slow(T&& value) | ||
| { | ||
| auto* new_node = new Node(move(value)); |
There was a problem hiding this comment.
Reaching directly into the SinglyLinkedList class here is very strange.
It would be nicer to implement this by adding an API such as SinglyLinkedList::insert(Iterator&, T&&).
Then you could iterate over the list until you find your insertion point and then call insert() with the iterator you're using.
PR SerenityOS#591 defines the rationale for kernel-level timers. They're most immediately useful for TCP retransmission, but will most likely see use in many other areas as well.
PR #591 defines the rationale for kernel-level timers. They're most immediately useful for TCP retransmission, but will most likely see use in many other areas as well.
Hi,
as I said on IRC, I would like to start working on Serenity's networking Stack. For that, I need reliable timers for timeout and retransmission events. The current method of lazily comparing g_uptime with a calculated timestamp is insufficient. Thus, I cobbled together a first version of kernel level timers. Currently, it lacks support for removing or rescheduling timers, which I would add when this PR is approved. I just wanted to get some initial feedback.
Cheers.