Kernel: Remove select() as a syscall #11229
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boricj
force-pushed
the
removeSelectSyscall
branch
from
ee51281
to
112853d
Compare
boricj
force-pushed
the
removeSelectSyscall
branch
2 times, most recently
from
2dc1752
to
c781356
Compare
Now that the userland has a compatiblity wrapper for select(), the kernel doesn't need to implement this syscall natively. The poll() interface been around since 1987, any code still using select() should be slapped silly. Note: the SerenityOS source tree mostly uses select() and not poll() despite SerenityOS having support for poll() since early 2019...
boricj
force-pushed
the
removeSelectSyscall
branch
from
c781356
to
29a8e97
Compare
IdanHo
reviewed
Dec 12, 2021
Kernel/Syscalls/poll.cpp
Outdated
| if constexpr (IO_DEBUG || POLL_SELECT_DEBUG) | ||
| dbgln("polling on {} fds, timeout={}", fds_info.size(), params.timeout); | ||
|
|
||
| if (current_thread->block<Thread::SelectBlocker>(timeout, fds_info).was_interrupted()) |
There was a problem hiding this comment.
Should we rename this to PollBlocker?
There was a problem hiding this comment.
I'm not sure, SelectBlocker isn't actually strongly tied to either select() or poll(), it's really just a generic blocker for events on a set of file descriptors. It was already a dubious name back when SerenityOS only had select(). Furthermore, the Selecting reason is actually worse because until now the thread could be in select(), poll(), pause(), sigsuspend() and who knows what else...
Since these questions are hard and this PR is just about removing the select() syscall from the kernel, I'll refrain from dealing with those for the time being.
|
Great idea! ⭐ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Once upon a time, Unix had no provisions for operating on more than one file descriptor at a time per process. There was blocking I/O as far as the eye can see. Then the BSD sockets API came and trouble brewed, for forking a process per file descriptor to handle seemed rather quaint in the age of early networking.
The
select()POSIX function is a relic from a past where 20 file descriptors per process was the limit. Then people soon realized that using bitfields to express sets of file descriptors was a quite silly design andpoll()soon appeared in 1987. Nearly 35 years later, it's time to invoke statute of limitations and remove that one bit of Unix legacy cruft from the kernel.This PR removes
select()as a kernel syscall and implements it as a compatibility wrapper inside LibC usingpoll(). This makespoll()currently the one and only native way on SerenityOS to wait for file descriptors to become ready, pending the day someone implements a more modern, higher-performance mechanism. Maintaining two nearly identical yet independent codepaths in the kernel for the exact same functionality is needlessly redundant, along with all the support code around it.Note that this PR does not migrate Serenity's userland to
poll().