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

Expose weak versions of CAS methods #150

Merged
merged 3 commits into from
Apr 12, 2023
Merged

Conversation

tsoutsman
Copy link
Contributor

Hi,

I'm working on implementing sync primitives in Theseus OS on top of spin locks. We currently have to implement our own spin lock that is identical to spin except that it exposes try_lock_weak, so that we can have custom lock implementations that use a weak CMPXCHG, while still using a strong CMPXCHG for try_lock.

It would be nice if we could upstream this so that we could directly depend on spin. I think it makes sense to include it as spin is often used to build higher-level primitives which could use a weak CMPXCHG for better performance.

Useful when using `spin` to implement higher-level sync primitives.

Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com>
Copy link
Collaborator

@zesterer zesterer left a comment

Choose a reason for hiding this comment

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

Thanks, these changes look good to me. Just a few minor comments. I'm happy to expose this API though.

src/mutex/spin.rs Outdated Show resolved Hide resolved
src/mutex/spin.rs Outdated Show resolved Hide resolved
Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com>
@tsoutsman tsoutsman requested a review from zesterer April 12, 2023 13:17
@zesterer zesterer merged commit 1f2e06c into mvdnes:master Apr 12, 2023
5 checks passed
@zesterer
Copy link
Collaborator

Are you looking to see a new release with this feature?

@tsoutsman
Copy link
Contributor Author

It would be nice but not critical. We're happy to just use a patch manifest section.

@tsoutsman
Copy link
Contributor Author

Sorry, I'm currently implementing our blocking spin lock and noticed there isn't a way to access the reader count from the guard.

Ideally, force_read_decrement would return the previous count so that we know when no readers are left. However, this would be a breaking change and require exposing rw_lock::READER, which is an implementation detail, so I think it may be better if we forked spin rather than upstreaming that change, considering how niche our use case is and how many dependents spin has.

@tsoutsman tsoutsman deleted the expose-weak branch April 13, 2023 01:06
kevinaboos pushed a commit to theseus-os/Theseus that referenced this pull request Apr 13, 2023
* Recent upstream changes exposed the `try_lock_weak()` function
  in `spin`'s `Mutex`, allowing us to use it in our implementation.

* See <mvdnes/spin-rs#150>

Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com>
github-actions bot pushed a commit to theseus-os/Theseus that referenced this pull request Apr 13, 2023
* Recent upstream changes exposed the `try_lock_weak()` function
  in `spin`'s `Mutex`, allowing us to use it in our implementation.

* See <mvdnes/spin-rs#150>

Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com> 8e9638d
github-actions bot pushed a commit to tsoutsman/Theseus that referenced this pull request Apr 14, 2023
…s-os#935)

* Recent upstream changes exposed the `try_lock_weak()` function
  in `spin`'s `Mutex`, allowing us to use it in our implementation.

* See <mvdnes/spin-rs#150>

Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com> 8e9638d
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.

None yet

2 participants