Skip to content
This repository was archived by the owner on Jul 6, 2019. It is now read-only.

[WIP] Synchronization primitives #11

Merged
merged 7 commits into from
May 9, 2014
Merged

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented May 4, 2014

Here is the current state of my mutex support. Progress on this has been pretty sporatic and things are still in a state of minor disarray but I've cleaned things up the best I can and rebased against master for reference.

There are a few things I should point out there,

  • This has only been compile-tested as I don't have hardware to test with at the moment
  • I'm not entirely convinced that the IrqDisabled token is reasonable given how it is abused in Mutex::lock. The idea here is that operations on Queue (which is ideally thread-safe) require that you prove that the caller is in a critical section with interrupts disabled. It might be better to instead use a reference-counting approach here instead.
  • It seems like the interface exposed by task could use some cleaning up. At the moment I've just marked a few things as pub to get things to compile but things could be a bit cleaner.
  • Again, this really hasn't been tested. Feedback is welcome, however.

@bgamari
Copy link
Contributor Author

bgamari commented May 4, 2014

7408c0e sketches what I mean by "reference counting" of critical sections (a poor choice of words).

@bgamari
Copy link
Contributor Author

bgamari commented May 4, 2014

a53c68b is another refinement.

static mut irq_level : uint = 0;

#[inline(always)]
pub fn disable_irqs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add some documentation here about which irqs are/aren't disabled. cpsid i doesn't disable Reset, HardFault or NMI. cpsid f doesn't disable NMI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@bharrisau
Copy link
Contributor

The cortex M3/M4 should be using LDREX to get the lock. Only the M0/M0+/M1 need to disable/enable interrupts.

Also thinking this would be more flexible (and easier to review) if you start with a lock primitive in cortex_m3 and build the mutex/condition variable on top of that.

Also needs a rebase/squash.

@bgamari
Copy link
Contributor Author

bgamari commented May 9, 2014

@bharrisau, thanks! I'll rework things a bit more and try my hand at using LDREX.

@bgamari
Copy link
Contributor Author

bgamari commented May 9, 2014

@bharrisau, how does this look? As far as I can tell, rustc is dropping the ldrex in Lock::unlock.

@farcaller
Copy link
Member

That one looks nice, I'll drop a few style comments in a moment.

#[must_use]
pub struct Guard<'a>(&'a Lock);

pub static STATIC_Lock: Lock = Lock { locked: Unsafe { value: false, marker1: InvariantType } };
Copy link
Member

Choose a reason for hiding this comment

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

Should be STATIC_LOCK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duly noted.

@farcaller
Copy link
Member

Also, a few generic comments: use 2-space idents and make sure you have licence headers in all new files.

Would it be reasonable to make CritSection API unsafe until we actually make it safe, so that we don't forget to do that? A dedicated tracking bug might also be useful.

@bgamari bgamari changed the title [WIP] Mutex support [WIP] Synchronization primitives May 9, 2014
*/
pub fn lock<'a>(&'a self) -> Guard<'a> {
unsafe {
// we need the critical section until the end of this function
Copy link
Member

Choose a reason for hiding this comment

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

Extra space before //

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh!

bgamari added 5 commits May 9, 2014 12:29
This is a basic locking primitive for buillding synchronization
primitives.
This implements a simple abstraction for critical sections where
interrupts must be disabled.
This is a simple container for thread-safe state with access guarded by
a CritSection.
This implements a simple thread-safe queue.
pub fn try_lock<'a>(&'a self) -> Option<Guard<'a>> {
unsafe {
match *self.owner.get() {
None => {
Copy link
Member

Choose a reason for hiding this comment

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

None => {

@farcaller
Copy link
Member

LGTM

farcaller added a commit that referenced this pull request May 9, 2014
Added a few synchronization primitives.
@farcaller farcaller merged commit a26f8be into hackndev:master May 9, 2014
@bharrisau
Copy link
Contributor

Good job. Once we get a solution for bit-banded access we can add a third method for handling locks - bit band writes are atomic so you can do a 32 position lock with low overhead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants