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.mutex: implement blocking mutexes on Linux #1463

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@shawnl
Copy link
Contributor

commented Sep 3, 2018

see #1455

Eventually I would like to see something like WTF::Lock/WTF::ParkingLot,
but that is a bunch of work.

@shawnl shawnl force-pushed the shawnl:mutex branch 2 times, most recently from 41ab32c to 8905f96 Sep 3, 2018

std.mutex: implement blocking mutexes on Linux
see #1455

Eventually I would like to see something like WTF::Lock/WTF::ParkingLot,
but that is a bunch of work.

@shawnl shawnl force-pushed the shawnl:mutex branch from 8905f96 to a2732fc Sep 3, 2018

@andrewrk andrewrk added this to the 0.4.0 milestone Sep 3, 2018

std.mutex: leave non-supported OSes as a spin-lock
sto that the tests can pass
@andrewrk
Copy link
Member

left a comment

This is an important component, so I am compelled to have high standards for acceptance. The logic here is nontrivial and difficult to reason about (because it deals with concurrency and atomic operations). So we need these things in order to accept it:

  • Tests.
  • Comments explaining in detail how the implementation works, so that one can verify the code matches the intended behavior.
  • Documentation comments explaining the API, and what guarantees the user can or cannot get from it.

pub const Held = struct {
mutex: *Mutex,

pub fn release(self: Held) void {
assert(@atomicRmw(u8, &self.mutex.lock, builtin.AtomicRmwOp.Xchg, 0, AtomicOrder.SeqCst) == 1);
if (@atomicRmw(u32, &self.mutex.lock, AtomicRmwOp.Sub, 1, AtomicOrder.Release) != 1) {
self.mutex.lock = 0;

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 3, 2018

Member

You can't have a naked write racing with an atomic write (the cmpxchg on line 42).

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 3, 2018

Member

Why doesn't the @atomicRmw do an xchg and set the value to 0? You can still check if the previous value was 1.

while (@atomicRmw(u8, &self.lock, builtin.AtomicRmwOp.Xchg, 1, AtomicOrder.SeqCst) != 0) {}
var c: u32 = undefined;
// This need not be strong because of the loop that follows.
// TODO implement mutex3 from https://www.akkadia.org/drepper/futex.pdf in x86 assembly.

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 3, 2018

Member

Is that really better? Can we have an issue to discuss it rather than this TODO comment?

// spin-lock
}
}
if (@cmpxchgWeak(u32, &self.lock, 0, 2, AtomicOrder.Acquire, AtomicOrder.Monotonic)) |value2| {

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 3, 2018

Member

Why does this jump straight to the "locked with waiters" state? shouldn't it just go back and try the original cmpxchg again?

@andrewrk andrewrk added the userland label Oct 3, 2018

@andrewrk

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

This is actually pretty well tested by the std.atomic.Queue tests. So I'm ok with using that as coverage.

@andrewrk andrewrk closed this in 66cb75d Oct 3, 2018

@shawnl shawnl deleted the shawnl:mutex branch Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.