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

i#2502 locks: Add acquires to bare loads in lock routines #5370

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

derekbruening
Copy link
Contributor

Adds load-acquire semantics to a number of bare loads (i.e., not
inside a mutex) in DR's various lock routines.

This includes a key missing load-acquire in the spinlock loop in
d_r_mutex_lock_app() which might help explain some performance issues
such as #4279.

Since some of these are in the utils.h header, and we have ordering
dependencies between utils.h and arch_exports.h, this required
splitting the atomic defines out of arch/arch_exports.h into a new
file arch/atomic_exports.h. Since those atomic defines use ASSERT
which references dynamo_options, this further required splitting the
option struct out of options.h into options_struct.h.

Issue: #2502, #4928, #4279

Adds load-acquire semantics to a number of bare loads (i.e., not
inside a mutex) in DR's various lock routines.

This includes a key missing load-acquire in the spinlock loop in
d_r_mutex_lock_app() which might help explain some performance issues
such as #4279.

Since some of these are in the utils.h header, and we have ordering
dependencies between utils.h and arch_exports.h, this required
splitting the atomic defines out of arch/arch_exports.h into a new
file arch/atomic_exports.h.  Since those atomic defines use ASSERT
which references dynamo_options, this further required splitting the
option struct out of options.h into options_struct.h.

Issue: #2502, #4928, #4279
@derekbruening
Copy link
Contributor Author

The header code that was moved was not modified. All the not-just-moved modifications (the inserted acquires) are in utils.c and utils.h.

Unfortunately there isn't a simple way to test these things. I ran dr$sim -offline thousands of times on the Jenkins machine and there are no hangs so this doesn't make anything worse; there is still a hang in dr$sim online (#4928). I don't have a setup to easily test performance losses or gains; we'll try to ask on #4279 to do that once this is in to see if it helped there.

@derekbruening derekbruening merged commit 5da8df6 into master Feb 18, 2022
@derekbruening derekbruening deleted the i4928-a64-hang branch February 18, 2022 17:55
derekbruening added a commit that referenced this pull request Mar 27, 2022
Fixes a regression from PR #5370 which added asserts to various DR
lock routines which are not valid for client locks marked as the app.

Adds sanity tests for each of the 3 types of locks that can be marked
as app locks.  These hit the asserts without this fix.

Fixes #5432
derekbruening added a commit that referenced this pull request Mar 27, 2022
Fixes a regression from PR #5370 which added asserts to various DR
lock routines which are not valid for client locks marked as the app.

Adds sanity tests for each of the 3 types of locks that can be marked
as app locks.  These hit the asserts without this fix.

Fixes #5432
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.

2 participants