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

threads: enable support for spinlock #324

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

loganek
Copy link
Contributor

@loganek loganek commented Sep 5, 2022

No description provided.

@loganek loganek changed the title threads: implement support for spinlock threads: enable support for spinlock Sep 5, 2022
@sbc100 sbc100 requested a review from abrown September 5, 2022 17:10
@sunfishcode
Copy link
Member

Could you say more about the context for this? WASI doesn't currently expose hooks for configuring the scheduler, so it seems like it'd be risking priority inversions and deadlocks. An alternative might be to implement the spin lock API with regular locks, however we should ideally look at how people expect to use these APIs.

@loganek
Copy link
Contributor Author

loganek commented Sep 6, 2022

@sunfishcode I'm just trying to compile sample applications to run on WASM runtime and I don't have any specific use case for this for now. I'm a bit reluctant implementing a spinlock differently than spinlocks are normally implemented as this might be confusing and won't meet certain expectations that people have when use it.
Having said that, I do understand your concerns, and I don't mind waiting for a bit to gather more feedback on that.

@sbc100
Copy link
Member

sbc100 commented Sep 6, 2022

I don't see how these are any different to spinlocks that one can already write using raw atomic instructions. I don't see any reason to diverge from the upstream musl code here. Spinlocks are already something that one should be using at ones own risk.

The fact that we don't expose "hooks for configuring the scheduler" perhaps makes them, if anything, slightly safer to use that on other platforms that do expose such things since the likelyhood of threads within a wasm program having a different priority is therefore lower (than in a program that is able to access such hooks and meddle with priorities).

@sunfishcode
Copy link
Member

I myself don't know of any real-world code that people want to run on wasm that uses the pthread spin lock API. If anyone has any such code, I'm interested in learning about it, and what assumptions it might be relying on.

I don't have a major objection to adding this, I just want to take what appears to be an opportunity to ask this question.

Copy link
Contributor

@yamt yamt left a comment

Choose a reason for hiding this comment

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

honestly speaking, i think pthread spin lock is a misfeaure with no good applications.
however, as it's in the standard, it isn't surprising if some applications are using it.

@loganek
Copy link
Contributor Author

loganek commented Jan 26, 2023

I still think we should enable it; spin lock has very clear definition and it's up to the user to use it or not.

Copy link
Collaborator

@abrown abrown left a comment

Choose a reason for hiding this comment

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

I agree with @sunfishcode's point that spin locks in WebAssembly seem a bit weird, but I do think that, for compatibility sake, having this functionality available in wasi-libc makes sense. Let's merge it!

@abrown abrown merged commit b481499 into WebAssembly:main Jan 26, 2023
abrown added a commit to abrown/wasi-sdk that referenced this pull request Jan 27, 2023
sunfishcode pushed a commit to WebAssembly/wasi-sdk that referenced this pull request Jan 28, 2023
* Update wasi-libc to latest HEAD

This pulls in a variety of changes, many of them related to enabling
threads in wasi-libc. There may be more changes necessary before
releasing a prelease version of wasi-sdk that supports the new
`wasm32-wasi-threads` target, but this should get the majority of them.

* review: incorporate WebAssembly/wasi-libc#324
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
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.

5 participants