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

p_mutex_*: Implement init/lock/unlock/etc: #162

Closed
wants to merge 2 commits into from

Conversation

indrora
Copy link
Contributor

@indrora indrora commented Jun 18, 2015

Implement the p_mutex functions.

p_mutex_t is a uint32_t* -- We waste 23 bits* on it, but it's a
nice round size.

This follows in the form that pthreads uses. Each of these functions
does a quick NULL check to be sure things aren't bad (except for
p_mutex_create, which checks that we're not trying to initialize an
empty p_mutex_t)

Signed-off-by: Morgan Gangwere [email protected]

* oops -- bits, not bytes.

@indrora
Copy link
Contributor Author

indrora commented Jun 18, 2015

see #160 -> fixes?

}
// Spin while we wait for the lock to become 0.
while(**mp != 0) { ;; }
**mp = 1;
Copy link

Choose a reason for hiding this comment

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

This is obviously not going to work, this is a race condition. Unless you know something about the target architecture that I don't. But generally, this is not going to work. You need an atomic lock instruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless you have a better option (because Atomics aren't part of the C spec we're going for UNLESS we've graduated to C11, which has native atomic operations) and our own atomic implementation isn't ready yet (because it (perversely) will probably depend on mutexes) how do you suggest we fix this chicken-and-egg problem?

@aolofsson
Copy link
Member

Hard to see how this is going to work without assembly... x86, ARM, and Epiphany all have special instructions to handle mutex.

@indrora
Copy link
Contributor Author

indrora commented Jun 19, 2015

I've lifted this quietly out of a systems textbook, based on Tanenbaum, who implements it as a two-state semaphore.

So, unless someone wants to come up and implement TAS, which is the only "required" part of this, this is the "best" solution at the moment.

hardware support is all fine and good, we should let the compiler figure that one out. Unless, of course, we're using C11, in which case, we should be using atomic operations from C11. But we're not, so we're stuck.

@bremby
Copy link

bremby commented Jun 20, 2015

I wouldn't implement this as your solution may give a false hope which may be even worse than not having it implemented at all. The Adapteva guys should decide what they want: either C11, compiler intrinsics or assembler.

indrora added 2 commits June 23, 2015 15:51
Implement the p_mutex functions.

p_mutex_t is a uint32_t* -- We waste 23 bytes on it, but it's a
nice round size.

This follows in the form that pthreads uses. Each of these functions
does a quick NULL check to be sure things aren't bad (except for
p_mutex_create, which checks that we're not trying to initialize an
empty p_mutex_t)

Signed-off-by: Morgan Gangwere <[email protected]>
Change up locking logic such that race conditions are
less of a problem.

in p_mutex_lock: spin until a lock is gotten *and* it
equals 1 with two nested loops and atomic operations only.

in p_mutex_trylock: fail early, but an atomic branch will
cause a small check to see if a race happened *and* the thread
asking for the mutex actually got the lock. No loops, only
returns.

Signed-off-by: Morgan Gangwere <[email protected]>
@indrora
Copy link
Contributor Author

indrora commented Jun 23, 2015

Thanks, git. I had to force push to my repo!

@indrora
Copy link
Contributor Author

indrora commented Jun 24, 2015

@bremby - There's nothing "special" about my solution -- it's a fairly textbook mutex, straight out of most university Operating Systems courses.

The C11 atomics aren't all that special, either. It'd reduce code-size by a tiny bit, but not much, plus requires compiler support. It doesn't quite help us.

The current patch ( @ c399aa6 ) does the set-and-test in as atomic a fashion as possible. In the case of p_mutex_lock, if the thread asking for a mutex manages to get a value other than 1 (a non-zero value indicating a locked mutex) it waits until it can attempt to take it again.

Yes priority inversion is possible. No, it can't be solved here. That's the job of Semaphores.

@bremby
Copy link

bremby commented Jun 25, 2015

There's nothing "special" about my solution -- it's a fairly textbook mutex, straight out of most university Operating Systems courses.

...yet it's still incorrect. Suppose 2 threads, *_mp == 0, one thread sets *_mp += 1 and gets rescheduled, second thread does the same thing, now **mp == 2, and your threads deadlock. Furthermore, your variable isn't volatile, which may result in compiler optimizing out your code and making it even worse than it is.

Secondly, what I've been taught on my university Operating Systems course (obviously much different from your university), mutexes provide higher-level API which provides not only mutual exclusion, but also yielding (sleeping), wake-up events and guarantee no thread starves. Your 'mutexes' resemble spinlocks.

But anyway, as I said previously, please don't try to implement mutexes unless you can guarantee they will work always (under every scheduling possible, however unlikely). Having mutexes working only partially or sometimes is worse than having none at all.

@olajep
Copy link
Member

olajep commented Aug 10, 2015

Implemented in #205

@olajep olajep closed this Aug 10, 2015
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.

4 participants