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.

Sign up for GitHub

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

Jump to bottom

Use Once instead of lazy_static #790

Closed
wants to merge 1 commit into from
Closed

Use Once instead of lazy_static #790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 0 additions & 5 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,11 +304,6 @@ libc = { version = "0.2.48", default_features = false }
[target.'cfg(not(target_os = "ios"))'.dependencies]
spin = { version = "0.5.0" }

[target.'cfg(any(target_os = "android", target_os = "linux"))'.dependencies]

[target.'cfg(any(target_os = "redox", all(unix, not(any(target_os = "macos", target_os = "ios")))))'.dependencies]
lazy_static = "1.2"

[target.'cfg(target_os = "windows")'.dependencies]
winapi = { version = "0.3.6", default_features = false, features = ["ntsecapi", "wtypesbase"] }

Expand Down
40 changes: 13 additions & 27 deletions src/rand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,23 +208,19 @@ mod sysrand {
))]
mod urandom {
use crate::error;
use spin::Once;
use std;

pub fn fill(dest: &mut [u8]) -> Result<(), error::Unspecified> {
use lazy_static::lazy_static;
static OPEN: Once<Result<std::fs::File, std::io::Error>> = Once::new();
Copy link
Owner

Choose a reason for hiding this comment

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

Here, wouldn't it make more sense to use std::sync::Once instead of spin::Once? We really don't want threads spinning while we wait for the file open operation to complete (and we can't try to open the file more than once concurrently, because we want to minimize the number of file handles we try to acquire).


#[cfg(target_os = "redox")]
static RANDOM_PATH: &str = "rand:";
#[cfg(unix)]
static RANDOM_PATH: &str = "/dev/urandom";

lazy_static! {
static ref FILE: Result<std::fs::File, std::io::Error> =
std::fs::File::open(RANDOM_PATH);
}

match *FILE {
Ok(ref file) => {
match OPEN.call_once(|| std::fs::File::open(RANDOM_PATH)) {
Ok(file) => {
use std::io::Read;
(&*file).read_exact(dest).map_err(|_| error::Unspecified)
},
Expand All @@ -237,29 +233,19 @@ mod urandom {
#[cfg(all(target_os = "linux", feature = "dev_urandom_fallback"))]
mod sysrand_or_urandom {
use crate::error;
use spin::Once;

enum Mechanism {
Sysrand,
DevURandom,
fn sysrand_supported() -> bool {
let mut dummy = [0u8; 1];
super::sysrand_chunk::chunk(&mut dummy[..]).is_ok()
}

pub fn fill(dest: &mut [u8]) -> Result<(), error::Unspecified> {
use lazy_static::lazy_static;

lazy_static! {
static ref MECHANISM: Mechanism = {
let mut dummy = [0u8; 1];
if super::sysrand_chunk::chunk(&mut dummy[..]).is_err() {
Mechanism::DevURandom
} else {
Mechanism::Sysrand
}
};
}

match *MECHANISM {
Mechanism::Sysrand => super::sysrand::fill(dest),
Mechanism::DevURandom => super::urandom::fill(dest),
static SYSRAND_SUPPORTED: Once<bool> = Once::new();
if *SYSRAND_SUPPORTED.call_once(sysrand_supported) {
super::sysrand::fill(dest)
Copy link
Owner

Choose a reason for hiding this comment

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

On Linux, at least, getrandom() can block a long time waiting for the kernel entropy pool to be initialized. It seems wrong to have all threads spinning while we wait for the syscall to complete. In theory all these spinning userspace threads could take away CPU time from the kernel and/or other processes that would be useful for initializing the entropy pool and allowing the kernel to return to userspace.

How about, instead, we keep track of whether getrandom() is supported using a tri-state atomic usize:

if 0, call getrandom() to see if it is available, atomically set the flag to 1 or 2 depending on the result, and continue as follows.
If 1, use sysrand::fill() to service the request.
if 2, use urandom::fill() to service the request.

The effect of this would be to push the mutex down to the kernel, out of this userspace process. Presumably the kernel isn't going to use spin locks in such a way that would cause it to start itself of CPU time needed to initialize the entropy pool.

WDYT?

} else {
super::urandom::fill(dest)
}
}
}
Expand Down