Fix unix changing from real-time to normal policy #39
Conversation
No effect when changing from real-time to normal policy.
WalkthroughThe recent updates refine thread priority management in Unix systems by setting a fixed priority for normal threads and adjusting relative priority via niceness. A new test ensures changes between realtime and normal policies require appropriate capabilities, enhancing system robustness and compliance. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedAST-based Instructions (13)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
src/unix.rs (1)
Line range hint
45-57: Minimize and document the use of unsafe code.There are multiple instances of unsafe code throughout the file, primarily used for low-level system interactions. While some of this is necessary, it's important to ensure that each use of unsafe is justified, minimal, and accompanied by comments explaining why it is needed and what specific precautions were taken to avoid common pitfalls. This practice helps maintain the safety and readability of the code.
Also applies to: 61-73, 114-114, 150-158, 364-364, 369-369, 530-530, 585-591, 601-607, 617-617, 677-693, 811-811, 870-870
src/unix.rs
Outdated
| // Normal priority threads must be set with static priority 0. | ||
| let params = ScheduleParams { sched_priority: 0 }.into_posix(); | ||
|
|
||
| let ret = unsafe { | ||
| libc::pthread_setschedparam( | ||
| native, | ||
| policy.to_posix(), | ||
| ¶ms as *const libc::sched_param, | ||
| ) | ||
| }; | ||
|
|
||
| match ret { | ||
| 0 => Ok(()), | ||
| e => Err(Error::OS(e)), | ||
| }?; | ||
|
|
||
| // Normal priority threads adjust relative priority through niceness. |
There was a problem hiding this comment.
Consider simplifying the error handling in the normal thread priority setting.
The current implementation uses a match statement to handle the return value of pthread_setschedparam, followed by a separate error check for setpriority. This could be streamlined by consolidating the error handling into a single block, improving readability and maintainability. Here's a proposed refactor:
let ret = unsafe {
libc::pthread_setschedparam(
native,
policy.to_posix(),
¶ms as *const libc::sched_param,
)
};
if ret != 0 {
return Err(Error::OS(ret));
}
set_errno(0);
let ret = unsafe { libc::setpriority(libc::PRIO_PROCESS, 0, fixed_priority) };
if ret != 0 {
return Err(Error::OS(errno()));
}
Ok(())
There was a problem hiding this comment.
|
Hi, @kmdade! |
kmdade
force-pushed
the
fix-unix-changing-between-policies
branch
from
f36ad97
to
72bdeb7
Compare
| // Normal priority threads must be set with static priority 0. | ||
| let params = ScheduleParams { sched_priority: 0 }.into_posix(); | ||
|
|
||
| let ret = unsafe { libc::setpriority(libc::PRIO_PROCESS, 0, fixed_priority) }; | ||
| if ret == 0 { | ||
| return Ok(()); | ||
| let ret = unsafe { | ||
| libc::pthread_setschedparam( | ||
| native, | ||
| policy.to_posix(), | ||
| ¶ms as *const libc::sched_param, | ||
| ) | ||
| }; | ||
|
|
||
| if ret != 0 { | ||
| return Err(Error::OS(ret)); | ||
| } | ||
|
|
||
| match errno() { | ||
| 0 => Ok(()), | ||
| e => Err(Error::OS(e)), | ||
| // Normal priority threads adjust relative priority through niceness. | ||
| set_errno(0); | ||
| let ret = unsafe { libc::setpriority(libc::PRIO_PROCESS, 0, fixed_priority) }; | ||
| if ret != 0 { | ||
| return Err(Error::OS(errno())); | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Consider consolidating error handling for clarity and maintainability.
The current implementation handles errors from pthread_setschedparam and setpriority in separate blocks. This could be streamlined by consolidating the error handling into a single block, improving readability and maintainability. Here's a proposed refactor:
let ret = unsafe {
libc::pthread_setschedparam(
native,
policy.to_posix(),
¶ms as *const libc::sched_param,
)
};
if ret != 0 {
return Err(Error::OS(ret));
}
set_errno(0);
let ret = unsafe { libc::setpriority(libc::PRIO_PROCESS, 0, fixed_priority) };
if ret != 0 {
return Err(Error::OS(errno()));
}
Ok(())| #[test] | ||
| fn change_between_realtime_and_normal_policies_requires_capabilities() { | ||
| use crate::ThreadPriorityOsValue; | ||
|
|
||
| const TEST_PRIORITY: u8 = 15; | ||
|
|
||
| let realtime_policy = ThreadSchedulePolicy::Realtime(RealtimeThreadSchedulePolicy::Fifo); | ||
| let normal_policy = ThreadSchedulePolicy::Normal(NormalThreadSchedulePolicy::Other); | ||
|
|
||
| // While we may desire an OS-specific priority, the reported value is always crossplatform. | ||
| let desired_priority = ThreadPriority::Os(ThreadPriorityOsValue(TEST_PRIORITY as _)); | ||
| let expected_priority = ThreadPriority::Crossplatform(ThreadPriorityValue(TEST_PRIORITY)); | ||
|
|
||
| let thread = std::thread::current(); | ||
| thread | ||
| .set_priority_and_policy(realtime_policy, desired_priority) | ||
| .expect("to set realtime fifo policy"); | ||
|
|
||
| assert_eq!(thread.get_schedule_policy(), Ok(realtime_policy)); | ||
| assert_eq!(thread.get_priority(), Ok(expected_priority)); | ||
|
|
||
| thread | ||
| .set_priority_and_policy(normal_policy, desired_priority) | ||
| .expect("to set normal other policy"); | ||
|
|
||
| assert_eq!(thread.get_schedule_policy(), Ok(normal_policy)); | ||
|
|
||
| // On linux, normal priority threads always have static priority 0. Instead the "nice" value is used. | ||
| #[cfg(not(target_os = "linux"))] | ||
| assert_eq!(thread.get_priority(), Ok(expected_priority)); | ||
| #[cfg(target_os = "linux")] | ||
| { | ||
| let nice = unsafe { libc::getpriority(0, 0) }; | ||
| assert_eq!(nice, TEST_PRIORITY as i32); | ||
| } | ||
| } |
There was a problem hiding this comment.
Expand the test coverage in change_between_realtime_and_normal_policies_requires_capabilities.
The current test validates the basic functionality of changing policies and priorities. However, it could be beneficial to include more edge cases, such as invalid priorities or unsupported policies, to ensure robustness. Would you like me to help by adding these test cases?
|
The suggested change is applied. |
Thank you for authoring this very helpful library.
I noticed that nothing was happening when attempting to modify the policy of a real-time thread, and I believe I have identified the issue.
Changes
set_thead_priority_and_policy()is updated to also call pthread_setschedparam when setting a "normal" policy so that the thread schedule policy is updated in addition to the nice value.Additional Notes
I develop on MacOS and Ubuntu, and have tested on both platforms. I admittedly did not familiarize myself with the overall cross-platform test plan, and will gladly follow any instructions. Suggested improvements to implementation or style are welcome, thank you for considering these changes.
Summary by CodeRabbit
New Features
Tests