Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Added support for most of <mutex> #113

Closed
wants to merge 4 commits into from
Closed

Added support for most of <mutex> #113

wants to merge 4 commits into from

Conversation

ogiroux
Copy link
Contributor

@ogiroux ogiroux commented Feb 2, 2021

Has mutex, timed_mutex, once_flag, call_once, unique_lock, scoped_lock, and varied free functions that go with them. Excludes only condition variable support and the recursive versions of mutex.

@wmaxey wmaxey self-assigned this Feb 3, 2021
@wmaxey
Copy link
Member

wmaxey commented Feb 3, 2021

TODO: Test ports and heterogeneous tests.

@brycelelbach brycelelbach modified the milestones: 2.0.0, 2.1.0 Feb 11, 2021
@brycelelbach brycelelbach added enhancement New feature or request. P1: should have Important but not necessary for release. labels Mar 29, 2021
@brycelelbach brycelelbach modified the milestones: 2.1.0, 1.6.0 May 18, 2021
@wmaxey wmaxey modified the milestones: 1.6.0, 2.0.0 Jul 28, 2021
@jrhemstad
Copy link
Collaborator

jrhemstad commented Jan 27, 2022

@wmaxey @ogiroux is this just waiting on porting tests? I'd like to move forward with exploring NVIDIA/cccl#990, but it depends on having a mutex first.

@miscco
Copy link
Collaborator

miscco commented Feb 23, 2023

@jrhemstad How important is this and should I try to get this rebased?

@jrhemstad
Copy link
Collaborator

@jrhemstad How important is this and should I try to get this rebased?

It's important, but not urgent. No need to try and get it merged before the next release. There is still a sizable amount of work to add tests before it can be merged anyways.

@miscco miscco force-pushed the mutex branch 4 times, most recently from 0f16134 to 2c19f4a Compare March 10, 2023 15:52
@miscco miscco force-pushed the mutex branch 2 times, most recently from 4952f9e to e8b2e53 Compare March 22, 2023 19:30
@miscco miscco requested a review from gevtushenko March 22, 2023 19:31
include/cuda/std/mutex Outdated Show resolved Hide resolved
include/cuda/std/mutex Show resolved Hide resolved
include/cuda/mutex Show resolved Hide resolved
handler.join_test_thread();
handler.syncthreads();

assert(init3_called[Sco] == 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert(init3_called[Sco] == 2);
assert(init3_called[Sco] == 1);

Comment on lines 85 to 56
if (init3_called[Sco] == 1)
#ifdef __CUDA_ARCH__
_LIBCUDACXX_UNREACHABLE();
#else
TEST_THROW(1);
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (init3_called[Sco] == 1)
#ifdef __CUDA_ARCH__
_LIBCUDACXX_UNREACHABLE();
#else
TEST_THROW(1);
#endif
assert(init3_called[Sco] == 1);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically that would not be the same. We need to throw to ensure that we do not increment the init3_called variable afterwards but continue the test as expected

@miscco
Copy link
Collaborator

miscco commented Mar 30, 2023

While everything works fine locally, this PR is incredibly flaky in CI.

We need to investigate why that is the case and find a strategy to unflake it

if (__m_ == nullptr)
__throw_system_error(EPERM, "unique_lock::lock: references null mutex");
if (__owns_)
__throw_system_error(EDEADLK, "unique_lock::lock: already locked");
#endif // _LIBCUDACXX_NO_EXCEPTIONS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without exceptions, on the host, we should be always printing the error, and aborting.

Without exceptions, on the device, we should be __traping, and when on debug mode, we should be printing the error.

Same for all other error handling in this PR for which we are already producing errors.

using __libcpp_mutex_base_t = __libcpp_mutex_t;
#else
template<int _Sco>
using __libcpp_mutex_base_t = __atomic_semaphore_base<_Sco,1>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some comments would help here, but from skimming through the implementation of __atomic_semaphore_base, it seems that its not fair, which will starve threads from different NUMA nodes at system scope?

@@ -274,6 +345,8 @@ void
swap(unique_lock<_Mutex>& __x, unique_lock<_Mutex>& __y) _NOEXCEPT
{__x.swap(__y);}

#ifndef _LIBCUDACXX_HAS_THREAD_API_CUDA
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is condition variable supported?

@miscco
Copy link
Collaborator

miscco commented Jul 10, 2023

Dropping in favor of NVIDIA/cccl#187

@miscco miscco closed this Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request. P1: should have Important but not necessary for release.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Implement std::mutex
7 participants