-
-
Notifications
You must be signed in to change notification settings - Fork 609
sched: Support POSIX's SCHED_RR scheduling policy #1338
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.
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
base: master
Are you sure you want to change the base?
Conversation
Previously, pull request cloudius-systems#1223 added support for the SCHED_FIFO but didn't implement SCHED_RR. This PR attempts to follow-up on that by proposing an implementation of SCHED_RR. Time slicing support, as mandated by SCHED_RR, is implemented though the `set_realtime_time_slice(duration)` API added in the aforementioned pull request. Within the scheduler, the amount of time the thread ran so far is tracked if it exceeds the set duration, the thread is preempted (if there is a runnable thread of same or higher priority). The thread's run time is reset on preemption. Signed-off-by: Sören Tempel <tempel@ibr.cs.tu-bs.de>
| // p is no longer running, if it has a realtime slice reset it. | ||
| if (p->_realtime.has_slice()) { | ||
| p->_realtime.reset_slice(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not entirely clear to me if the slice should be reset if the thread is no longer runnable (e.g. because of blocking I/O). POSIX does not explicitly describe when the slice should be reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm also not sure, but think this if is right. I think the idea of the time slice is to make sure that a single thread never in its priority group never runs more than 1ms (for example) without letting other threads in its group run. But if the thread blocks or yields voluntarily (I believe this if covers both cases, right?), then it gives some other thread a chance to run and it too has a chance to run for a whole time-slice, so it's only fair that this thread's time slice is reset to zero. I think.
I tried searching if anybody mentions this question, and couldn't find such a discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I am missing something, Linux only seems to reset the time slice once it expires not when the thread is no longer runnable. AFAIK This is the only place in the rt scheduler where it is reset: https://github.com/torvalds/linux/blob/v6.16/kernel/sched/rt.c#L2580-L2583
wkozaczuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code change looks good. I did ask some questions though.
| // If the threads have the same realtime priority, then only reschedule | ||
| // if the currently executed thread has exceeded its time slice (if any). | ||
| if (t._realtime._priority == p->_realtime._priority && | ||
| ((!p->_realtime.has_slice() || p->_realtime.has_remaining()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So all this means we are going to keep the current thread p running should stay running if _time_slice is 0 (run 'forever' until yields or waits) OR there is still time left to run per its _time_slice, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[…] should stay running if
_time_sliceis 0 (run 'forever' until yields or waits) OR there is still time left to run per its_time_slice, right?
Yes p->_realtime.has_slice() checks if it has a time slice (i.e. _time_slice != 0), p->_realtime.has_remaining() checks if there is still time remaining on the slice (if it has one).
Note that, even if the thread has exceeded its time slice it may still be selected to run again if there is no thread with a higher priority. Hence, the priority comparison in the if condition.
| enqueue(*p); | ||
| p->_realtime.reset_slice(); | ||
| } else { | ||
| // POSIX requires that if a real-time thread doesn't yield but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means that if the current thread p _time_slice is 0 OR p still has some remaining time to run, we will call enqueue_first_equal(). Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's correct. If we got here it means p was preempted. If it still has remaining time, it means it was preempted by a higher-priority realtime thread but when that higher-priority thread doesn't want to run, this thread p should continue running and continue its current time slice. The documentation says: "A SCHED_RR thread that has been preempted by a higher priority thread and subsequently resumes execution as a running thread will complete the unexpired portion of its round-robin time quantum.". It should be the first one in its priority group to run (and therefore enqueue_first_equal()) just like when no time slices existed.
|
@nyh What do you think? |
nyh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very nice to me, and appears correct although I'm a bit worried that I'm rusty in this code and might have missed something. I only left a few minor comments/requests.
I just realized that we never actually implemented the POSIX API for these features ;-) I we think we have such patches in #386 and maybe it will be nice to revive them.
| enqueue(*p); | ||
| p->_realtime.reset_slice(); | ||
| } else { | ||
| // POSIX requires that if a real-time thread doesn't yield but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's correct. If we got here it means p was preempted. If it still has remaining time, it means it was preempted by a higher-priority realtime thread but when that higher-priority thread doesn't want to run, this thread p should continue running and continue its current time slice. The documentation says: "A SCHED_RR thread that has been preempted by a higher priority thread and subsequently resumes execution as a running thread will complete the unexpired portion of its round-robin time quantum.". It should be the first one in its priority group to run (and therefore enqueue_first_equal()) just like when no time slices existed.
| // p is no longer running, if it has a realtime slice reset it. | ||
| if (p->_realtime.has_slice()) { | ||
| p->_realtime.reset_slice(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm also not sure, but think this if is right. I think the idea of the time slice is to make sure that a single thread never in its priority group never runs more than 1ms (for example) without letting other threads in its group run. But if the thread blocks or yields voluntarily (I believe this if covers both cases, right?), then it gives some other thread a chance to run and it too has a chance to run for a whole time-slice, so it's only fair that this thread's time slice is reset to zero. I think.
I tried searching if anybody mentions this question, and couldn't find such a discussion.
|
Thanks a lot to both of you for the detailed comments and feedback! I made some minor changes and left further comments above. I think the main two things that remain to be sorted out are:
|
|
Rebased, removed the flaky test, made the other test a little more useful and added the |
Previously, pull request #1223 added support for the
SCHED_FIFObut didn't implement theSCHED_RRpolicy. This PR attempts to follow-up on that by proposing an implementation ofSCHED_RR.Time slicing support, as mandated by
SCHED_RR, is implemented though theset_realtime_time_slice(duration)API added in the aforementioned pull request. Within the scheduler, the amount of time the thread ran so far is tracked if it exceeds the set duration, the thread is preempted (if there is a runnable thread of same or higher priority). The thread's run time is reset on preemption.This is my first OSv pull request, I added some basic tests which I can expand further. Additionally, I checked that the tests added #1223 still pass. Further testing is definitely needed, hence this is a draft PR for now.
Let me know what you think.