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

ceph: remove atomic_t #14502

Closed
wants to merge 22 commits into from
Closed

Conversation

chardan
Copy link
Contributor

@chardan chardan commented Apr 13, 2017

This is intended to remove atomic_t from the codebase, replacing uses of
it with the C++ standard library. It should add no features and cause no
regressions. The "include/atomic.h" header file and "common/simple_spin.h"
are removed.

Please be sure to inspect default values that I've assigned to atomics-- I
occasionally guessed-- and check for any undefined behavior I may have
introduced. I'm also not entirely sure if the removal of InstanceReplayer.h
is something I may have done through misuse of git, or something expected.
Guidance appreciated.

I've taken a pretty mechanical approach here-- for example, replacing
read() with load(). I wouldn't mind using std::atomic<>'s type coercion for a
more natural syntax (x = y as opposed to x = y.load()).

Another refinement might be to apply a template to help with situations
where we compare-and-swap, but intended to discard the swapped-to value (
avoiding having to add exposed variables for compare_exchange_*).

Please let me know if you'd like to see either or both of these included
as additional commits.

Thank you!

@chardan
Copy link
Contributor Author

chardan commented Apr 13, 2017

@cbodley @smithfarm @liewegas

{
return pthread_spin_destroy(&l->lock);
lock.clear(std::memory_order_release);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, do we have a micro benchmark result to compare the performance with and without the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet. I'll see what I can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quick-n-dirty, but here's a start. Please feel free to kick the tires:
https://github.com/chardan/ceph-spinlock-minibench

@liewegas
Copy link
Member

Hmm, like other cleanups, this will be easier if we break it up into a few different commits and PRs targetting different subsystems. They'll have to go through different teuthology suites and possibly get rebased to avoid conflicts with other PRs. Can you break this into osd/, mds/, librbd/, rgw/, and then everything else (common)? The last patch that removes the old atomic.h will come separately and last.

@chardan
Copy link
Contributor Author

chardan commented Apr 19, 2017

@liewegas If there are too many separate commits, I can also squish some of them together.

Jesse Williamson added 19 commits April 19, 2017 04:41
…f spin locks.

Removes simple_spin.cc, simple_spin.h; moves spinlock_t -> std::atomic_flag.

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
This commit is a "catch all".

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

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
@liewegas
Copy link
Member

@chardan commits are fine. Can you break it into a few separate PRs though so that one conflict in say rbd doesn't block everything else from test/merge? Thanks!

Jesse Williamson added 2 commits April 19, 2017 06:31
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
@chardan
Copy link
Contributor Author

chardan commented Apr 20, 2017

Further adventures continue at:
#14655

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants