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

Improve RDRAND implementation #24

Merged
merged 2 commits into from
Jun 12, 2019
Merged

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Jun 10, 2019

This change makes a few improvements to be closer to Intel's recommendations

  1. The value of RETRY_LIMIT is the one recommended by Intel.
  2. Use chunks_exact_mut to elide most calls to memcpy. See generated assembly.
  3. Remove all unsafe code except that used to invoke the RDRAND intrinsic.
  4. Adds Travis build check for x86_64-fortanix-unknown-sgx

Stylistically, the code is now much easier to read. The file has also been renamed to rdrand.rs to better reflect what it does. This diff makes it easier to see what has actually changed.

@dhardy dhardy requested a review from newpavlov June 10, 2019 06:38
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Overall looks good to me except two small nitpicks.

src/rdrand.rs Outdated Show resolved Hide resolved
src/rdrand.rs Outdated Show resolved Hide resolved
@josephlr
Copy link
Member Author

@newpavlov this should be ready to merge, provided you're OK with an implementation that needs up to two calls to memcpy at the beginning and end of dest.

I have an alternative implementation that only calls memcpy on small slices. On large slices, it calls write_unaligned at the beginning and end of dest (writing to some of the memory twice). While the alternative implementation also gives a smaller binary, it has to use much more unsafe code to stop the compiler from emitting slice_index_len_fail or panic hooks.

@newpavlov
Copy link
Member

newpavlov commented Jun 11, 2019

Personally I prefer the alternative implementation. We even could drop 0 => {} branch to make binary even smaller, as I guess calls ofgetrandom with zero-length slices will be really rare.

If we'll decide to keep the current implementation, then you can make get_rand_unaligned slightly more idiomatic, while keeping the efficiency:

fn get_rand_unaligned(dest: &mut [u8]) -> Result<(), Error> {
    for chunk in dest.chunks_mut(mem::size_of::<u64>()) {
        let data = get_rand_u64()?;
        let n = chunk.len();
        chunk.copy_from_slice(&data.to_ne_bytes()[..n]);
    }
    Ok(())
}

@dhardy
What do you think?

@josephlr
Copy link
Member Author

If we'll decide to keep the current implementation, then you can make get_rand_unaligned slightly more idiomatic, while keeping the efficiency:

Oh that's much nicer, I just updated this PR to use essentially that code. The generated assembly still gives what we expect.

Personally I prefer the alternative implementation. We even could drop 0 => {} branch to make binary even smaller, as I guess calls of getrandom with zero-length slices will be really rare.

I'm still trying to figure out if there is a way to get the alternative implementation working without as much unsafe code. I'll let you know my progress.

@josephlr
Copy link
Member Author

So after looking at this some more, it’s probably not worth it to hyper-optimize this to maximize the number of aligned accesses. On x86 the same code gets emitted for aligned/unaligned accesses anyway (unaligned ones just happen to be slower).

I’ll rewrite this to just use chunks_mut_exact, should make everything fairly short.

Note: as most of the time this is being used to fill a freshly allocated 128/256 but key, everything will usually be properly aligned regardless.

@josephlr
Copy link
Member Author

@dhardy @newpavlov this should be ready for final review/merging, see the updated PR description.

src/rdrand.rs Outdated Show resolved Hide resolved
@newpavlov newpavlov merged commit 9e64082 into rust-random:master Jun 12, 2019
@josephlr josephlr deleted the rdrand-ext branch June 12, 2019 10:50
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.

2 participants