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

Add a simple spinlock mutex type #607

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

stephenswat
Copy link
Member

In order to facilitate some edge case handling in device code, this commit adds a very simple spinlock-type mutex using vecmem atomic references.

@stephenswat stephenswat added the feature New feature or request label Jun 7, 2024
@stephenswat
Copy link
Member Author

This will fail until acts-project/vecmem#275 is merged.

@stephenswat stephenswat marked this pull request as draft June 7, 2024 13:50
@stephenswat stephenswat force-pushed the feat/mutex branch 4 times, most recently from 53416c5 to a2394c9 Compare June 13, 2024 15:51
@stephenswat
Copy link
Member Author

This is now ready for review.

@stephenswat stephenswat marked this pull request as ready for review June 13, 2024 15:51
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Is the templating actually needed? Could we not just choose one specific integer type that would need to be used with mutexes? This way we just make it easier for somebody to use an inappropriate type. 🤔 (With a bunch of complicated compiler errors to follow.)

Of course in that case the functions will all need to be explicitly declared inline. (They are currently implicitly that because of the templating.)

@krasznaa
Copy link
Member

Also-also: Would it not be good to also provide a traccc::device::lock_guard type? Since it's usually not a good idea to call lock() and unlock() on a mutex manually. 🤔

Unfortunately I don't think that we could use std::lock_guard with CUDA. (I'm pretty sure that SYCL would accept it though.) Since that would be the nicest, using our custom mutex with the general lock guard...

@stephenswat
Copy link
Member Author

stephenswat commented Jun 13, 2024

Unfortunately, it's not that simple. 😟 Because RAII is implicitly executed by every thread in a warp, using a naive lock_guard structure would guarantee deadlock as 32 threads simultaneously try to lock the same mutex. The way out of this would be to implement lock_guard-like types for every platform, but then we start hiding which thread owns the lock from the user, which is in itself bad if they want to do sub-group level work division... The only way I can image that would make this work is to incorporate cooperative groups into the mix, but we don't use those anywhere, and while SYCL has an equivalent semantic construct I am not sure if HIP, Alpaka and Kokkos do.

@stephenswat
Copy link
Member Author

Is the templating actually needed? Could we not just choose one specific integer type that would need to be used with mutexes?

The danger here is that different platform might support lock-free operations on different types. And some types may be slower than others. Although I am pretty sure something like uint32_t is a sane choice on most platforms, I think it makes sense to leave the choice open to the user. A good default value is probably a nice idea, though.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Any chance of adding some SYCL tests as well...? Read: Please add SYCL tests as well.

@stephenswat
Copy link
Member Author

Sure, I can add some SYCL tests. But this kind of "write one piece of common code, have to write tests for all platforms" is going to quickly become unmanageable if we start doing the same for Kokkos, Alpaka, etc. 😟

@krasznaa
Copy link
Member

I'm very open to suggestions. But we must test it somehow that the "common code" indeed works correctly on all backends that we want to use. If you prefer to set up just one test with Alpaka or Kokkos for all available backends, I'm fine with that.

@stephenswat stephenswat force-pushed the feat/mutex branch 2 times, most recently from 6312b8d to 26b48ad Compare June 18, 2024 13:03
@stephenswat
Copy link
Member Author

This is now ready, with SYCL tests.

In order to facilitate some edge case handling in device code, this
commit adds a very simple spinlock-type mutex using vecmem atomic
references.
The purpose of this class is to provide an RAII-based mechanism for
releasing locks on mutexes.
This commit adds some tests for the newly introduced `traccc::mutex` and
`traccc::unique_lock` types for CUDA and SYCL platforms.
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

The lock <-> _lock names are used in a bit confusing way in the SYCL tests, but since those are just tests, this is more of a nitpick. Other than that, this seems fine.

@stephenswat stephenswat merged commit 5061b38 into acts-project:main Jun 20, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants