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

std: use block_current_task for thread parking on Hermit #98534

Closed
wants to merge 1 commit into from

Conversation

joboet
Copy link
Contributor

@joboet joboet commented Jun 26, 2022

Mutex and Condvar are being replaced by more efficient implementations, which need thread parking themselves (see #93740). Therefore, the generic Parker needs to be replaced on all platforms where the new lock implementation will be used.

Hermit has the block_current_task(_with_timeout)/wakeup_task syscalls for building custom synchronization primitives. block_current_task sets the thread's state to parked. The next yield to the scheduler will block until the task is woken up again. As Hermit does not use preemptive multitasking, this can be used to run userspace code (e.g. checking a condition) in between the syscalls.

This implementation uses an atomic state variable, similar to the futex parker, but instead of a PARKED state it stores the parking threads PID. The syscalls are used to emulate a futex operation:

  1. Set the task state to blocked (block_current_task)
  2. Check the state:
    a. if it was set to notified while changing the task state, "wakeup" the task, consume the token and return
    b. else sleep (yield_now)

CC @stlankes, @mkroening

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 26, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2022
@rust-log-analyzer

This comment has been minimized.

@joboet

This comment was marked as off-topic.

@bors
Copy link
Contributor

bors commented Jun 26, 2022

@joboet: 🔑 Insufficient privileges: not in try users

Copy link
Contributor

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @joboet.

This looks very good to me! 👍

Copy link
Contributor

@stlankes stlankes left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @joboet.

@bors
Copy link
Contributor

bors commented Jun 26, 2022

☔ The latest upstream changes (presumably #98545) made this pull request unmergeable. Please resolve the merge conflicts.

@joboet joboet force-pushed the hermit_parker branch 2 times, most recently from e26c164 to dc14f3f Compare June 27, 2022 09:44
@joboet
Copy link
Contributor Author

joboet commented Jul 18, 2022

r? rust-lang/libs

// Acquire ordering is necessary to obtain the token, as otherwise the parked thread
// could perform memory operations visible before it was unparked. Since once set,
// the token cannot be unset by other threads, the state can be reset with a relaxed
// store once it has been read with acquire ordering.
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is convincing to me. My understanding (perhaps flawed, I'm definitely not an expert in this) is that relaxed doesn't guarantee that other threads will observe the new empty state; we need a release store that synchronizes with the other thread's acquire ordering in order to guarantee correct behavior here.

In general, I'm not sure that hermit should deviate from the pattern of the general unix thread parker implementation, which uses SeqCst ordering for all of these operations.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding (perhaps flawed, I'm definitely not an expert in this) is that relaxed doesn't guarantee that other threads will observe the new empty state; we need a release store that synchronizes with the other thread's acquire ordering in order to guarantee correct behavior here.

That's not correct. SeqCst or Release+Acquire also don't guarantee any other thread will directly observe the new value either. The specification only talks about ordering, not about timing. And even Relaxed guarantees a perfectly consistent ordering for all operations on the same atomic variable. Using stronger memory ordering only means that you can under some conditions guarantee that something that happened to another piece of memory is observable once an atomic store/change is observed, but is unrelated to when that change is observed in the first place.

library/std/src/sys/hermit/thread_parker.rs Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

I'm also going to r? @m-ou-se who has put a lot more thought recently into this, since I suspect it'll be easy for me to miss something when reviewing this, and since I'm less than confident about the Relaxed memory ordering usage here.

@bors
Copy link
Contributor

bors commented Jul 24, 2022

☔ The latest upstream changes (presumably #99251) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@joboet

This comment was marked as outdated.

@joboet
Copy link
Contributor Author

joboet commented Sep 4, 2022

Since Hermit now has futex syscalls, I'm going to close this PR, as Hermit can now share the futex parker used by other systems.

@joboet joboet closed this Sep 4, 2022
@joboet joboet deleted the hermit_parker branch February 18, 2023 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants