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

feat: add RwSpinlock readers-writer lock #18

Merged
merged 1 commit into from Oct 18, 2023

Conversation

mkroening
Copy link
Contributor

This adds the RwSpinlock implementation from hermit-sync, which has been derived from spin, but

  • with separation of UPGRADABLE and EXCLUSIVE,
  • with optional exponential backoff,
  • with impl RawRwLockRecursive.

Depends on #16 for the backoff feature.
Closes #15

@mkroening
Copy link
Contributor Author

Rebased on latest master. Would you like more elaborate documentation and examples for these new types?

@mkroening mkroening mentioned this pull request Oct 17, 2023
@phil-opp
Copy link
Member

Looks like this still uses the backoff feature instead of the Relax parameter.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks a lot!

Note that I'm not an expert atomic operations, so I'm not 100% certain that all the orderings etc are correct. They looked all reasonable to me, though, and I didn't notice any issues.

It would be great if we could apply some more tooling to catch potential issues at some point (in some future PR). For example, we could run the randomized lock/unlock tests under a data race detector.

src/rw_spinlock.rs Outdated Show resolved Hide resolved
src/rw_spinlock.rs Outdated Show resolved Hide resolved
Signed-off-by: Martin Kröning <martin.kroening@eonerc.rwth-aachen.de>
@mkroening
Copy link
Contributor Author

mkroening commented Oct 17, 2023

Looks like this still uses the backoff feature instead of the Relax parameter.

Fixed. I forgot during rebase. 😅

It would be great if we could apply some more tooling to catch potential issues at some point (in some future PR). For example, we could run the randomized lock/unlock tests under a data race detector.

Good point. Did you have any specific tool in mind?

I took a look at loom once, but their atomics are not const-constructible, so that clashes with lock_api. We might work around that, but that is probably a larger effort.

@phil-opp
Copy link
Member

Thanks!

I took a look at loom once, but their atomics are not const-constructible, so that clashes with lock_api. We might work around that, but that is probably a larger effort.

Yeah, looks like loom will be difficult to use then. I found this related issue: tokio-rs/loom#170

As an alternative, we could try Miri or ThreadSanitizer, which provide less strong guarantees, but should be easier to set up.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fixes!

@phil-opp phil-opp merged commit 9d803fe into rust-osdev:master Oct 18, 2023
4 checks passed
@mkroening mkroening deleted the rwlock branch October 18, 2023 07:28
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.

Readers-writer lock
2 participants