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

Implement sched::sched_getaffinity() #1148

Merged
merged 1 commit into from Nov 24, 2019

Conversation

thib-ack
Copy link
Contributor

Hello,

I found the function sched::sched_setaffinity() but I also needed to get the process affinity and I did not find the function sched::sched_getaffinity()
So I added the function which maps sched_getaffinity(2) which "get a process's CPU affinity mask" to the sched.rs file.
I hope the code match your guidelines (returned Result<CpuSet> instead of pointer parameter)
If that's not the case, tell me, I will fix asap.

I hope this will help !

Thanks,
Thibaut

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Could you also please add a CHANGELOG entry and a test?

src/sched.rs Outdated
libc::sched_getaffinity(
pid.into(),
mem::size_of::<CpuSet>() as libc::size_t,
& mut cpuset.cpu_set,
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: there should be no space between & and mut.

src/sched.rs Outdated
)
};

match res {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can do this more simply by Errno::result(res).and(Ok(cpuset))

@thib-ack
Copy link
Contributor Author

thib-ack commented Nov 1, 2019

  • Issues fixed
  • test added (in new dedicated file test_sched.rs)
  • changelog added

Thanks

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Could you please also add docs for sched_getaffinity? And since you're here, it would be great if you could do the same for sched_setaffinity and for the CpuSet methods.

let initial_affinity = sched_getaffinity(Pid::from_raw(0)).unwrap();
let mut at_least_one_cpu = false;
let mut last_valid_cpu = 0;
for field in 0..(8 * mem::size_of::<libc::cpu_set_t>()) {
Copy link
Member

Choose a reason for hiding this comment

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

What does the magic number 8 mean? You should remove it if possible, or define it as a const if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value comes from the CpuSet::set() and CpuSet::is_set() methods.
https://github.com/nix-rust/nix/blob/master/src/sched.rs#L63

        pub fn is_set(&self, field: usize) -> Result<bool> {
            if field >= 8 * mem::size_of::<libc::cpu_set_t>() {
                Err(Error::Sys(Errno::EINVAL))
            } else {
                Ok(unsafe { libc::CPU_ISSET(field, &self.cpu_set) })
            }
        }

That's because there is no easy way to know the size of a CpuSet and in the test I want to check each bits of the CpuSet.

I can do differently:

  • add a method CpuSet::size() which will return this value.
  • call CpuSet::is_set() until I get a Errno::EINVAL
    What do you prefer ?

Copy link
Member

Choose a reason for hiding this comment

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

Better to add a method to CpuSet. But I would call it count, because that's what the C macros call it.

@@ -0,0 +1,36 @@

#[cfg(any(target_os = "android",
Copy link
Member

Choose a reason for hiding this comment

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

You should move the #[cfg()] into test/test.rs, for consistency with other files.

@thib-ack thib-ack force-pushed the add_sched_getaffinity branch 2 times, most recently from 5895dc1 to 47e9413 Compare November 2, 2019 09:42
@thib-ack
Copy link
Contributor Author

thib-ack commented Nov 2, 2019

Hi,

I'll be AFK for the next 2 weeks unfortunately. I'll continue to work on this when I come back.
For example, I'm thinking the sched::sched_getaffinity() should take a &CpuSet as parameter instead of building one internally. That way, users could allocate the CpuSet once and use it many times in sched_getaffinity().
You agree ?

Thanks,

@asomers
Copy link
Member

asomers commented Nov 2, 2019

Hi,

I'll be AFK for the next 2 weeks unfortunately. I'll continue to work on this when I come back.
For example, I'm thinking the sched::sched_getaffinity() should take a &CpuSet as parameter instead of building one internally. That way, users could allocate the CpuSet once and use it many times in sched_getaffinity().
You agree ?

Thanks,

No, I disgree. Return arguments are very C-ish. It's much Rustier to use return values. I doubt there will be any performance difference, because rustc is able to do what C++ people call "return by move".

@asomers
Copy link
Member

asomers commented Nov 2, 2019

You'll have to rebase to fix the build error on nightly.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

I like the docs! I think we're getting close now.

src/sched.rs Outdated
Err(Error::Sys(Errno::EINVAL))
} else {
Ok(unsafe { libc::CPU_CLR(field, &mut self.cpu_set) })
}
}

/// Return the maximum number of CPU in CpuSet
#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the #[inline]. The compiler is smart enough to figure it out for itself.

src/sched.rs Outdated
pub fn unset(&mut self, field: usize) -> Result<()> {
if field >= 8 * mem::size_of::<libc::cpu_set_t>() {
CpuSet::count();
Copy link
Member

Choose a reason for hiding this comment

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

Why this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, copy-paste mistake

src/sched.rs Outdated
///
/// Binding the current thread to CPU 0 can be done as follows:
///
/// ```rust
Copy link
Member

Choose a reason for hiding this comment

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

I would probably make this rust,no_run. Otherwise, you'll be changing the affinity of the doc test process.

sched_getaffinity(2) get a process's CPU affinity mask
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Nov 24, 2019
1148: Implement sched::sched_getaffinity() r=asomers a=thib-ack

Hello,

I found the function `sched::sched_setaffinity()` but I also needed to get the process affinity and I did not find the function `sched::sched_getaffinity()`
So I added the function which maps [sched_getaffinity(2)](https://linux.die.net/man/2/sched_getaffinity) which "get a process's CPU affinity mask" to the sched.rs file.
I hope the code match your guidelines (returned `Result<CpuSet>` instead of pointer parameter)
If that's not the case, tell me, I will fix asap.

I hope this will help !

Thanks,
Thibaut

Co-authored-by: Thibaut Ackermann <thibaut@ackermann.dev>
@bors
Copy link
Contributor

bors bot commented Nov 24, 2019

Build succeeded

@bors bors bot merged commit 18c2038 into nix-rust:master Nov 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants