Skip to content
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

Fix unix changing from real-time to normal policy #39

Merged
merged 2 commits into from
Apr 29, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 57 additions & 9 deletions src/unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,19 +596,29 @@ pub fn set_thread_priority_and_policy(
e => Err(Error::OS(e)),
}
} else {
// If this is a normal-scheduled thread, the priority is
// set via niceness.
set_errno(0);
// 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(),
&params 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(())
Comment on lines +599 to +621
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(),
        &params 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(())

}
}
}
Expand Down Expand Up @@ -820,6 +830,44 @@ mod tests {
assert!(thread_schedule_policy_param(thread_id).is_ok());
}

// Running this test requires CAP_SYS_NICE.
#[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);
}
}
iddm marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +834 to +869
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


#[test]
#[cfg(target_os = "linux")]
fn set_deadline_policy() {
Expand Down
Loading