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

common: Implements simple_spin_t in terms of std::atomic_flag. #14370

Merged
merged 1 commit into from Apr 8, 2017

Conversation

Projects
None yet
5 participants
@chardan
Contributor

chardan commented Apr 6, 2017

Note that I'm not sure how this will work on the s360 (but, I /expect/ it to be correct). Similarly, I believe I've picked the most relaxed memory order that will work for the situation, but I'm not 100% sure since I can't try it on all target architectures or stress the concurrency as much as I'd like. Please test carefully!

Signed-off-by: Jesse Williamson jwilliamson@suse.de

@smithfarm smithfarm requested review from tchaikov and badone Apr 6, 2017

@jecluis

This comment has been minimized.

Member

jecluis commented Apr 6, 2017

@chardan didn't review the patch, but looking at the commit message I would like to suggest you change the title to common: implement simple_spin_t in terms of std::atomic_flag, and that you add a brief description of the problem and the fix to the commit message body.

@jecluis jecluis added the common label Apr 6, 2017

@liewegas liewegas changed the title from Implements simple_spin_t in terms of std::atomic_flag. to common/simple_spin: Implements simple_spin_t in terms of std::atomic_flag. Apr 6, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 6, 2017

While we're futzing with this, can we kill atomic_t and use the c++ intrinsics

@badone

This comment has been minimized.

Contributor

badone commented Apr 6, 2017

As suspected the build failure is for the add_executable call to unittest_simple_spin

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 7, 2017

@badone Thanks for catching that! I didn't realize it was part of the test system at first, so I'd taken it out, then put it in back without recommitting-- will fix.

@liewegas Sounds like a good idea to me. Would you rather see that as part of this PR, or another?@smithfarm's PR should make things ok for the s390 WRT this functionality, if there's a delay.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 7, 2017

@chardan I'm not @liewegas but . . . since the immediate problem of the broken s390 build has been addressed there is no hurry and you could just add more commits to this PR.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 7, 2017

@chardan Totally anal nit - could you remove the period from the end of the commit description? The release notes are auto-generated from these descriptions and periods on the end don't work well with the release notes template.

(Hmm, I guess that could be addressed in the release notes script, too! Thanks for giving me the idea.)

UPDATE: see #14385

@smithfarm smithfarm changed the title from common/simple_spin: Implements simple_spin_t in terms of std::atomic_flag. to common: Implements simple_spin_t in terms of std::atomic_flag. Apr 7, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 7, 2017

@chardan Please ignore my totally anal nit. I realized the script processes PR descriptions, not commit descriptions, and we can will strip off any trailing punctuation in the script itself.

common: Implements simple_spin_t in terms of std::atomic_flag
Removes the requirement of machine-specific code in favor of the standard
library. See also PR #14337.

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
@chardan

This comment has been minimized.

Contributor

chardan commented Apr 7, 2017

@smithfarm No worries! (And, fixed anyway.)
@liewegas Looks like there are about 254 occurrences of atomic_t. Additionally, at least one more spinlock implementation, etc.-- looks like it will need some careful attention and be fairly high-touch. If I had to vote, I'd say separate PR. Meanwhile, I'll dive in!

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 7, 2017

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 7, 2017

@liewegas Agreed, I'll set it up separately.
@badone I expect it to build, etc. now.

@liewegas liewegas merged commit 82b3c5d into ceph:master Apr 8, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment