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 fio_atomic_xchange fallback implementation for older GCC #55

Merged
merged 1 commit into from
Feb 26, 2019

Conversation

Low-power
Copy link
Contributor

@Low-power Low-power commented Feb 26, 2019

An or operation can't exchange those 2 values as expected in fio_atomic_xchange, makes fio_unlock no effect.

@codecov-io
Copy link

codecov-io commented Feb 26, 2019

Codecov Report

Merging #55 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   50.26%   50.32%   +0.06%     
==========================================
  Files          33       33              
  Lines       13432    13432              
==========================================
+ Hits         6751     6760       +9     
+ Misses       6681     6672       -9
Impacted Files Coverage Δ
lib/facil/fio.h 82.7% <ø> (ø) ⬆️
lib/facil/fio.c 58.82% <0%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25f1229...ab6885c. Read the comment docs.

@Low-power
Copy link
Contributor Author

Although nand operation can't really exchange any values, but 0 and 1, this is enough to fix the fio_unlock bug, since it only use 0 and 1.

@Low-power
Copy link
Contributor Author

Low-power commented Feb 26, 2019

I force pushed a new commit that uses __sync_val_compare_and_swap instead.

Reference: https://gcc.gnu.org/onlinedocs/gcc-4.1.0/gcc/Atomic-Builtins.html

@Low-power
Copy link
Contributor Author

Low-power commented Feb 26, 2019

Sorry, the function names was missing a _ character in my last commit. Fixed now.

@boazsegev
Copy link
Owner

Thank you for the PR!
☺️👍🏻🎉👏🏻

@boazsegev boazsegev merged commit cf0a6be into boazsegev:master Feb 26, 2019
@boazsegev
Copy link
Owner

I should probably find a way to test on a wider range of compilers, as well as testing with and without the makefile (since this is a possible use-case).

Thank you very much for exposing this 🙏🏻☺️

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.

3 participants