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

Fix CLockFreeGuard, introduce AtomicMutex #1797

Merged
merged 6 commits into from
Mar 6, 2023
Merged

Conversation

prasannavl
Copy link
Member

@prasannavl prasannavl commented Mar 6, 2023

/kind fix

  • CLockFreeGuard uses barrier that's unsuitable for general use case, and results in multiple threads entering critical sections due to premature barrier optimizations. This fixes it.
  • Additionally, the 1ms thread sleep isn't helpful, which on platforms like Linux which default to higher resolution thread quantums might be palatable, but will have heavy regressive behaviour on platforms like Windows which default to 15ms quantum slices.
  • Introduce AtomicMutex that progressively spins, yields and sleeps to remove this, while also working with regular unique_lock patterns.
  • The goal is to remove CLockFreeGuard entirely in subsequent PRs and replace it with standard patterns of std::unique_lock coupled with AtomicMutex

Comment on lines +385 to +387
// We could theoritically use CAS here to prevent the additional write, but
// but requires loop on weak, or using strong. Simpler to just use an exchange for
// for now, since all ops are seq_cst anyway.
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
// We could theoritically use CAS here to prevent the additional write, but
// but requires loop on weak, or using strong. Simpler to just use an exchange for
// for now, since all ops are seq_cst anyway.
// We could theoritically use CAS here to prevent the additional write
// but requires loop on weak, or using strong. Simpler to just use an exchange
// for now, since all ops are seq_cst anyway.

@Bushstar Bushstar merged commit 83e4daf into master Mar 6, 2023
@Bushstar Bushstar deleted the pvl/fix-atomic-locks branch March 6, 2023 04:27
@dcorral dcorral mentioned this pull request Mar 7, 2023
40 tasks
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.

4 participants