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

test: migrate atomic_t to std::atomic #14655

Merged
merged 1 commit into from May 24, 2017

Conversation

Projects
None yet
4 participants
@chardan
Contributor

chardan commented Apr 20, 2017

The purpose of these PRs is to remove atomic_t, replacing it with the standard library. No new features should be added, and no regressions should be caused. Ultimately, the intent is to entirely remove "include/atomic.h".

The other PRs are:
rgw:
#15001

librados,libradosstriper,test:
#14658

common,osdc,filestore,test,msg:
#14866

osdc,mds,filestore,librbd,xio,messenger:
#14657

PR #14502 is superseded by these PRs.

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 20, 2017

@liewegas PRs moved here. I hope this is closer to what you were after!

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 20, 2017

I know that this particular PR won't build standalone (others probably not, either). Suggestions as to how this should be resolved?

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 20, 2017

If there are patches that need to go first, let's test+merge that first, and then do the others after. Probably the common/ etc one?

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 25, 2017

@liewegas Ok, let's start with "rodan"!
#14656

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 25, 2017

@liewegas "baragon" should be ready for a look now.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 27, 2017

Jenkins re-test this please

@liewegas liewegas added bug fix cleanup and removed bug fix labels May 1, 2017

@chardan chardan changed the title from migrate atomic_t to std::atomic (gojira) to misc., test: migrate atomic_t to std::atomic May 15, 2017

uint64_t start = Cycles::rdtsc();
for (int i = 0; i < count; i++) {
value.compare_and_swap(test, test+2);
value.compare_exchange_weak(test, test+2);

This comment has been minimized.

@tchaikov

tchaikov May 16, 2017

Contributor

i believe this should be value.compare_exchange_strong()

This comment has been minimized.

@chardan

chardan May 16, 2017

Contributor

You're correct-- it would be fine if we only ran on architectures that provided cmp/swp instructions, but we can't promise that. I did a fixup PR for a previously-committed PR, and will patch it again here:
#15030 (comment)

@@ -13,6 +13,9 @@
#include "DaemonState.h"
// re-include our assert to clobber the system one; fix dout:
#include "include/assert.h"

This comment has been minimized.

@tchaikov

tchaikov May 16, 2017

Contributor

this change should be folded into the commit which it is fixing.

This comment has been minimized.

@chardan

chardan May 19, 2017

Contributor

Should be corrected now.

@chardan chardan changed the title from misc., test: migrate atomic_t to std::atomic to test: migrate atomic_t to std::atomic May 19, 2017

@@ -23,7 +23,7 @@ using std::ostringstream;
using std::string;
static sem_t *sem;
static atomic_t stop_flag;
static std::atomic<unsigned> stop_flag;

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

nit, why not std::atomic<bool>?

This comment has been minimized.

@chardan

chardan May 19, 2017

Contributor

I think atomic is a better fit. Coming right up!

test: migrate atomic_t to std::atomic<>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

@liewegas liewegas merged commit 499c1af into ceph:master May 24, 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