use_file: Remove use of spin-locks #125

josephlr · 2020-01-02T08:37:14Z

Remove the general purpose spin-lock from getrandom, and don't spin
when polling /dev/random. We also remove the use of spin locks when
opening the persistent fd for platforms that require it.

For both these cases, we can just use the pthread lock/unlock methods
in libc. We also do some minor cleanup to better make use of Result
types and DropGuards.

Essentially, this change does the "standard" Mutex based synchronization approach, it just has to do it without libstd. With this change, we continue to have the property that getrandom uses at most one file descriptor per process.

EDIT: Updated description to indicate that we are removing all uses of spin locks.

Thanks to @matklad for pointing this out. See his blog post about getrandom and matklad/once_cell#61.

Signed-off-by: Joe Richey joerichey@google.com

matklad

This is fine, as the open() syscall will never block when opening a device

This is better, but, in theory, and if you are really unlucky, can still lead to a priority inversion, if the thread is preempted. Non-blocking massively reduces chances of preemption, but does not make them zero. Moreover, we are doing a syscall and I have vague recollections that the kernel uses syscalls as a chance to check if the quant is exhausted, so preemtion probability might actually be higher than in the uniformly distributed case. So, this is likely ok, but not almost surely ok :)

src/use_file.rs

josephlr · 2020-01-02T11:18:04Z

This is fine, as the open() syscall will never block when opening a device

This is better, but, in theory, and if you are really unlucky, can still lead to a priority inversion, if the thread is preempted.

I agree, after looking over the code, I think removing spinning entirely is the best bet (and also makes the code more readable). In the very rare case we race on the open call, it's fine to open /dev/urandom twice and then just close one.

matklad · 2020-01-02T11:28:28Z

It just occurred to me that if we are using the open syscall here then we ... can just call pthread_lock as well?

josephlr · 2020-01-02T12:18:33Z

It just occurred to me that if we are using the open syscall here then we ... can just call pthread_lock as well?

