-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Kernel: Make the shutdown procedure infallible #20786
Kernel: Make the shutdown procedure infallible #20786
Conversation
8a56c9f
to
ad29610
Compare
ad29610
to
241b6f4
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
This new template will be used for variables that are expected to be written only once during the whole kernel lifetime, but to be read multiple times without inducing heavy locking penalties.
This is done by making the power state switch code to rely on a process that is always alive, initially started during boot. Other functions are infallible as well - such as the procedure to kill all user processes, so now they're marked as such.
We do this by simplifying the unmounting code during the shutdown procedure to a great extent, so only the actual last call on unmount for each Mount could fail, but still will not stop the procedure from moving forward with the rest of the shutdown sequence.
241b6f4
to
e877cb5
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions! |
|
||
static constexpr StringView power_management_task_name = "Power Management Task"sv; | ||
Thread* g_power_management_task_thread; | ||
WritableOnce<PowerStateCommand> g_power_management_task_command { PowerStateCommand::None }; |
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.
Is this truly Writeable once? I could foresee us wanting to do suspend/resume as well in the future.
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.
Sure, but for now we don't support these things, so maybe a FIXME is sufficient for now?
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.
I have something in place to fix this in a better way :)
// NOTE: This is balanced by a `new` statement that is happening in various places before inserting the Mount object to the list. | ||
delete &mount; |
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.
"new that is happening in various places" :/ . This code is a bit smelly. Deleting a reference is not very idiomatic.
Can we centralize where the allocation and deletion happens so that we don't have funky lifetimes?
return stack; | ||
} | ||
|
||
ErrorOr<void> VirtualFileSystem::unmount_all(Badge<PowerManagementTask>) |
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... why is it ok to continue on with shutdown after the first filesystem fails to unmount? This code as written will not try to unmount every FS at least once. it always bails on the first error.
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.
Huh, I missed that. Definitely should try unmounting everything, since we’ve had bugs in the past with non-unmountable file systems.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
PR is blocked now on #22968. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
I am not sure this is necessary when we actually merge #22968, so until I figure out what to do next, let's close this :) |
cc @kleinesfilmroellchen