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

use_file: replace mutex with nanosleep-based loop #478

Closed
wants to merge 3 commits into from

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Jun 16, 2024

Replaces libc::pthread_mutex_t with libc::nanosleep-based wait loop with bounded exponential backoff. This approach is similar to what we use in the VxWorks backend.

This eliminates the problematic dependency on pthread mutex, reduces amount of unsafe and makes code slightly more straightforward. It also resolves concerns about atomic ordering from #469 without hurting the optimistic path.

The main disadvantage of this approach is somewhat higher latency in pathological cases (e.g. spawning thousands of threads which simultaneously call getrandom). This especially could be noticeable on Linux booted without sufficient entropy, i.e. when program spends a significant amount of time in wait_until_rng_ready. But such cases should be extremely rare in practice.

On Linux we could use the futex syscall to implement a simple quasi-mutex based on the FD atomic.

@newpavlov newpavlov requested a review from josephlr June 16, 2024 15:05
@newpavlov
Copy link
Member Author

cc @briansmith

@briansmith
Copy link
Contributor

Replaces libc::pthread_mutex_t with libc::nanosleep-based wait loop with bounded exponential backoff. This approach is similar to what we use in the VxWorks backend.

Didn't getrandom replace its previous use of spin locks because of the blog post that pointed out issues with spin locks in getrandom, in PR #125?

My hope is that we can figure out an efficient solution that doesn't require us to implement our own synchronization primitives, or at least limit them to very basic atomics.

As I mentioned elsewhere, I filed rust-lang/rust#126239 so that the language team can clarify the Mutexes semantics to make it clearer that the way we use the libstd Mutex is correct.

@newpavlov
Copy link
Member Author

IIRC the main problem with the old spinlock implementation was priority inversion in the polling loop.

On Linux futex-based synchronization should be relatively easy to implement. The problem is with the other targets, which do not have futex-like APIs.

@newpavlov
Copy link
Member Author

I will close this PR and will try a different approach based futex on Linux/Android and std::sync::Mutex on other targets in a different PR later.

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