I looked into doing that originally, and then again when I was fixing this issue. It seems much more complicated to do than the current code. There are a bunch of pthread methods you have to call (or at least the stdlib calls them, I'm not sure the complexity (vs this PR) is worth it.

matklad · 2020-01-02T12:57:56Z

There are a bunch of pthread methods you have to call (or at least the stdlib calls them

stdlib needs to guard agains reentrant locking, which we don't have to, so the implementation is pretty straight forward (modulo the lack of UnsafeSyncCell):

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=053aa21299f80833e3ee779a05bf55c5

I like it because it doges the other edge case -- unbounded number of concurrently opened file descriptors (not that my assertion from the blog that an init functions would be run at most #cores times is wrong).

matklad · 2020-01-02T13:18:40Z

stdlib needs to guard agains reentrant locking, which we don't have to

Well, which I think we don't need to do, for the following reasons:

we don't run any user code while holding the mutex (in particular, no closure, generics or custom Drops are in sight)
so the only reason why we might reenter the function is signal handlers, but we don't advertise getrandom as async safe

gnzlbg · 2020-01-02T16:54:02Z

I like it because it doges the other edge case -- unbounded number of concurrently opened file descriptors

I was just going to mention this. File-descriptors are a finite resource on most OSes, and I'd be more comfortable with a solution that does not risk exhausting them in pathological cases.

josephlr · 2020-01-03T05:26:02Z

@matklad @gnzlbg I think you are both right. The code to use pthreads wasn't that bad (after I added a DropGuard helper type) and it avoids some weird pathological edge cases.

Let me know what you think.

gnzlbg · 2020-01-03T08:59:22Z

src/use_file.rs

+// numbers. The file will be opened exactly once. All successful calls will
+// return the same file descriptor. This file descriptor is never closed.
+fn get_rng_fd() -> Result<libc::c_int, Error> {
+    static FD: AtomicUsize = AtomicUsize::new(LazyUsize::UNINIT);


I'm not sure how often this needs to happen, or whether this happening is worth optimizing, but a different way to implement this is to do something like this to keep the common path branchless: /~https://github.com/calebzulawski/multiversion/pull/6/files#diff-0a878480aac95b54dd822d02c4ad345eR76

cc @TethysSvensson - it might be a thing worth attempting in a subsequent PR after an approach that works "correctly" gets merged.

@gnzlbg The reason why that trick works in multiversion is, that the value we are trying to lazily compute is already a function pointer. This means that the best-case performance is always going to involve at least 1 indirect function call. The trick makes sure that the common path does not pay anything in addition to that 1 indirect function.

Here it looks like you are trying to lazily compute a file descriptor. As far as I can tell, the current cost in the common path is a single conditional branch on a relaxed load. This appears to me to be cheaper than alternative cost of 1 indirect function call on a relaxed load.

I do have one suggestion though, assuming that you would ever want to inline get_rng_fd. I think in that case, you would want to inline the initial check, while keeping the slow initializer un-inlined. I am imagining something like this:

#[inline] fn get_rng_fd() -> Result<libc::c_int, Error> { static FD: AtomicUsize = AtomicUsize::new(LazyUsize::UNINIT); if let Some(fd) = get_fd() { Ok(fd) } else { initialize_slow() } #[inline(always)] fn get_fd() -> Option<libc::c_int> { ... } #[inline(always)] fn initialize_slow() -> Result<libc::c_int, Error> { ... } }

However if I understand this code correctly, the I don't think this function is meant to be inlined(?), so in that case it does not really matter either way.

However if I understand this code correctly, the I don't think this function is meant to be inlined(?), so in that case it does not really matter either way.

This is correct, the only external function of this crate is getrandom::getrandom which is not marked #[inline], so I don't think marking internal functions #[inline] really does anything.

In fact it looks like (in release mode) the compiler just inlines everything in this file into use_file::getrandom_inner. I checked with the current nightly build.

EDIT: here's the ASM when compiled with --release.

src/use_file.rs

matklad

LGTM, noticed only a couple of unrelated nits.

src/use_file.rs

src/util_libc.rs

Signed-off-by: Joe Richey <joerichey@google.com>

Don't spin when polling /dev/random. We also remove the use of spin locks when opening the persistent fd for platforms that require it. For both these cases, we can just use the pthread lock/unlock methods in libc. This includes adding Mutex and DropGuard abstractions. Signed-off-by: Joe Richey <joerichey@google.com>

We no longer use spin-locks anywhere in getrandom, so remove any interfaces which spin. Signed-off-by: Joe Richey <joerichey@google.com>

josephlr · 2020-01-06T22:06:56Z

@dhardy @newpavlov This PR is ready for review/merging. Comments have been addressed, and the CI is passing.

get_rng_fd is the main functional change, most of the remaining changes are either nits or code moving around. I'd either "rebase" or "merge" this PR as to not lose the commit history (where I tried to split up this change).

newpavlov

Looks good to me! I have only one question and if @dhardy does not have any objections I will do the merge.

newpavlov · 2020-01-07T04:08:44Z

src/use_file.rs

+    // before returning, making sure we don't violate the pthread_mutex_t API.
+    static MUTEX: Mutex = Mutex::new();
+    unsafe { MUTEX.lock() };
+    let _guard = DropGuard(|| unsafe { MUTEX.unlock() });


Is it guaranteed that destructor will run right before function exit (be it return or panic)? For some reason I thought that with NLL drop place can change depending on liveness analysis.

NLL (and its future extensions w/ Polonious or what not) is just for Lifetimes. Drop is still scope based, so the drop method is always executed when the object leaves scope. As the guard is at function scope, it drops on return or on panic unwinding.

The liveness analysis is just used to determine if a set of borrows are permitted. This is why (modulo compiler bugs) NLL was introduced without it being a breaking change.

Docs: https://doc.rust-lang.org/book/ch15-03-drop.html

dhardy

One question above, otherwise looks good to me.

dhardy · 2020-01-07T10:47:09Z

src/use_file.rs

+    }
+    unsafe fn lock(&self) {
+        let r = libc::pthread_mutex_lock(self.0.get());
+        debug_assert_eq!(r, 0);


Should we not check the return value in all builds? (Also unlock.)

stdlib also uses debug_assert, and man page specifically says that lock/unlock basically can't fail.

Aha. Closer reading of the possible error values shows that none should be applicable in this case, and if any were that would be a code error, so okay. (Assuming EAGAIN is specific to recursive locks.)

dhardy · 2020-01-07T10:47:11Z

src/use_file.rs

+
+unsafe impl Sync for Mutex {}
+
+struct DropGuard<F: FnMut()>(F);


I'm surprised this isn't a part of the core library!

Often times, this does not work in higher level code, because closure borrows the environment, and so you can't modify it. We only able to use it because we close over low-level file descriptor, which is Copy.

To be clear, it is an oftentimes useful utility, but it is not as useful in practice as it could seem.

EDIT: you can, of course, make a guard smart-pointer with DerefMut (https://docs.rs/scopeguard/1.0.0/scopeguard/#scope-guard-with-value), but that's a more complex API with some design choices.

josephlr requested review from newpavlov and dhardy January 2, 2020 08:38

matklad reviewed Jan 2, 2020

View reviewed changes

src/use_file.rs Outdated Show resolved Hide resolved

josephlr force-pushed the spin branch from 6500307 to 8aaba1c Compare January 2, 2020 11:06

josephlr force-pushed the spin branch from 8aaba1c to ce72175 Compare January 2, 2020 11:21

josephlr changed the title ~~use_file: Avoid use of spin-locks~~ use_file: Remove use of spin-locks Jan 2, 2020

josephlr force-pushed the spin branch from ce72175 to 5485481 Compare January 3, 2020 05:21

gnzlbg reviewed Jan 3, 2020

View reviewed changes

matklad reviewed Jan 3, 2020

View reviewed changes

src/use_file.rs Outdated Show resolved Hide resolved

newpavlov mentioned this pull request Jan 5, 2020

Prepare release v0.1.14 #128

Merged

matklad mentioned this pull request Jan 6, 2020

lazy_static uses spinlocks in my with-std crate rust-lang-nursery/lazy-static.rs#150

Open

josephlr force-pushed the spin branch from 733359d to 29ac2b8 Compare January 6, 2020 19:50

matklad reviewed Jan 6, 2020

View reviewed changes

src/use_file.rs Outdated Show resolved Hide resolved

src/util_libc.rs Outdated Show resolved Hide resolved

josephlr added 3 commits January 6, 2020 13:56

util_libc: open_readonly shoud return a Result

3fdeab7

Signed-off-by: Joe Richey <joerichey@google.com>

util: Remove unused spin-lock interfaces

d728d5b

We no longer use spin-locks anywhere in getrandom, so remove any interfaces which spin. Signed-off-by: Joe Richey <joerichey@google.com>

josephlr force-pushed the spin branch from 29ac2b8 to d728d5b Compare January 6, 2020 21:57

newpavlov approved these changes Jan 7, 2020

View reviewed changes

dhardy reviewed Jan 7, 2020

View reviewed changes

newpavlov merged commit c5e2025 into rust-random:master Jan 7, 2020

josephlr deleted the spin branch January 7, 2020 19:55

josephlr mentioned this pull request Jan 8, 2020

Merge branch 'master' into 0.2 #130

Merged

briansmith mentioned this pull request Jun 17, 2024

use_file: replace mutex with nanosleep-based loop #478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use_file: Remove use of spin-locks #125

use_file: Remove use of spin-locks #125

josephlr commented Jan 2, 2020 •

edited

Loading

matklad left a comment

josephlr commented Jan 2, 2020

matklad commented Jan 2, 2020

josephlr commented Jan 2, 2020

matklad commented Jan 2, 2020

matklad commented Jan 2, 2020

gnzlbg commented Jan 2, 2020 •

edited

Loading

josephlr commented Jan 3, 2020

gnzlbg Jan 3, 2020

TethysSvensson Jan 3, 2020 •

edited

Loading

TethysSvensson Jan 3, 2020

josephlr Jan 3, 2020 •

edited

Loading

matklad left a comment •

edited

Loading

josephlr commented Jan 6, 2020

newpavlov left a comment

newpavlov Jan 7, 2020

josephlr Jan 7, 2020

dhardy left a comment

dhardy Jan 7, 2020

matklad Jan 7, 2020

dhardy Jan 7, 2020

dhardy Jan 7, 2020

matklad Jan 7, 2020 •

edited

Loading


		unsafe impl Sync for Mutex {}

		struct DropGuard<F: FnMut()>(F);

use_file: Remove use of spin-locks #125

use_file: Remove use of spin-locks #125

Conversation

josephlr commented Jan 2, 2020 • edited Loading

matklad left a comment

Choose a reason for hiding this comment

josephlr commented Jan 2, 2020

matklad commented Jan 2, 2020

josephlr commented Jan 2, 2020

matklad commented Jan 2, 2020

matklad commented Jan 2, 2020

gnzlbg commented Jan 2, 2020 • edited Loading

josephlr commented Jan 3, 2020

Choose a reason for hiding this comment

TethysSvensson Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephlr Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

matklad left a comment • edited Loading

Choose a reason for hiding this comment

josephlr commented Jan 6, 2020

newpavlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

josephlr commented Jan 2, 2020 •

edited

Loading

gnzlbg commented Jan 2, 2020 •

edited

Loading

TethysSvensson Jan 3, 2020 •

edited

Loading

josephlr Jan 3, 2020 •

edited

Loading

matklad left a comment •

edited

Loading

matklad Jan 7, 2020 •

edited

Loading